atfork handler violates POSIX

[dwmw2]

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 pthread_atfork() child handler in pkcs11-helper appears to violate this requirement by calling the module's C_Initialize() function in the child. It is not reasonable to assume that C_Initialize() will only call async-signal-safe functions, and thus we shouldn't be doing so from a multi-threaded process.

I know that the non-normative PKCS#11 Usage Guide encourages this behaviour. But POSIX forbids it in certain circumstances, so we need to be able to avoid it.

Even in non-threaded applications, in many cases we are really doing a trivial fork-and-exec, and anything we do in the child is going to be thrown away so the atfork handler is entirely unneeded anyway. And is known to cause problems with various PKCS#11 providers including OpenSC (see https://github.com/OpenSC/OpenSC/issues/333) and p11-kit-proxy (https://bugs.freedesktop.org/show_bug.cgi?id=90289).

Since we wrap all calls into the module, there's no need to re-initialise it "just in case" someone accidentally calls into it from the child process without doing so. A scheme like the p11-kit 'p11_forkid' where the atfork handler merely updates an internal counter and then it gets checked when someone subsequently uses PKCS#11 should probably be sufficient.

[alonbl]

please take this into PKCS#11 forum, if they change the spec pkcs11-helper will also change. thanks.

On 4 June 2015 at 17:21, dwmw2 notifications@github.com wrote:

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 pthread_atfork() child handler in pkcs11-helper appears to violate this requirement by calling the module's C_Initialize() function in the child. It is not reasonable to assume that C_Initialize() will only call async-signal-safe functions, and thus we shouldn't be doing so from a multi-threaded process.

I know that the non-normative PKCS#11 Usage Guide encourages this behaviour. But POSIX forbids it in certain circumstances, so we need to be able to avoid it.

Even in non-threaded applications, in many cases we are really doing a trivial fork-and-exec, and anything we do in the child is going to be thrown away so the atfork handler is entirely unneeded anyway. And is known to cause problems with various PKCS#11 providers including OpenSC (see OpenSC/OpenSC#333 https://github.com/OpenSC/OpenSC/issues/333) and p11-kit-proxy (https://bugs.freedesktop.org/show_bug.cgi?id=90289).

Since we wrap all calls into the module, there's no need to re-initialise it "just in case" someone accidentally calls into it from the child process without doing so. A scheme like the p11-kit 'p11_forkid' where the atfork handler merely updates an internal counter and then it gets checked when someone subsequently uses PKCS#11 should probably be sufficient.

— Reply to this email directly or view it on GitHub https://github.com/OpenSC/pkcs11-helper/issues/5.

[dwmw2]

The PKCS#11 spec does not require this behaviour; it is merely recommended by the non-normative Usage Guide. POSIX, on the other hand, does clearly forbid it. I don't think there's really much doubt that in this case we should be doing what POSIX says.

But since the Usage Guide is clearly recommending a behaviour which is broken, I have indeed taken it up with the PKCS#11 TC: https://lists.oasis-open.org/archives/pkcs11/201505/msg00002.html

They are looking into it, but it was their response which drew my attention to the fact that the Usage Guide is non-normative and thus that "this isn't a specification issue as such": https://lists.oasis-open.org/archives/pkcs11/201505/msg00010.html

[dwmw2]

See also this response in which they say _"the issues you are raising are related to implementation issues with specific vendors and/or some vendors use of pthreadatfork ... and not with the specification itself." : https://lists.oasis-open.org/archives/pkcs11/201505/msg00004.html

It certainly seems to be the case that the PKCS#11 TC don't believe that they are making you violate POSIX in this manner. Getting the Usage Guide changed would of course be useful, but I don't think we should wait for that change before fixing pkcs11-helper to stop violating the POSIX spec.

[alonbl]

As much as I respect your opinion, the spec has precedence. Please work with the right people to resolve your issues.

On 4 June 2015 at 18:22, dwmw2 notifications@github.com wrote:

See also this response in which they say _"the issues you are raising are related to implementation issues with specific vendors and/or some vendors use of pthreadatfork ... and not with the specification itself." : https://lists.oasis-open.org/archives/pkcs11/201505/msg00004.html

It certainly seems to be the case that the PKCS#11 TC don't believe that they are making you violate POSIX in this manner. Getting the Usage Guide changed would of course be useful, but I don't think we should wait for that change before fixing pkcs11-helper to stop violating the POSIX spec.

— Reply to this email directly or view it on GitHub https://github.com/OpenSC/pkcs11-helper/issues/5#issuecomment-108933651.

[dwmw2]

