Use vfork() in openvpn_execve() instead of fork()

The pkcs11-helper library installs a pthread_atfork() handler which, in the child, will call the C_Initialize() method on any PKCS#11 provider module which is active in the parent.

I'm going to assume that Alon knows what he's doing and that this is actually the correct thing for pkcs11-helper to be doing. Nevertheless, it does seem to be central to a number of issues.

Firstly, OpenVPN is forking to spawn systemd-ask-password from *within* the PIN request callback, which is expressly forbidden and leads directly to a deadlock.

Secondly, even if we work around that by disabling systemd support, we find that the OpenSC PKCS#11 module doesn't cope. It ends up confusing pcscd by interacting with it over two separate connections, as described at
http://sourceforge.net/p/opensc/mailman/message/34073754/

Third, the p11-kit-proxy.so module also doesn't cope, and suffers an internal deadlock as described at
https://www.mail-archive.com/[email protected].../msg00126.html

Even if this behaviour of calling C_Initialize() in the child is strictly correct, it doesn't seem that we can rely on provider modules to cope with it in the general case. Perhaps it would be safer to not actually reinitialise the modules until later, if they are really going to be accessed again?

Hi Alon, thanks for the quick response and the citation from the spec; I had looked briefly and not yet found that.

It's quite clear, as I had assumed, that your code in pkcs11-helper is doing the right thing.

That's why we have callbacks. You call back from pkcs11-helper into the application when you need a PIN. As you say, the point in time that such is required is unknown. Which is why we can't make life easy by asking for it in advance.

I'm aware of the misconception you mention above, but I'm not sure why you mention it here. OpenVPN doesn't seem to be guilty of it.

It's a tool which gets spawned to ask the user (sysadmin) for a password. I've never actually seen it working in practice, but I think it's something vaguely similar to ssh-askpass.

I think it's designed to work even during a graphical boot sequence before any real UI is present (for example for asking for root file system encryption passphrases).

I am unfamiliar with the general design decisions around OpenVPN's interaction with the user — I'm only trying to fix up the PKCS#11 support. But I think the management interface is *optional*.

Users are seeing this particular issue even when OpenVPN is being run in the foreground, and as non-root users. If I create a persistent tun device and run as myself with --ifconfig-noexec, it still happens.

Perhaps one might argue this is simply a bug in OpenVPN — it *shouldn't* be spawning the systemd password request tool in that case. And that's 100% true for the PIN request, since the pkcs11 -helper documentation explicitly forbids the application from forking in the PIN callback.

I think the management interface has been able to provide a certificate for a while. It's recently also gained the ability to perform the signing step too, so we really can offload it all now.

Persuading the OpenVPN developers that such should be the *only* possible mode of operation, however, is a different prospect :0

Thanks for confirming that. I'll go back and resume that thread and point the finger more squarely away from OpenVPN and pkcs11-helper :)

Indeed. Thanks.

Although I do wonder how many other provider modules are going to suffer similar issues, and it doesn't *hurt* to be defensive in OpenVPN and use vfork() instead of fork(). I don't really see the down -side.

Well, not quite. Even apart from the 'be liberal in what you accept' principle as discussed above, the systemd_ask_password handling is clearly broken. And actually further testing shows that part is *not* fixed by my patch, since I only modified openvpn_execve(). We can't use vfork() in openvpn_popen() because that does other things between the fork and the execve().

So we end up with this deadlock...

