Permit 0-length inputs to Sign(Update), etc #82
Conversation
See miekg/pkcs11#82 for the fix we need. re #7
|
Why do we need this? Thought most of this convoluted stuff was needed for Windows support. The cypher -> cipher is worth a separate PR btw. |
|
I've made a separate PR for the spelling stuff. I can rebase this one onto that when it's landed, if you like. Why we need it (or at least, why I want it): 0-length inputs are an edge case but they do arise from time to time. The example I ran into was a test case for https://github.com/thalesignite/crypto11/ but particularly for hashes they do turn up in production too. Currently they panic this API, which is not very friendly l-) I've added another commit with some symmetric testing (including this case). It turns out SoftHSMv2 doesn't behave very nicely in some of these cases (though it does cope with e.g. hashing), so I guess it's an edge case that people often forget to consider. |
Now done. |
|
I still believe this PR destroys the ability to run properly on windows - which I don't have or use, so I can say for sure (we also don't have tests for it, yeah.). But a safer way is to check for nil slices and then error out, I think. |
|
It seems to work fine on Windows but I didn't do an exhaustive test. The Windows issues come into play with structures, not byte arrays. I think this would be tidier if Just my opinion though. Also the |
&data[0] panics for zero-length arrays.
Agreed. |
|
I'll look into the |
|
2.3.0 on Fedora 28. Amusingly, it seems to be crashing for the exact same reason, trying to do |
|
That's confusing. Do you have a backtrace? |
As far as I can tell it's still the case with the current codebase: https://github.com/opendnssec/SoftHSMv2/blob/develop/src/lib/SoftHSM.cpp#L2427 |
|
Oh, I see what you mean. I think I'll disable the test here, rather than try to debug softhsm. |
Signed-off-by: Richard Kettlewell <Richard.Kettlewell@thalesesecurity.com>
|
Is anything else needed before this can be merged? |
|
[ Quoting <notifications@github.com> in "Re: [miekg/pkcs11] Permit 0-length ..." ]
Is anything else needed before this can be merged?
probably not, but I haven't made any time to re-review this.
|
|
that failure @mtharp mentioned is that serious or "are you holding it wrong"? otherwise lgtm |
|
@miekg AIUI it is an issue in SoftHSM. |
We depend upon miekg/pkcs11#82.
We depend upon miekg/pkcs11#82.
We depend upon miekg/pkcs11#82.
* Implement cipher.Block for AES and DES3 re #6 * Fast CBC support re #6 * Exercise GCM in tests re #6 * HSM-native GCM For testing with SoftHSM2 you need at least version 2.4.0, i.e. at least Debian buster/sid or Ubuntu cosmic (or BYO). This commit also updates our dependency on github.com/miekg/pkcs11 to one with GCM support. re #6 * HMAC implementation re #7 * Finalized symmetric crypto interface You can now have a crypto11.BlockModeCloser, and must call Close(), or a cipher.BlockMode, but it has a finalizer. re #6 * Expose CBC via cipher.AEAD This is rather an abuse of the cipher.AEAD interface as the name and description both indicate it provides authenticated encryption, which is not the case for CBC. The risk of using it in a context where authentication is required is mitigated only by documentation. re #6 * Linter-driven cleanup * Split symmetric support into separate files re #6 re #7 * Documentation review re #6 * Keep blockModeCloser alive during PKCS#11 calls re #6 * Implement HMAC Reset() and make Sum() friendlier re #7 * HMAC empty inputs without panicing re #7 * update Gopkg.lock We depend upon miekg/pkcs11#82. * Query GCM capability rather than provider
&data[0]panics for zero-length arrays.I considered passing a null pointer (and length word of 0) for empty arrays. I chose not to do this because many standard C functions (e.g.
memcpy) forbid this, and they may be used internally by the PKCS#11 implementation.