Signature broken for HSM devices in the current master #178
Comments
These fixes should also address #178.
|
Please let us know whether the fix I implemented works for you. |
Better, but still not good. It doesn't quite work for HSM, and broke it for PIV. I see three big problems:
Also, the way it implements the fall-back seems broken. I don't see an obvious problem, but it does not work properly: keeps prompting for the PIN, and eventually fails... For the first problem I recommend For the 2nd one - solution I have in my fork works. Perhaps you could implement it. Tested for RSA-PKCS1 and RSA-PSS. Here's HSM SPY: And the same for PSS: This indicates that it still tried to re-authenticate. I know for sure that the keys on the HSM do not have the Also, RSA-OAEP decryption on the HSM fails with the symptom that looks like the case On a PIV token: OpenSSL manages to get the PIV token to sign, but I still consider this wrong. Prompting for the token key and then for the SIGN key is fine. But keeping prompting for the SIGN key...?! Here's the SPYL And this shows how PIV token that cannot do internal padding (supports raw RSA only) doesn't work any more: Crash details: All of the above work fine with my branch/fork. OpenSSL-1.1.0-stable with the proposed fix to (1): I can provide complete SPY for your master and for my fork, if needed - it's verbose but IMHO can be useful. Here are excerpts for my fork: And for HSM: |
|
@mouse07410 PKCS#11 2.40 Section 5.11: Also see 5.2 Conventions for functions returning output in a variable-length buffer SPY trace 97: C_Sign is a successful call (i.e.,one which returns CKR_OK) to determine the length of the buffer needed to hold the signature. The above says the session stays active. But the SPY Trace 98: C_SignInit tries to use the active session and returns CKR_OPERATION_ACTIVE I do not see a cancel operation function. It appears the calling application should have allocated the buffer then called C_Sign to continue the operation, not call C_SignInit to start over. I would suspect that the OpenSC PKCS#11 module may have allowed C_SignInit be called twice to in effect cancel the previous operation. But the Yubico module does not. So this is a gray area in the PKCS#11 documentation: No way to cancel an operation. An application or libp11 needs to address a sign operation with the buffer == NULL and somehow not restart the operation with C_SignInit , but go right to the C_Sign. |
@dengert in my code - yes I did, as you suggested. And I also kept all the necessary handling of But I don't see this flag set in the master. The master does not seem to create a new pmeth blob, at least explicitly... Also, I suspect that the streamlining that @mtrojnar did, affected how the repeated calls and fallback are done, which probably causes the failing attempts to re-initalize the session that at least this HSM (and PIV) refuse to do. I might be able to try this with CAC next week, but you seem to be right on the money here:
Exactly. And your proof-of-concept code that I polished (https://github.com/mouse07410/libp11/tree/rsa-oaep-prep) did exactly that, and works fine. |
|
@mouse07410 You are right: I indeed haven't started implementing OAEP yet. AFAIR in fact this feature wasn't even requested (or maybe it was requested somewhere deep within the PSS feature request)... |
|
Signature appears to be fixed for HSM but remains broken for PIV. And using OpenSSL with PIV, it prompts for the SIGN key twice, after it prompted for the token key. I think HSM support is good, but the fallback remains broken, which is why PIV doesn't work. On PIV, for PKCS1 signature it does sign, but the created signature fails verification: For PSS signature on PIV, it just fails and crashes: with the crash report: On the good side, here's the good logs from HSM:
Please consider the request submitted. ;-) P.S. OpenSSL-1.0.2-stable added the get methods for the |
|
Please consider opening a new issue, so that we can prioritize and track your request. |
Thanks, done: #180 |
UpdateWith this change it stops prompting for the PIN unnecessarily. Update 2After tweaking the test-code, and with the above change:
|
CKU_CONTEXT_SPECIFIC logins should not be performed after an unsuccessful initialization. This bug surfaced in #178.
|
@mouse07410 You are (mostly) right. In case I also fixed the other occurrences of this bug. |
|
@mtrojnar I agree, and thanks for fixing this in other places as well. Current status:
Also, is there a possibility that somewhere in your code the engine misses the opportunity to set |
|
@mouse07410 Could you please capture some PIV SPY traces with the current master branch? |
|
First, I do not observe crashes any more. RSA-PSS seems to work on PIV tokens now: RSA-PKCS1 still produces bad signatures: With OpenSSL |
|
And for comparison, this is the "good" output from my fork: |
|
Why doesn't it work with simple RSA PKCS1 padding, and only one card and not with other cards? Also, the signed value is different in failed and successful tests. Maybe it's something obvious and I can't see it simply because it's too late... Anyway, I have rewritten the code to make it fairly clear and it should be easy to spot the bug. |
|
In the bad one, you are signing just the hash: In the good one you are signing the digest: |
I have only PIV-compatible cards, and a couple of HSM devices. All I can say is that it looks like when the token supports only raw RSA, there's a problem (potentially related to how the fallback is handled, and what it is falling back on). When the token can do the padding internally, it works. @dengert so what should be signed is this ASN.1 structure, right? Note that both "good" and "bad" examples were created by OpenSSL with identical invocations... |
|
Right. pkcs1 padding signs the ASN1 digest which includes the hash. With PSS padding, it takes just the hash. (In opensc, the asn1 with the digest are hard coded, so they just concatenate them with the hash.) I would suggest your run the debugger, break on pkcs11_try_pkey_rsa_sign then step through it and look at the lengths and what EVP_MD_size(sig_md) You could also break on pkey_rsa_sign to see if it Or add some fprintf statements to see what is is doing. Not in the original "proof of concept" code, only PSS was handled. All other was handled by the orig_pkey_rsa_sign. There it does for pkcs1 padding. |
So my fork that essentially uses your "proof of concept" code, has the fallback to And the current master tries to do it all on its own, and fails to ASN.1-encode the hash with the algorithm (which depends on the actual hash used) before sending it for signing. To solve, one would create 5 "prefixes", one for each allowed hash (with accommodation for its length), right? Also, I recall you mentioning that engine may advertise a mechanism that is not supported (entirely) by hardware. I checked. Here's what I have for the NEO (PIV-compliant): And here's what HSM provides (note that pretty much everything except raw digests is in HW): @dengert One thing puzzles me here. Originally, the pkcs11 engine (libp11) was able to do Now we got an HSM that cannot do But when the engine tries to do the same with a PIV token (that also says the input for signing is not converted to the ASN.1 Digest structure, so even though it is signed it fails verification: My conjecture (based on your explanation) is:
What do you think the solution should be? Pass @mtrojnar did you change behavior of what the "proof of concept" code used as P.S. @mtrojnar you might like to know that your current master appears to do RSA-OAEP encryption on PIV tokens. Fallback there seems to work fine. |
|
At times in our discussion the terms "token", "on-board", "PKCS#11 module", "hardware" and "software". Different 'PKCS#11 modules" can present a different list of support mechanisms even when using the same "hardware token". In your list of mechanisms, I assume the PIV was using the opensc-pkcs11 module, and the HSM was using the Yubico PKCS#11 module. The CKF_HW flag can be misleading for a compound operation such as RSA-PKCS. opensc-pkcs11 reports: So although the CKF_HW is set, part of the operation (padding) must be done in software. And the PIV hardware can not do public key operations, so a verify must be all in software. The same holds for the SHA*-RSA-PKCS mechanisms, the SHA* part must be done in software, because the PIV hardware does not support it. and the PKCS must be done in software as well. and the CKF_HW flag is not set. I am not as familiar with the HSM as you are. But noting that it does not support RSA-X-509, but does support RSA-PKCS implies either (1) that the padding is done only in the hardware or (2) the PKCS#11 module is doing the padding in software and the card does support a RSA RAW operation but the module does not advertise it. (From conversations you had with Yubico, (1) is the case.) The HSM also says it supports SHA*-RSA-PKCS for example The point of all of this is: PKCS#11 modules implement a lot of software that the application could have done itself to make it easier on the application so it can just stick to making PKCS#11 calls. OpenSC added the digest mechanisms to make it easier. It looks like the Yubico PKCS#11 module added them too. Note that neither of these set the CKF_HW flag on these single operation mechanisms So my speculation is the Yubico HSM does not support the digest operations in hardware. If that is the case, there is little or no advantage in trying to have the software in the PKCS#11 module do the hashing vs having the caller do the hashing in software. As to some of your speculations: In other words the caller passes in whatever they want to be padded It does not have to be a digest. PKCS#11 2.40 2.1.14 PKCS #1 v1.5 RSA signature with MD2, MD5, SHA-1, SHA-256, SHA- Here it says it does comply with PKCS #1 v1.5 and computes the hash and digestInfo. @mtrojnar |
|
@dengert currently the code appears to send to the token DigestInfo for RSA-PKCS, and a raw hash for RSA-PSS. Do you think it should send DigestInfo in both cases?
I'm pretty certain that (1) is the case. They planned to have it certified, and cited security concerns when I asked to add raw RSA support in case somebody wants it.
I did not ask. If you think it's beneficial, I will ask them. But from our point of view, we're only dealing with the PKCS#11 interface module - so even if they (contrary to what I want to believe :) do part of the padding in their software, it won't affect what we need to pass to it for PSS signature. Correct?
I'm only trying to avoid the "Unimplemented digest" error when OpenSSL (misguidedly, IMHO) tries to offload all the operations to the engine. I'm currently working with OpenSSL team attempting to resolve this problem, but having "belt and suspenders" has always been my strategy. ;-) And I see no harm in having the engine advertise support for digests (and offloading them back to OpenSSL). Am I wrong here? |
This commit reverts attempts to use EVP_PKEY for more than just PSS.
|
Thank you. I agree with your assessment. It should be fixed now. |
|
@mouse07410 For PSS only the hash is sent, along with the parameters. PKCS#11 2.40 2.1.o PKCS #1 RSA PSS |
|
Using OpenSSL with openssl/openssl#4503 applied, and modifying your code with based on Steve Henson's newly added method
Thank you! P.S. Now - to RSA-OAEP encryption. ;-) Update |
|
@mouse07410 I will apply your fix. I'm glad that now you know that it is needed #176 (comment). 😆 |
Thank you!! 😉 More than that - I now know why, where, and how! 🤡 And I hope that this openssl/openssl#4503 (comment) would be merged soon by the OpenSSL team. Ironically, OpenSSL-1.0.2 can function correctly without changes, thanks to Update |
|
I think this issue is fixed. Will check on another machine this evening, and (hopefully ;) close it. |
|
Well, not quite. One small problem remained. Accessor methods Update #182 fixes it. |
|
#182 is merged |
The long term lock keeping was originally added in commit e81e335 "NULL sig support OpenSC#178" to support querying the size of the signature with sig=NULL. However, this commit was immediately followed up by 7a1fca4 "EVP_PKEY_FLAG_AUTOARGLEN for EVP_PKEY_meth_new()" which refers to same issue too. The EVP_PKEY_FLAG_AUTOARGLEN makes OpenSSL core handle sig=NULL case before calling the algorithm specific sign function. Thus we never get the sig=NULL call in the current code. Thus the original hack is unneeded. This effectively reverts e81e335 and adds an error handling if sig=NULL would happen.
The long term lock keeping was originally added in commit e81e335 "NULL sig support OpenSC#178" to support querying the size of the signature with sig=NULL. However, this commit was immediately followed up by 7a1fca4 "EVP_PKEY_FLAG_AUTOARGLEN for EVP_PKEY_meth_new()" which refers to same issue too. The EVP_PKEY_FLAG_AUTOARGLEN makes OpenSSL core handle sig=NULL case before calling the algorithm specific sign function. Thus we never get the sig=NULL call in the current code. Thus the original hack is unneeded. This effectively reverts e81e335 and adds an error handling if sig=NULL would happen.
The long term lock keeping was originally added in commit e81e335 "NULL sig support OpenSC#178" to support querying the size of the signature with sig=NULL. However, this commit was immediately followed up by 7a1fca4 "EVP_PKEY_FLAG_AUTOARGLEN for EVP_PKEY_meth_new()" which refers to same issue too. The EVP_PKEY_FLAG_AUTOARGLEN makes OpenSSL core handle sig=NULL case before calling the algorithm specific sign function. Thus we never get the sig=NULL call in the current code. Thus the original hack is unneeded. This effectively reverts e81e335 and adds an error handling if sig=NULL would happen.
The long term lock keeping was originally added in commit e81e335 "NULL sig support #178" to support querying the size of the signature with sig=NULL. However, this commit was immediately followed up by 7a1fca4 "EVP_PKEY_FLAG_AUTOARGLEN for EVP_PKEY_meth_new()" which refers to same issue too. The EVP_PKEY_FLAG_AUTOARGLEN makes OpenSSL core handle sig=NULL case before calling the algorithm specific sign function. Thus we never get the sig=NULL call in the current code. Thus the original hack is unneeded. This effectively reverts e81e335 and adds an error handling if sig=NULL would happen.
First, thank you for starting to add support for PSS and OAEP.
Problem:
Current master fails to perform RSA-PSS (and RSA-PKCS) signing on HSM:
Reason:
In 297fdda#diff-2590b49662dd77719cf90b1afe59cf81R173 you are forcing re-auth, which is rejected by the HSM device.
Solution:
Replace the offending line with something like
As we're at it, might add SHA224 and SHA-1 to
sig_mdhash just before 3a7750e#diff-2590b49662dd77719cf90b1afe59cf81R86, and SHA-1 just before 3a7750e#diff-2590b49662dd77719cf90b1afe59cf81R102.As a nit, it would be nice to display the
xxx_mdnot as numbers but as names, using something likeEVP_MD_name(md)instead ofEVP_MD_type(md). I'd much prefer seeingSHA384to673. ;-)Thanks!
The text was updated successfully, but these errors were encountered: