OpenSC / pkcs11-helper

Library that simplifies the interaction with PKCS#11 providers for end-user applications using a simple API and optional OpenSSL engine
Other
66 stars 43 forks source link

Deadlock caused by fork handlers #16

Closed loskutov closed 5 years ago

loskutov commented 6 years ago

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.

#0  0x00007feefe2196a0 in __GI___nanosleep
    at ../sysdeps/unix/sysv/linux/nanosleep.c:28
#1  0x00007feefe24c7d4 in usleep (useconds=<optimized out>)
    at ../sysdeps/posix/usleep.c:32
#2  0x00007feefe716232 in _pkcs1h_threading_mutexLockAll ()
    at https://github.com/OpenSC/pkcs11-helper/blob/master/lib/pkcs11h-threading.c#L305
#3  0x00007feefe264578 in __run_fork_handlers
    (who=who@entry=atfork_run_prepare) at register-atfork.c:122
#4  0x00007feefe219707 in __libc_fork () at ../sysdeps/nptl/fork.c:58
#5  0x00007feefcf22712 in fork_exec_with_fds
    (intermediate_child=intermediate_child@entry=0, working_directory=working_directory@entry=0x0, argv=argv@entry=0x5650d153d1e0, envp=envp@entry=0x0, close_descriptors=close_descriptors@entry=1, search_path=search_path@entry=1, search_path_from_envp=0, stdout_to_null=0, stderr_to_null=0, child_inherits_stdin=0, file_and_argv_zero=0, cloexec_pipes=0, child_setup=0x0, user_data=0x0, child_pid=0x7ffc22321898, child_close_fds=0x7ffc223217b0, stdin_fd=-1, stdout_fd=5, stderr_fd=7, error=0x7ffc22321a58) at https://github.com/GNOME/glib/blob/master/glib/gspawn.c#L1647
#6  0x00007feefcf22fab in fork_exec_with_pipes
    (intermediate_child=intermediate_child@entry=0, working_directory=working_directory@entry=0x0, argv=0x5650d153d1e0, envp=envp@entry=0x0, close_descriptors=close_descriptors@entry=1, search_path=search_path@entry=1, search_path_from_envp=0, stdout_to_null=0, stderr_to_ro=0, cloexec_pipes=0, child_setup=0x0, user_data=0x0, child_pid=0x7ffc22321898, standard_input=0x0, standard_output=0x7ffc22321890, standard_error=0x7ffc22321894, error=0x7ffc22321a58) at https://github.com/GNOME/glib/blob/master/glib/gspawn.c#L1958
#7  0x00007feefcf233c6 in g_spawn_sync
    (working_directory=working_directory@entry=0x0, argv=<optimized out>, envp=envp@entry=0x0, flags=flags@entry=G_SPAWN_SEARCH_PATH, child_setup=child_setup@entry=0x0, user_data=user_data@entry=0x0, standard_output=0x7ffc223219e8, standard_error=0x7ffc223219f0, exit_status=0x7ffc223219e4, error=0x7ffc22321a58)
    at https://github.com/GNOME/glib/blob/master/glib/gspawn.c#L394
#8  0x00007feefcf23c07 in g_spawn_command_line_sync
    (command_line=command_line@entry=0x5650d153d100 "dbus-launch --autolaunch=790ef33180164b2a91d23c8c584cb1c2 --binary-syntax --close-stderr", standard_output=standard_output@entry=0x7ffc223219e8, standard_error=standard_error@entry=0x7ffc223219f0, exit_status=exit_status@entry=0x7ffc223219e4, error=error@entry=0x7ffc22321a58) at https://github.com/GNOME/glib/blob/master/glib/gspawn.c#L935
#9  0x00007feefd0b5cd0 in get_session_address_dbus_launch (error=error@entry=0x7ffc22321a58) at https://github.com/GNOME/glib/blob/master/gio/gdbusaddress.c#L1131
#10 0x00007feefd0b780a in get_session_address_platform_specific (error=0x7ffc22321a58) at https://github.com/GNOME/glib/blob/master/gio/gdbusaddress.c#L1559
#11 0x00007feefd0b780a in g_dbus_address_get_for_bus_sync (bus_type=bus_type@entry=G_BUS_TYPE_SESSION, cancellable=cancellable@entry=0x0, error=error@entry=0x0)
    at https://github.com/GNOME/glib/blob/master/gio/gdbusaddress.c#L1639
#12 0x00007feefd0c2d96 in get_uninitialized_connection (bus_type=bus_type@entry=G_BUS_TYPE_SESSION, cancellable=cancellable@entry=0x0, error=error@entry=0x0)
    at https://github.com/GNOME/glib/blob/master/gio/gdbusconnection.c#L7187
#13 0x00007feefd0c86ad in g_bus_get_sync (bus_type=bus_type@entry=G_BUS_TYPE_SESSION, cancellable=cancellable@entry=0x0, error=error@entry=0x0)
    at https://github.com/GNOME/glib/blob/master/gio/gdbusconnection.c#L7282
#14 0x00007feefd0a95fa in g_application_impl_register
    (application=application@entry=0x5650d1533890 [GApplication], appid=0x5650d1533790 "org.opensc.notify", flags=G_APPLICATION_NON_UNIQUE, exported_actions=0x5650d152ccd0, remote_actions=remote_actions@entry=0x5650d1533838, cancellable=cancellable@entry=0x0, error=0x0) at https://github.com/GNOME/glib/blob/master/gio/gapplicationimpl-dbus.c#L545
#15 0x00007feefd0a6464 in g_application_register (application=0x5650d1533890 [GApplication], cancellable=0x0, error=0x0) at https://github.com/GNOME/glib/blob/master/gio/gapplication.c#L2120
#16 0x00007feefd7a21e5 in C_Initialize () at /usr/lib/x86_64-linux-gnu/opensc-pkcs11.so
#17 0x00007feefe71f6e9 in pkcs11h_addProvider
    (reference=0x5650d1515008 "/usr/lib/x86_64-linux-gnu/opensc-pkcs11.so", provider_location=<optimized out>, allow_protected_auth=0, mask_private_mode=0, slot_event_method=0, slot_poll_interval=0, cert_is_private=0) at https://github.com/OpenSC/pkcs11-helper/blob/master/lib/pkcs11h-core.c#L778
alonbl commented 6 years ago

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.

loskutov commented 6 years ago

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.

alonbl commented 6 years ago

On Mon, Oct 8, 2018 at 8:06 PM Ignat Loskutov notifications@github.com 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.

loskutov commented 6 years ago

Yubikey has a PIV applet, which is accessed via PKCS#11, as far as I understand.

loskutov commented 6 years ago

It looks like the issue is caused by the notifications UI: https://github.com/OpenSC/OpenSC/blob/master/src/ui/notify.c#L446 https://github.com/OpenSC/OpenSC/blob/master/src/pkcs11/pkcs11-global.c#L241

loskutov commented 6 years ago

So, if forks are forbidden inside C_Initialize, does it mean it's an OpenSC issue or OpenVPN should be aware of fork-unsafe providers and use the fork-unsafe mode of pkcs11-helper (which it currently doesn't)?

alonbl commented 6 years ago

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.

alonbl commented 6 years ago

@frankmorgner : see this one as well.

frankmorgner commented 6 years ago

Violating PKCS#11 by forking is debatable. After all, its pkcs11-helper that deadlocks itself, because it doesn't expect a fork inside the atfork handler. I've created https://github.com/OpenSC/pkcs11-helper/pull/17 to work around this limitation and to hopefully fix this issue (currently untested).

rosly commented 5 years ago

Would be possible to fix this issue? It blocks us from using Yubikey with OpenVPN.

alonbl commented 5 years ago

On Tue, Feb 26, 2019 at 7:05 PM Radosław Biernacki notifications@github.com wrote:

Would be possible to fix this issue? It blocks us from using Yubikey with OpenVPN.

Does yubikey also forks in C_Initialize?

loskutov commented 5 years ago

My usecase is also using OpenVPN with Yubikey, which is worked around by building pkcs11-helper without threading support.

alonbl commented 5 years ago

On Tue, Feb 26, 2019 at 7:21 PM Ignat Loskutov notifications@github.com 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] https://github.com/OpenSC/pkcs11-helper/commit/9b8debf331d7bd5eda1fa6feb322c0e31657e9b5

rosly commented 5 years ago

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.

Cannot find debug symbols for OpenVPN but the stacktrace is following: (gdb) bt