The PKCS#11 spec does not require this behaviour. The POSIX spec forbids it. It is only non-normative PKCS#11 usage guide that recommends it.

I completely agree that the specs should take precedence :)

[bjoernv]

This issue was mentioned here https://community.openvpn.net/openvpn/ticket/538#comment:7

[eszikk]

Is there any solution for this problem? Because the connecting hangs when I want to use a smart card for authenticating.

[alonbl]

Is there any solution for this problem?

The vendor of your card provider should be notified that it does not support C_Initialize after fork per PKCS#11 specification.

[dwmw2]

To make that easier, please could you provide a specific reference to the section of the normative specification that the module is violating?

[alonbl]

In PKCS#11 2.2:

6.6.1    Applications and processes 
In  the  scenario  specified  above,  C  should  actually  call  C_Initialize  whether  or  not  it  
needs  to  use  Cryptoki;  if  it  has  no  need  to  use  Cryptoki,  it  should  then  call  C_Finalize
immediately  thereafter.    This  (having  the  child  immediately  call  C_Initialize  and  then  call 
C_Finalize  if  the  parent  is  using  Cryptoki)  is  considered  to  be  good  Cryptoki  
programming practice, since it can prevent the existence of dangling duplicate resources 
that were created at the time of the fork() call; however, it is not required by Cryptoki. 

Something happened to rsasecurity they assigned this standard to some other body, which released PKCS#11 2.4, I do not know who is actually implementing that, this entire section was removed.

Anywyay, as the only "complex" application left to use this library is openvpn, I will add an API to remove this.

However, I recommend for openvpn (as I recommended long long ago), to remove the entire PKCS#11 support there is no sense to load PKCS#11 provider into privileged process that is not running at user context, the certificate and private key should come from the management interface and acquired by the user interface program. This will secure the openvpn daemon and move the concern to the user interface. For Windows it will be easier to use CryptoAPI, for *NIX it will be easier to use PKCS#11 without forking anything.

[dwmw2]

Something happened to rsasecurity they assigned this standard to some other body, which released PKCS#11 2.4, I do not know who is actually implementing that, this entire section was removed.

I think it just got moved around as the doc was split into parts, and it ended up in the Usage Guide document here: http://docs.oasis-open.org/pkcs11/pkcs11-ug/v2.40/cn02/pkcs11-ug-v2.40-cn02.html#_Toc406759985

I suspect if we look further back in history, I suspect we'll find that this section predates the POSIX requirements around fork() in multi-threaded processes. That "change" in POSIX (which I think was really just describing the status quo, as locks are going to be in an undefined state) turned it into bad advice. As more and more applications end up using threads, the problem becomes more and more apparent.

However, I recommend for openvpn (as I recommended long long ago), to remove the entire PKCS#11 support there is no sense to load PKCS#11 provider into privileged process that is not running at user context

Agreed, this seems like exactly the way to handle this. ISTR the OpenVPN side of this basically already works, and all that's needed is a tool which connects to the management interface and provides the required service?

[alonbl]

Fixed in pkcs11-helper-1.25 when fork mode is set to unsafe.

However, in OpenVPN there is a problem because the deamon initialize itself twice as far as I remember, and knowing the maintainers, nothing has changed in the project for the past 5 years the time I have done rework of this project and failed to push more.

It is on your hands now...

I guess you can perform first initialization when safe fork mode is TRUE then to disable it or perform the all PKCS#11 initialization after the fork.

Once again, I argue that the vendor is not complying with the PKCS#11 spec, the issue should be reported and fixed.

And, many parts of OpenVPN should be re-written, especially, remove all key handling and password handling from the main process and moved into a mandatory management process, which can be either another service or user interface. This will greatly simplify OpenVPN implementation and allow much more complexity compared to existing.

[frankmorgner]

If I'm not mistaken, https://github.com/OpenSC/libp11 has taken the approach to check for a fork (and reinitialize if needed) in every entry point of the library. Maybe, this would also be possible for pkcs11-helper to avoid calling C_Initialize() via the fork handler and being PKCS#11 compliant...

[frankmorgner]

Also, there is this problem

[alonbl]

@frankmorgner : the reinitialize should be done immediately after fork, regardless if PKCS#11 methods are being called. In multi-thread application the fork must happen only when it is "safe", this means that pthread_atfork may perform activities that out of reach of library. In this case, the pkcs11-helper is using pthread_atfork to make sure the to call C_Initialize post fork. The behavior in which a provider performs fork is creating a deadlock.

[frankmorgner]

Have you looked at the problem above?