HMAC cleanup fixes - #135
Conversation
The HMAC algorithm involves multiple calls to the PKCS#11 API. If it fails half-way through, a session can be leaked. This change ensures that the call to 'cleanup' is always made, and also ensures that this call is safe to call twice by adding a guard to check if the session ID has been cleared, signalling that the session has already been returned once to the pool. Without this test, a session might be returned twice (which could panic as the pool is oversize, or would corrupt the pool by adding the same session handle twice). The guard is needed rather than using Put(hi.session) as a nil would create a new session and increase the size of the pool, leaking the old session.
SoftHSM (now?) supports the HMAC mechanisms, so the unconditional skip is unnecessary. The hash-independent subtests (Empty/MultiSum/Reset) move to the plain SHA256 case so they run on SoftHSM; the _GENERAL cases have no SoftHSM equivalent and skip themselves via skipIfMechUnsupported. MultiSum now exercises the post-Sum errHmacClosed guard against a real token.
GenerateSecretKeyWithAttributes tries vendor-specific CKM_NC_*_HMAC_KEY_GEN before the generic-secret fallback, but only advanced to the next mechanism on CKR_TEMPLATE_INCONSISTENT or CKR_ATTRIBUTE_VALUE_INVALID. SoftHSM rejects the unknown nShield mechanism with CKR_MECHANISM_INVALID, so HMAC key generation failed outright there rather than falling back. Add that code to the fall-through set. Also fix the operator precedence: `ok && A || B` parsed as `(ok && A) || B`, so a nil-or-wrong-type error could still match the second branch.
|
Hi @is-alnilam 😃 , TY 🙏 for your interest and this PR. In 2026 the big project for
The other big project for 2026 is giving + moving the crypto11 project under the Eclipse Foundation. This is an opportunity for us to fix bugs like yours before releasing the new version ! On our current branch pkcs11-v3.2-ml-kem, we implemented a revision of you fix at 02c2ba8 with the help of LLM code assistant. You are very welcome to try one of our crypto11@v2 release candidate like for example crypto11@v2.0.0-rc3. Tell us if this fixes your issue. Your PR is mentioned in the commit 02c2ba8 and in Line 45 in 2b4b06e We are considering adding a file |
2 participants
Proposed Changes
While reviewing HMAC error paths, I started looking into the way that errors are returned from
Sum(). This can only panic, due to the Go APIs. I was looking into how best to recover from that when I spotted a leak of the session handle. As HMAC is a multi-step process, errors can occur at points where the existingcleanupfunction isn't called—this change closes those gaps.This change does introduce the possibility that
cleanupwill be called twice, which would cause the session to be returned to the pool twice. This is guarded against by adding a test forhi.session == nilat the start ofcleanup.When testing, I also spotted that the tests didn't run on SoftHSM, as there's a comment that SoftHSM doesn't support HMAC. I'm not sure if this relates to an older version of SoftHSM? Current versions do support HMAC (I'm using it in production), so I removed this guard.
That then exposed a bug in
symmetric.gowhere the fallback from a specific key type to a generic key wasn't being allowed as the particular return type didn't match the code's expectations—it turns out that I've been working around this at the application layer without realising what the issue was. I've checked the return values given by SoftHSM and the Utimaco HSM simulator and have updated the check to work with both of those, as well as the previous nShield and CloudHSM values. (My workaround was to copy theCipherHMACSHA384definition into my code, but to remove the first entry in theGenParamslist, leaving only the generic secret option.)Types of Changes
Bugfix
Verification
New and updated test cases cover the majority of the functionality. For the 'mid process' error case, though, this is difficult to test and will probably have to be left as just reviewed.
Testing
I've updated the HMAC tests to cover cases where my changes may have changed behaviour (or could trigger problems). I've not been able to test the specific leak, as to do wo would need a way to inject faults into the HSM session and I'm not sure how/if that can be done.
I've also updated the general HMAC tests (as mentioned above) to allow testing from SoftHSM. As well as enabling the tests and fixing the bug on mechanism fallback, I've also moved the 'full' test flag from the truncated 'general' HMAC (which isn't supported on SoftHSM) onto the full SHA256 HMAC, which should be supported everywhere.
Tests have been run against SoftHSM and Utimaco HSM simulator—I may be able to test against CloudHSM in a few days.
Linked Issues
User-Facing Change
Further Comments
The precise trigger for the errors investigation was CloudHSM—an HSM in the cluster can be removed at any time, creating a need to retry any operations which were in progress at the time. Retrying signing was easy enough, but HMAC is more difficult due to the Go APIs and the lack of an error return (just the panic). This then led to digging further and spotting the potential for a leak.