opensc-notify: forks inside C_Initialize
#1507
Comments
|
You can compile OpenSC with |
|
@dwmw2 has already argued in OpenSC/pkcs11-helper#5 that |
|
You mean, who locks the mutex in the first place? However, I don't have a good understanding of the PKCS#11 spec and whether the Interface Usage Guide should be considered a part of it, but pkcs11-helper is obviously designed having this requirement in mind. Given it's a part of the OpenSC project, I suppose there should be some consistent understanding of this issue. |
|
I didn't realize that it is already the initialization that fails. I've moved that part to the load time of the library (even before C_Initialize). If that doesn't work, I think there's no other possibility than to move it to C_GetSlotList... |
|
Even before |
I just realize that pkcs11-helper disallows forking in any PKCS#11 provider this way, because every PKCS#11 library call is protected with a mutex. This is very restrictive. For tasting this forbidden fruit, we could add a daemon mode to opensc-notify outside of the PKCS#11 module (which we already have for Windows), but unfortunately I don't currently have time for that. I think initializing OpenSC at @alonbl, what do you think? |
|
@frankmorgner: you present this as pkcs11-helper issue, while PKCS#11 is a standard... the problem of opensc forks within the provider is a generic common violation an unexpected behavior of PKCS#11 provider or any standard library. I am truly sorry, but having a code specific for a provider implementation only because there is no time to solve it properly is not a good reason, the fact that OpenSC causes leak of handles for any application into subprocess or if daemon runs as root then user interface programs runs as root, is not something that people should accept, regardless if this works or not with pkcs11-helper. I guess that if you already have this for windows, it should be possible to have this for all platforms without a complete write. It should be as simple as opening a unix domain socket and if success send notifications into it, the implementation of the peer can be done at later time. |
|
This OpenSC bug looks invalid to me. You're allowed to fork() when you like, otherwise glib wouldn't be permitted to do it implicitly on a library call as it does. The POSIX atfork documentation clearly forbids calling any but a limited set of safe system calls from the handler. Yet nothing in PKCS#11 says that a The bug here is with pkcs11-helper violating the POSIX specification and common sense, which has been discussed at length. But doesn't it already have an option to turn that off? |
|
This change to pkcs11-tool together with #1621 should at least make it work (without leaking ressources): diff --git a/lib/pkcs11h-core.c b/lib/pkcs11h-core.c
index dd3133e..737dfe0 100644
--- a/lib/pkcs11h-core.c
+++ b/lib/pkcs11h-core.c
@@ -698,13 +698,6 @@ pkcs11h_addProvider (
provider_location
);
-#if defined(ENABLE_PKCS11H_THREADING)
- if ((rv = _pkcs11h_threading_mutexLock (&_g_pkcs11h_data->mutexes.global)) != CKR_OK) {
- goto cleanup;
- }
- mutex_locked = TRUE;
-#endif
-
if ((rv = _pkcs11h_mem_malloc ((void *)&provider, sizeof (struct _pkcs11h_provider_s))) != CKR_OK) {
goto cleanup;
}
@@ -742,6 +735,13 @@ pkcs11h_addProvider (
goto cleanup;
}
+#if defined(ENABLE_PKCS11H_THREADING)
+ if ((rv = _pkcs11h_threading_mutexLock (&_g_pkcs11h_data->mutexes.global)) != CKR_OK) {
+ goto cleanup;
+ }
+ mutex_locked = TRUE;
+#endif
+
#if defined(_WIN32)
gfl = (CK_C_GetFunctionList)GetProcAddress (
provider->handle,
@@ -825,6 +825,13 @@ pkcs11h_addProvider (
cleanup:
+#if defined(ENABLE_PKCS11H_THREADING)
+ if (mutex_locked) {
+ _pkcs11h_threading_mutexRelease (&_g_pkcs11h_data->mutexes.global);
+ mutex_locked = FALSE;
+ }
+#endif
+
if (provider != NULL) {
if (provider->handle != NULL) {
#if defined(_WIN32)
@@ -839,13 +846,6 @@ cleanup:
provider = NULL;
}
-#if defined(ENABLE_PKCS11H_THREADING)
- if (mutex_locked) {
- _pkcs11h_threading_mutexRelease (&_g_pkcs11h_data->mutexes.global);
- mutex_locked = FALSE;
- }
-#endif
-
#if defined(ENABLE_PKCS11H_SLOTEVENT)
_pkcs11h_slotevent_notify ();
#endif
|
|
@alonbl what do you think about the two comments above? Within the OpenSC Project, we should at least find a compromise that doesn't put the work on the user... |
|
On Mon, Mar 25, 2019 at 3:37 PM Frank Morgner ***@***.***> wrote:
@alonbl <https://github.com/alonbl> what do you think about the two
comments above? Within the OpenSC Project, we should at least find a
compromise that doesn't put the work on the user...
There is no work on the users, there is a proper design, I explained over
and over why a library must not fork, any library, especially security
libraries and especially to fork user interface within privileged process.
I guess the message is not communicated correctly.
The only valid argument is yours as you not have resources to fix the
opensc PKCS#11 implementation and expect all consumers to alter behavior
based on your assumptions.
Look, I am more than willing to give pkcs11-helper to anyone that is
interested in maintaining it, but this is a complete transfer as I cannot
be responsible for anything diverted from the standards, this means that I
will probably also stop maintaining the gnupg-pkcs11 and you left with
openvpn as the only dependency, and also in this case it is wrong for the
daemon to directly access PKCS#11 as everything should be done via the
management interface.
However, the issue triggered by pkcs11-helper is only a symthom of invalid
design and implementation of the opensc PKCS#11 provider, so even if you
brute-force it somehow, it will pop-up in other places.
|
|
Hmm, it's a pity that you simply ignore other objections as invalid and classify any attempt to gracefully resolve this as forcing. Anyway, reading the source code it seems like glib tries to forks to run @loskutov, before starting OpenVPN, please try running Afterwards, glib should use it's standard IPC (which should not involve forking). By the way, isn't openvpn running as root with setuid? If so, glib should normally not launch dbus (checked with |
|
On Tue, Mar 26, 2019 at 3:41 AM Frank Morgner ***@***.***> wrote:
By the way, isn't openvpn running as root with setuid? If so, glib should normally not launch dbus (checked with g_check_setuid())...
This exactly the issue... if the process is privileged, running user
interface in the context is problematic, this is why openvpn has the
management interface to delegate the interaction to a different
process, and by now should include the private key operations.
However, in the past we have added support of running openvpn as non
privileged process, I am not sure this[1] still up-to-date, but the
principle shold be the same.
[1] http://alonbl.shoutwiki.com/wiki/Gentoo/OpenVPN_Non_Root
|
|
glib should detect both, running as root and running with sudo. In both cases, glib will not fork. |
|
On Tue, Mar 26, 2019 at 11:53 AM Frank Morgner ***@***.***> wrote:
glib should detect both, running as root *and* running with sudo
<https://github.com/GNOME/glib/blob/82c3e92855b73ea49651387c7606d99064078a38/glib/gutils.c#L2617-L2622>.
In both cases, glib will *not fork*
<https://github.com/GNOME/glib/blob/82c3e92855b73ea49651387c7606d99064078a38/gio/gdbusaddress.c#L1082-L1088>
.
That's great!
|
|
Sure, but for some reason it's obviously not the case in @loskutov's setup. |
resolve a deadlock caused when provider fork in C_Initialize/C_Finalize. Thanks: Frank Morgner <frankmorgner@gmail.com> Bug: OpenSC/OpenSC#1507 Bug: OpenSC#16
Problem Description
Here's a stack backtrace:
I'm trying to use OpenVPN with PKCS#11 authentication and OpenSC as the provider. On the frame
#16sc_notify_initcalls some glib functions. As glib is unaware of the limitations that PKCS#11 spec puts on the code that's executed afterC_Initializeis called, it callsforkinternally. In this particular case, it results in pkcs11-helper hang (see also OpenSC/pkcs11-helper#16).Proposed Resolution
Probably the notifications mechanism should be redesigned to avoid external library calls inside
C_Initialize(no specific suggestions though).The text was updated successfully, but these errors were encountered: