-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deadlock caused by fork handlers #16
Comments
This is opensc provider which forks during C_Initialize?!?! In what case does it happen? PKCS#11 provider must not fork I can think of so many things that may go wrong in this case. |
Well, I can only tell it happens when I'm trying to use OpenVPN with a Yubikey token. If I rebuild pkcs11-helper with threading support disabled, it appears to work. |
On Mon, Oct 8, 2018 at 8:06 PM Ignat Loskutov ***@***.***> wrote:
Well, I can only tell it happens when I'm trying to use OpenVPN with a Yubikey token. If I rebuild pkcs11-helper with threading support disabled, it appears to work.
How is Yubikey related to OpenSC? I see something with gnome in stack
that is triggered by opensc provider.
See #5, I added support for unsafe fork mode, not sure if openvpn
developers used this already or not.
|
Yubikey has a PIV applet, which is accessed via PKCS#11, as far as I understand. |
It looks like the issue is caused by the notifications UI: https://github.com/OpenSC/OpenSC/blob/master/src/ui/notify.c#L446 |
So, if forks are forbidden inside |
PKCS#11 spec requires to perform C_Initialize for all loaded providers post fork(), and C_Finalize if providers are not being used. The information of what providers are loaded by the process is not something a single provider has access to. Performing fork() also inherit resources to child process, and in some cases may allow child process to access cryptographic resources. A PKCS#11 provider that performs fork() violates the PKCS#11 spec and may cause lockout of parent process. I am unsure how to handle this within the constraint of the standard and common sense, in any case please try to patch openvpn to use unsafe fork #5 and hope for the best... One alternative to fix this is to have a separate program of pinentry that listens to unix domain socket within user's home directory (or /run/user/$uid) and for the PKCS#11 provider to connect into that program in order to query for credentials. |
@frankmorgner : see this one as well. |
Violating PKCS#11 by forking is debatable. After all, its pkcs11-helper that deadlocks itself, because it doesn't expect a |
Would be possible to fix this issue? It blocks us from using Yubikey with OpenVPN. |
On Tue, Feb 26, 2019 at 7:05 PM Radosław Biernacki ***@***.***> wrote:
Would be possible to fix this issue? It blocks us from using Yubikey with
OpenVPN.
Does yubikey also forks in C_Initialize?
|
My usecase is also using OpenVPN with Yubikey, which is worked around by building pkcs11-helper without threading support. |
On Tue, Feb 26, 2019 at 7:21 PM Ignat Loskutov ***@***.***> wrote:
My usecase is also using OpenVPN with Yubikey, which is worked around by
building pkcs11-helper without threading support.
Different issue, please see[1], openvpn should use unsafe forks after
forking of the daemon or just use unsafe forks by calling
pkcs11h_setForkMode(FALSE) to support providers that violate the PKCS#11
spec.
However, it is best if you report yubikey that they violate the PKCS#11
spec if they do not allow C_Initialize/C_Finalize sequence in child to
terminate the PKCS#11.
[1]
9b8debf
|
Not sure what you mean by "Yubikey". Cannot find debug symbols for OpenVPN but the stacktrace is following: |
On Tue, Feb 26, 2019 at 7:42 PM Radosław Biernacki ***@***.***> wrote:
Not sure what you mean by "Yubikey".
I'm using Yubikey 4 device with OpenVPN and OpenSC together with PIV
profile and pkcs#11 (right?)
The Yubikey device itself is as any other reader and smartcard. There is
no additional SW from Yubico in the stack.
Thanks!
OK, so you use OpenSC software which forks within the provider which is
illegal. This is new, in the past it was only at C_Initialize.
I guess it tries to display a popup, however, the provider should not
interact with the user it should either accept the PIN from the C_Login or
support protected authentication which should interact with user using a
different method.
As fork is not allowed in PKCS#11 (or any library), the right solution
would be similar to the way agents are working, there should be a unix
domain socket on which the user interface is listening and an environment
variable with the location, the PKCS#11 provider should interact with the
user interface via the socket, avoiding fork which may lockdown parent
application and may lead to sensitive handle leak.
|
Thank you for reply.
Yes this is exactly what is happening. My card is protected by PIN. Do not know the guts of OpenSC but as a workaround I'm using following config in OpenVPN: This creates TCP socket and I interact with it so I can provide PIN beforehand (it does not try to display popup I guess). |
The issue in opensc-notify remains unfixed, unfortunately: OpenSC/OpenSC#1507 |
On Wed, Feb 27, 2019 at 12:05 PM Frank Morgner ***@***.***> wrote:
@alonbl <https://github.com/alonbl> please point out where it's forbidden
to fork in C_Initialize
We already discussed that many times I think....
"""
6.5.1 Applications and processes
In general, on most platforms, the previous paragraph means that an
application consists of a single process.
Consider a UNIX process P which becomes a Cryptoki application by calling
C_Initialize, and then uses the fork() system call to create a child
process C. Since P and C have separate address spaces (or will when one of
them performs a write operation, if the operating system follows the
copy-on-write paradigm), they are not part of the same application.
Therefore, if C needs to use Cryptoki, it needs to perform its own
C_Initialize call. Furthermore, if C needs to be logged into the token(s)
that it will access via Cryptoki, it needs to log into them even if P
already logged in, since P and C are completely separate applications.
In this particular case (when C is the child of a process which is a
Cryptoki application), the behavior of Cryptoki is undefined if C tries to
use it without its own C_Initialize call. Ideally, such an attempt would
return the value CKR_CRYPTOKI_NOT_INITIALIZED; however, because of the way
fork() works, insisting on this return value might have a bad impact on the
performance of libraries. Therefore, the behavior of Cryptoki in this
situation is left undefined. Applications should definitely not attempt to
take advantage of any potential “shortcuts” which might (or might not!) be
available because of this.
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.
"""
If your provider forks out of C_Initialize you get infinite recursion as
the child must call C_Initialize/C_Finalize to cleanup resources... nor
that you can perform the cleanup for the other providers and/or sensitive
components that exists in the process as you are not aware of them, causing
severe leakage of sensitive resources to the child process.
A library is not expected to fork without main consent, allowing main to
perform the necessary preparations.
I suggest that if you need notification, establish a unix domain socket for
that purpose.
|
if (pid=fork()==0){
C_Initialize();
C_Finalize();
} Anyway, changing the default behavior of OpenSC to not fork should be OK. |
On Thu, Feb 28, 2019 at 9:39 AM Frank Morgner ***@***.***> wrote:
1. The quoted paragraph doesn't specify a *mandatory* requirement
It specifies a best practice for main consumer of the provider, the
provider implementation must make no assumption how main is implemented.
1. The quoted paragraph doesn't specify usage of an *atfork* handler.
It specifies what main should do when forking, main can use any method it
likes to comply, as main may fork its own processes.
1. The following solution would equally satisfy the *recommended*
requirement without infinite recursion.
if (pid=fork()==0){
C_Initialize();
C_Finalize();
}
Once again... there may be 10 providers in the process...
If C_Initialize forks, it may fork the process with providers that have not
been finalized, causing sensitive resource leak.
If the main uses atfork to comply with requirement of never leak when fork
is called, and if the C_Initialize forks, the fork it will trigger the same
code cause infinite recursion.
A library cannot fork, this is bad practice, the process may have
capabilities and/or permissions and/or sensitive handles that should not be
inherited by any other process, especially user interface programs. The
library, especially low-level cryptography library, has no visibility of
the grand plan of the main process to make decision of sharing the context.
I do agree that the PKCS#11 should have a C_Detach or similar method to
detach a child in child instead of adding the confusion of
re-initialization, but this will not change the above statement.
Anyway, changing the default behavior of OpenSC to not fork should be OK.
Thanks!
|
resolve a deadlock caused when provider fork in C_Initialize/C_Finalize. Thanks: Frank Morgner <[email protected]> Bug: OpenSC/OpenSC#1507 Bug: OpenSC#16
I was finally able to reproduce the problem with fork and found and fixed an other problem (OpenSC/OpenSC@7449b00). OpenVPN should work in OpenSC's and pkcs11-helper's master now. |
It seems to fix the problem for me, thanks! (However, I have another deadlock, which is probably a subject for another issue.) |
What's the problem? |
|
I'm experiencing a deadlock; a fancified stacktrace is below. It looks like it's a bad idea to call library functions (which can call
fork
internally) having the fork-mutexes locked.The text was updated successfully, but these errors were encountered: