openssl: 3.0.12 reveals pkcs11 engine / SoftHSM cleanup issue causing a segfault on exit

The #23063 PR was merged, this should resolve this problem as a side effect. However the underlying issue is still there.

• We could mandate that C++ modules used with openssl not use global singletons (thereby avoiding the c++ runtime atexit registration, and hence the problem writ large. This would work, but is effectively a “not our problem” approach, and may not get lots of traction (see here)

I think this is the only reasonable approach, the fact SoftHSM uses atexit handlers in a loadable module is broken for any application using it directly, not just openssl.

The PKCS#11 API contract is that it is the application that controls the module shutdown via the finalizer function, using an atexit handler violates the contract.

An application that installs an exit handler to free its resources on exit will face the same segfault issue when using the PKCS#11 API directly as well.

So this specific segfault is effectively a SoftHSM issue and not an OpenSSL issue.

I would agree with that.

That said, given the fact that (1) softHSM has completely ignored the issue, despite several people raising it and (2) the fact that any other C++ library would be susceptible to this issues, that would seem to me to constrain our forward looking actions to:

1. Strongly recommending that users call OPENSSL_cleanup prior to exit (to avoid the issue)
2. Stating that any users who don’t do (1), and encounter a crash of this nature can only fix it by urging the offending c++ library to fix it (i.e. declare this a not-our-problem)

I don’t see why any of this needs to wait for 4.0. I think 4.0 could be where we force to use the libctx. We could deprecate the non-libctx APIs to encourage the new APIs.

@mattcaswell in most cases libraries should be mandated to use their own openssl ctx, or provide the application with a call to set the openssl ctx to use (or a way top tell the app they are done with the default context).

If libraries were to use their own context then they could simply free their own context without conflict with the main application.

Of course Openssl needs to be able to handle the case where some contexts are still out and about when OPENSSL_cleanup() is called, and defer some un-initialization to when the latest context is actually freed by the latest library.

@kroeckx I agree with that, and moving forward I feel like we should definitely mandate that OPENSSL_cleanup be called prior to exit, but I’m not sure we can mandate that for an already released version (but perhaps I’m wrong), that’s likely an OTC discussion

There are issues with mandating this.

We cannot mandate that libraries using OpenSSL should call OPENSSL_cleanup. If the application and a library it is using are both using OpenSSL then the library must not call OPENSSL_cleanup in case the application is still using it. We could recommend that applications should call OPENSSL_cleanup and libraries should not. But then we still have problems where an application does not use OpenSSL directly at all, but a library that the application depends on does.

I am suggesting softhsm is changed to be made out of 2 dsos, where the C wrapper loads the C++ one. This problem is not unique to libp11.

Any application using the pkcs11 API directly can trigger this same behavior (in fact I experienced it already early on with the pkcs11-provider and softhsm) if they decide to unload the pkcs11 module in an atexit() handler (which I think is perfectly fine if the application is in control of it).

It is a softhsm issue and should be directed to that project for resolution.

To expand on that, we should not be shutting down an engine/provider in atexit, that should have been done before that. There are basically 2 phases to the cleanup.

What is not clear to me currently is at what time and by who SoftHSM gets unloaded, and why p11 still has a reference to it’s data.

The problem is that, while the libp11 data that is referenced by the ENGINE_PILE is still perecty valid, the underlying data that libp11 references in the SoftHSM library is not valid (due to the fact that the C++ runtime has already deleted those objects)

Isn’t this a libp11 problem to solve?

@nhorman said:

given that the commit in question increases the refcount of the private key in the pkcs11 token by 1 (above what it would have been without the commit), so I would have, if anything expected to see leaks with the commit rather than without

and

@beldmit I might be mistaken, but for every keytype in that patch, you call {keytype}_get1, followed by EVP_set1. Each of those calls ups the refcnt of the keytype. You do call keytype_free, but that only releases one of the two counts.

I’m not seeing an increase in refcnt.

The pattern for each legacy key type is:

        RSA *rsa = EVP_PKEY_get1_RSA(pkey);
        EVP_PKEY_set1_RSA(pkey, rsa);
        RSA_free(rsa);

Lets assume the RSA object starts with a ref count of 1. After the EVP_PKEY_get1_RSA call it now has a recnt of 2. The EVP_PKEY_set1_RSA call does multiple things:

1. Up the ref count on the incoming RSA object. So the refcnt is now 3.
2. Frees any existing RSA object inside the EVP_PKEY (decrementing its refcnt). Since the existing RSA object is the same as the incoming RSA, this decrements the refcnt back to 2.
3. Replaces the old RSA object with the incoming RSA object (they are the same, so no change)

Finally RSA_free is called which decrements the refcnt to 1.

So we start with a refcnt of 1, and end with a refcnt of 1.

@rsbeckerca atexit should guarantee that registered function order is exactly the reverse of the registration order (ie… calling atexit(a), atexit(b), atexit© implies that when exit is called, the registered functions should be called explicitly in c, b, a order.

That said, I’m struggling to see how this can ever work. I say that because OPENSSL_init_crypto should always be called prior to any dynamic engine being loaded (at least, I think it should). Given the OPENSSL_init_crypto is where the OPENSSL_cleanup handler is atexit registered, we should be guararnted that it will run after any subsequent atexit calls are made.

Assuming that softhsm registers an atexit handler (be it via explicit call, which I can’t find), or via some wrapper magic in the c++ runtime, we should be guaranteed that the softhsm atexit handler will run prior to the openssl atexit handler. The implication here being that we should be guaranteed that the softhsm singleton object gets freed prior to the pcks11 library shutting down, causing the error the use after free error we are seeing.

I’m sure I’m missing something, but right now this seems very broken.

There are 2 issues here.

Issue 1, softhsm should really , really , really, use their own libctx,a nd stop stomping on the default context that should be reserved for the main application.

Issue 2 atexit() handler, which is often called before the engine tries to close all sessions, one could argue that openssl is wrong in installing an atexit() handler w/o being directed by the application which is the only entity that has a chance of knowin in which order atexit() handlers should be installed (as it influences the order in which they are run.

But really softhsm should handle this with fixing issue 1, then it can use its own context and not get messed up when it is requested to close sessions after openssl already destroyed the default library context.

Note that softhsm is not the only pkcs11 module that has these issues, so much so I had to add this PR to the pkcs11-provider which experienced the same problems as the engine early on: https://github.com/latchset/pkcs11-provider/pull/225

I’ll try to look at it.