0 0x00007ffff6726bb1 in __GI___nanosleep (requested_time=requested_time@entry=0x7fffffff6c90, remaining=remaining@entry=0x0) at ../sysdeps/unix/sysv/linux/nanosleep.c:27

1 0x00007ffff6759184 in usleep (useconds=) at ../sysdeps/posix/usleep.c:32

2 0x00007ffff7563ec2 in ?? () from /usr/lib/x86_64-linux-gnu/libpkcs11-helper.so.1

3 0x00007ffff6726cb5 in __libc_fork () at ../sysdeps/nptl/fork.c:96

4 0x00005555555853a3 in ?? ()

5 0x00005555555865af in ?? ()

6 0x0000555555583dc3 in ?? ()

7 0x00005555555921b5 in ?? ()

8 0x00007ffff7565f01 in ?? () from /usr/lib/x86_64-linux-gnu/libpkcs11-helper.so.1

9 0x00007ffff756763d in ?? () from /usr/lib/x86_64-linux-gnu/libpkcs11-helper.so.1

10 0x00007ffff7569e35 in pkcs11h_certificate_getCertificateBlob () from /usr/lib/x86_64-linux-gnu/libpkcs11-helper.so.1

11 0x00007ffff7570ec2 in pkcs11h_openssl_getX509 () from /usr/lib/x86_64-linux-gnu/libpkcs11-helper.so.1

12 0x00007ffff7571480 in pkcs11h_openssl_session_getX509 () from /usr/lib/x86_64-linux-gnu/libpkcs11-helper.so.1

13 0x00007ffff7571539 in pkcs11h_openssl_session_getEVP () from /usr/lib/x86_64-linux-gnu/libpkcs11-helper.so.1

14 0x0000555555593889 in ?? ()

15 0x0000555555593242 in ?? ()

16 0x00005555555c5574 in ?? ()

17 0x000055555557478b in ?? ()

18 0x0000555555579098 in ?? ()

19 0x0000555555579bc0 in ?? ()

20 0x0000555555593ea8 in ?? ()

21 0x00007ffff666f1c1 in __libc_start_main (main=0x55555555fc10, argc=3, argv=0x7fffffffdef8, init=, fini=, rtld_fini=, stack_end=0x7fffffffdee8) at ../csu/libc-start.c:308

22 0x000055555555fc4a in ?? ()

alonbl commented 5 years ago

On Tue, Feb 26, 2019 at 7:42 PM Radosław Biernacki notifications@github.com 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.

rosly commented 5 years ago

Thank you for reply.

I guess it tries to display a popup

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: management-hold management 127.0.0.1 8888 management-query-passwords

This creates TCP socket and I interact with it so I can provide PIN beforehand (it does not try to display popup I guess). If this is a different BUG thank sorry for polluting this thread :/

loskutov commented 5 years ago

The issue in opensc-notify remains unfixed, unfortunately: OpenSC/OpenSC#1507

frankmorgner commented 5 years ago

@alonbl please point out where it's forbidden to fork in C_Initialize

@loskutov, @rosly, you can work around this issue by configuring OpenSC with --disable-notify.

alonbl commented 5 years ago

On Wed, Feb 27, 2019 at 12:05 PM Frank Morgner notifications@github.com 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.

frankmorgner commented 5 years ago
  1. The quoted paragraph doesn't specify a mandatory requirement
  2. The quoted paragraph doesn't specify usage of an atfork handler. The following solution would equally satisfy the recommended requirement without infinite recursion.
    if (pid=fork()==0){
    C_Initialize();
    C_Finalize();
    }

Anyway, changing the default behavior of OpenSC to not fork should be OK.

alonbl commented 5 years ago

On Thu, Feb 28, 2019 at 9:39 AM Frank Morgner notifications@github.com 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!

frankmorgner commented 5 years ago

I was finally able to reproduce the problem with fork and found and fixed an other problem (https://github.com/OpenSC/OpenSC/commit/7449b007688ccce16eb8265cd82a69a945fd38f5). OpenVPN should work in OpenSC's and pkcs11-helper's master now.

loskutov commented 5 years ago

It seems to fix the problem for me, thanks! (However, I have another deadlock, which is probably a subject for another issue.)

frankmorgner commented 5 years ago

What's the problem?

alonbl commented 5 years ago

What's the problem?

https://github.com/OpenSC/pkcs11-helper/issues/24