#0  0x00007f26e64a52c0 in __nanosleep_nocancel () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007f26e64d5b14 in usleep (useconds=<optimized out>) at ../sysdeps/unix/sysv/linux/usleep.c:32
#2  0x00007f26e6ffb096 in _pkcs1h_threading_mutexLockAll () at pkcs11h-threading.c:306
#3  0x00007f26e64a5362 in __libc_fork () at ../sysdeps/nptl/fork.c:89
#4  0x00007f26e72224a5 in __fork () at pt-fork.c:25
#5  0x00007f26e8ff05e0 in openvpn_popen ([email protected]...=0x7ffdb20c87a0, [email protected]...=0x0) at misc.c:373
#6  0x00007f26e8ff23fd in get_console_input_systemd (capacity=4096, input=0x7ffdb20c9972 "", echo=<optimized out>, prompt=0x7f26e9e95d88 "Enter PIV_II (PIV Card Holder pin) token Password:") at console.c:172
#7  get_console_input (prompt=0x7f26e9e95d88 "Enter PIV_II (PIV Card Holder pin) token Password:", [email protected]...=false, [email protected]...=0x7ffdb20c9972 "", [email protected]...=4096) at console.c:205
#8  0x00007f26e8fef7a8 in get_user_pass_cr ([email protected]...=0x7ffdb20c8970, auth_file=<optimized out>, [email protected]...=0x0, [email protected]...=0x7ffdb20ca980 "PIV_II (PIV Card Holder pin) token", [email protected]...=21, [email protected]...=0x0) at misc.c:1146
#9  0x00007f26e8ffcfc8 in get_user_pass (flags=21, prefix=0x7ffdb20ca980 "PIV_II (PIV Card Holder pin) token", auth_file=0x0, up=0x7ffdb20c8970) at misc.h:272

#10 _pkcs11_openvpn_pin_prompt (global_data=<optimized out>, user_data=<optimized out>, token=<optimized out>, retry=<optimized out>, pin=0x7ffdb20cadf0 "@\260\f\262\375\177", pin_max=1024) at pkcs11.c:235 #11 0x00007f26e6ffd0cb in _pkcs11h_session_login (session=0x7f26e9e41a30, [email protected]...=0, [email protected].....=1, user_data=0x0, mask_prompt=<optimized out>) at pkcs11h-session.c:1023

David Woodhouse                            Open Source Technology Centre
[email protected]...                              Intel Corporation

systemd-ask-password is a utility which prepares systemd to query the user for some information, usually a password or PIN.

Very simply said, it prepares a pipe to a front-end which does the real querying. So consider systemd-ask-password the back-end side. The front-end presents the prompt it received from the back-end, collects the user input and sends the result back to the forked out process which then ends up in OpenVPN in the end. The idea around this concept is that systemd (via for example systemd-tty-ask-password-agent) may not need to kick off the front-end if it already have cached the information needed.

With that said, I mentioned earlier today that there is some talks about improving the whole query interface in systemd, where a DBus interface will be available. Thus removing the need to fork out this little helper program.

That is correct.

I agree this is a misbehaviour in OpenVPN, when it does things which is clearly stated what is forbidden.

Yes, there are some features available via the management interface which allows an external process to do the RSA signing and verification, iirc. I also believe interface is used on Android, to be able to make usage of the keystorage in Android.

If it breaks existing users configs and setups, it's generally a no-go.
We've already tried that a few times, and reverted when enough users
started complaining.

I've spent my evening reading more about vfork() and fork(). I've based my trust this time in two books [1] on Linux system programming.

Both books are really clear that vfork() should be avoided, and even claiming it was a mistake by introducing that syscall in Linux. Its semantic can in many cases cause other issues, such as the execve()'d process not using _exit() but exit(). This can cause plenty of mess in regards to flushing of buffers and other file descriptors.

The reason I believe vfork() "works" and fork() "does not work" is simply due to the fact that fork() does a copy-on-write-copy to the child, which includes the inheritance of the atexit() handlers. I believe also that p11-helper also uses pthreads_atexit(), which is also executed when a fork()ed process exits. When uysing vfork(), this copy does not happen at all, and then execve() takes over completely. So when vfork() exits, it doesn't do anything than just "die" (*as long as* _exit() is used). It is in fact utterly important that exit() is not used in a process which has been vfork()ed, it has to be _exit() to avoid undefined behaviours.

Then regarding to standards and portability. SUSv3/POSIX.1-2001 lists vfork() as deprecated, and SUSv4/POSIX.1-2008 doesn't even include vfork().

[1] - Linux System Programming, Rob Love
- The Linux Programming Interface, Michael Kerrisk

I acknowledge that you have found a potential solution for solving this issue. I'm grateful for that! But I do question if it is the proper solution.

The current patch gets a NAK from me as well. When two system programming books recommend strongly /not/ to use vfork() but fork(), plus the deprecation in SUS, it scares me too just much to make this change on all systems which supports vfork() - not just Linux systems with systemd.

As a temporarily hack, I could maybe accept it if we can contain the fork() to vfork() change to a scenario where ENABLE_SYSTEMD in addition to HAVE_VFORK_H/HAVE_WORKING_VFORK; I'll have to sleep on that for a couple of days first. But this may just as well get NAK from the other developers.

Anyone got better ideas? The Management interface is one option which has been mentioned, but I am not too happy about that implementation (mentioned in another e-mail). But if it can be made to work more smoothly, I prefer that over using a deprecated libc/system call.

kind regards,

David Sommerseth

That's mostly a concern if you only action in the child isn't to call execve().

On vfork() the parent stops until the child (sharing its VM) either exits or calls execve().

It is the case the the child process directly exits that you have most fun. We weren't doing that.

No, the child just execs something else, so *everything* about the parent is lost. The reason it works is that *during* the fork, the pthread_atfork handlers (which call C_Initialize on PKCS#11 modules in the child right before it's all going to be thrown away on exec anyway) are not called for a real fork() but not vfork()

It is calling C_Initialize() "again" which breaks things (for buggy providers).

Well that's a better reason to NAK it than an assertion that PKCS#11 provider modules must all be perfect and we don't want to tolerate anything less :)

Those are separate issues; my vfork patch has nothing to do with systemd.

Using vfork stops breakage, even without systemd, when we later fork to run stuff like ifconfig. In this case it is *only* working around misbehaving providers.

For the systemd thing, the only workaround we have for now is just to disable it. We can't uss vfork() in openvpn_popen() because there we *do* do something else between the fork and the exec.

Well, since we know we never will use PKCS#11 from a child after fork() it would be quite nice to have a way to get pkcs11-helper *not* to do the atfork thing. Using vfork() was just a cheap hack to avoid that.

This *is* supposed to be usable software, rather than a test/torture suite for PKCS#11 providers, after all :)

dwmw2

Looking back at this with the benefit of a (slightly) larger screen, about a pint of coffee, and an absence of small children, I find I was not at my most coherent. Let me try that bit again...

The interesting caveats of vfork should not bother us when all we do in the child is exec. Everything is thrown away then anyway. However my patch *would* need to be careful in the case where the exec fails, including calling _exit at you say. And any logging or error reporting it does in the child would also be "fun" with the vfork restrictions.

That in itself is probably sufficient reason to drop the idea.

It was only a cheap hack that seemed useful. As noted, it wasn't even a panacea because we couldn't use it in openvpn_popen() and thus it didn't address the systemd thing.

A better approach would probably be to disable the atfork handlers in OpenVPN entirely since I believe we don't need them.

dwmw2

Hi,

With the patch to #480 which moves the only "real fork()" (daemon()) to "before any crypto stuff happens", we should never do any fork() now which will cause crypto operations in the child - so, if these handlers can be disabled (or turned into "assume there will be no crypto in the child, so just plain FAIL if any is attempted"), this could be a possible avenue out of this mess...

I'm not sharing David S.'s concern about vfork() being deprecated, but if it is not actually fixing all aspects of the problem, it's not the right solution anyway - so, no need to really decide about that :-)

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

I think that's the best approach, yes.

Yeah, I'm not really scared of vfork, although my patch should have switched to using _exit() instead of exit() if the execve() failed in the child. I note there *was* no other error handling to worry about in that case, so my concern about complexity there was unfounded.

Really, it depends on how we solve the other issues. If we kill openvpn_popen() and make the systemd thing work some other way (via DBus or whatever), then we have fixed the outright *bug* in OpenVPN. But we are still left with the issue that some of the *common* PKCS#11 providers have issues with being re-initialised in the child process after a fork (like when we spawn ifconfig).

As noted above, the best answer is *not* to do that. But I strongly suspect Alon (who owns pkcs11-helper) won't concur. For reasons I can understand, if not entirely agree with, he's very keen on requiring absolute spec compliance from all PKCS#11 provider modules. He doesn't like the idea of working around bugs in them.

He is right, of course, that we should *fix* them (where they're open source and we can). But I personally think that OpenVPN should be tolerant of the known issues *too*. As I said, it's supposed to be a useful piece of software in *practice*, not just a PKCS#11 test suite.

So if we can't persuade him to give us a mode in pkcs11-helper which disables this pthread_atfork() behaviour, then using vfork() — just to avoid *triggering* that unwanted atfork handler — might still be a relatively attractive proposition.

(There's already the 'safe mode' thing in pkcs11-helper but I don't think that actually avoids invoking C_Initialize() on the modules.)

dwmw2

Those should be opened with O_CLOEXEC anyway.

Indeed :)

I posted a proof-of-concept patch for OpenSC¹ but it was more of a demonstration of where the problem lies than a real fix. Partly because it *does* still leak file descriptors, AFAICT.

If an application *knows* that it will never use PKCS#11 after a fork(), as in this case where we *know* that we're always just going to exec something else, it certainly doesn't *damage* the well-behaved providers if we simply refrain from making that C_Initialize() call that we *know* to be gratuitous in the child.

dwmw2

¹ http://sourceforge.net/p/opensc/mailman/message/34086897/

Can you explain where I'm wrong?

If there is a specific case in which we *know* that all we're ever going to do after a fork() is exec something else, so *everything* is going to be thrown away...

... and if we *know* that calling C_Initialize() in the child, which we know is gratuitous in our case, is actually causing problems in some fairly common PKCS#11 provider modules...

... and if we *know* that these bugs have actually been fixed before and they keep recurring...

... and if we *know* that even if we fix the bugs again, people will still be using the existing buggy versions for some time...

... why in $DEITY's name would we not want to — in accordance with Postel's Law — *avoid* the gratuitous call to C_Initialize() which is triggering the issue?

dwmw2

That thread is interesting; thanks for the reference.

In it, Stef pointed out¹ that the behaviour of automatically calling C_Initialize() from the atfork child handler is forbidden by POSIX and triggers undefined behaviour.

So I'm quite happy to drop the 'test suite' vs. 'real program' argument, as you requested. It's not just a subjective quality-of-implementation issue; what pkcs11-helper is currently doing, when it does it from a multi-threaded application, is just plain wrong. :)

The pthread_atfork() documentation² says:

"It is suggested that programs that use fork() call an exec function
very soon afterwards in the child process, thus resetting all states. In the meantime, only a short list of async-signal-safe library routines are promised to be available."

It then goes on to explain why pthread_atfork() doesn't really work for its original intended purpose, before concluding more forcefully and using the word 'requires' instead of 'suggested':

"As explained, there is no suitable solution for functionality which
requires non-atomic operations to be protected through mutexes and locks. This is why the POSIX.1 standard since the 1996 release requires that the child process after fork() in a multi-threaded process only calls async-signal-safe interfaces."

The fork() documentation³ does indeed say that, as well as:

"When the application calls fork() from a signal handler and any of the
fork handlers registered by pthread_atfork() calls a function that is not async-signal-safe, the behavior is undefined."

The "short list of async-signal-safe library functions" is indeed short⁴.

It certainly doesn't seem to be the case that C_Initialize() is guaranteed to call only functions on that short list. So calling C_Initialize() from within the atfork child handler would appear to trigger undefined behaviour according to POSIX.

Might it be better to simply set a flag to indicate that the state is invalid, and then re-initialize the module *if* a further call would need to be made into a PKCS#11 provider?

In most cases no such call is going to happen, so calling C_Initialize() immediately is at best a gratuitous performance hit, and at worst it's triggering more serious issues.

dwmw2

¹ http://lists.gnu.org/archive/html/gnutls-devel/2011-10/msg00115.html
² http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_atfork.html
³ http://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html
⁴ http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03

I completely understand your point of view, however I don't really agree with the conclusion.

OpenVPN is not a test suite designed to test for absolute compliance with all esoteric aspects of the PKCS#11 specification, so it isn't useful for it to just blow up whenever anything is slightly wrong.

OpenVPN is a tool which people want to *work*. If there's a trivial change which can make it more tolerant of issues in the software it has to interoperate with, then we should make that change.

I have filed a separate ticket #549 for this, since I was wrong to attribute it to #538 — #538 really is a bug in OpenVPN since it's blatantly forking in a callback which is documented as not permitting that. And the vfork() trick can't easily be applied in that case since we need to do other things between the fork and the exec.

dwmw2