Description: |
|
---|
5869 Need AES CMAC support in KCF+PKCS11
Review Request #445 — Created April 25, 2017 and submitted
Information | |
---|---|
jbk | |
illumos-gate | |
5869 | |
Reviewers | |
general | |
mbarden |
5869 Need AES CMAC support in KCF+PKCS11. From Matt Barden matt.barden@nexenta.com
Ran test suite and was successful for all implemented modes (userland CCM and GCM modes are not yet implemented, so they failed with CKR_MECHANISM_INVALID as expected).
- 3
- 0
- 42
- 4
- 49
Description | From | Last Updated |
---|---|---|
I have not reviewed that this correctly implements the RFC algorithm. I'd appreciate getting someone else to do that. |
|
|
Hi, perhaps condense the duplication of the if-block and else-block's last three statements? Or perhaps to align explicitly with the ... |
|
|
Maybe this comment goes better with the XOR below since the next two lines are strictly concerning K2 from K1? |
|
Testing Done: |
|
---|
-
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) Where does this value come from? A standard somewhere? If so, can we define that?
Also, macros are conventionall in CAPS.
-
-
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) Surely we should assert / VERIFY that, no? What prevents a caller from reusing an allocated context and calling init_ctx again?
-
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) I have not reviewed that this correctly implements the RFC algorithm. I'd appreciate getting someone else to do that.
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) The comment for const_Rb mentions that this is only for 128 bit keys. What's guaranteeing that only 128-bit keys are being used from an API perspective? I ask because while RFC 4494 only mentions using AES-128, I'm not sure what we're doing that guarantees that this is the case in the system.
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) Should we use explicit_bzero in userland so the compiler doesn't optimize it away since it'll see it as a stack operation?
-
-
-
usr/src/common/crypto/modes/ctr.c (Diff revision 1) WHy is this cast to void? If there was an error, shouldn't we propagate it?
-
usr/src/common/crypto/modes/ecb.c (Diff revision 1) Why is this cast to void? If there was an error, shouldn't we propagate it?
-
usr/src/common/crypto/modes/gcm.c (Diff revision 1) Why is this cast to void? If there was an error, shouldn't we propagate it?
-
usr/src/common/crypto/modes/modes.c (Diff revision 1) I don't think length should be a signed value if we can avoid it. That way we have defined overflow semantics. Note this has knock on effects for crypto_put_output_data(), but it's not clear why it'd be a signed value to begin with. Is there a reason that a size of int was chosen versus size_t, etc.?
-
usr/src/common/crypto/modes/modes.c (Diff revision 1) What happens if offset + len overflow here? Or rather, what's responsible for making sure that before we do any writing out of this data, that we won't overflow any of the arithmetic due to a buggy offset or base value? I think this is more generally true of this function.
-
-
usr/src/lib/pkcs11/pkcs11_softtoken/common/softAESCrypt.c (Diff revision 1) Any particular reason this was moved?
-
usr/src/lib/pkcs11/pkcs11_softtoken/common/softAESCrypt.c (Diff revision 1) Should the comment be updated for CMAC?
-
usr/src/lib/pkcs11/pkcs11_softtoken/common/softAESCrypt.c (Diff revision 1) Capitalize?
Also, why don't we care about the validation? Is that a property of CMAC or some reason? The second part of this comment isn't very clear on the reason / explanation.
-
usr/src/lib/pkcs11/pkcs11_softtoken/common/softAESCrypt.c (Diff revision 1) We should change this to an error checking mutex and use mutex_enter/exit in the future. Since this isn't an error checking lock.
-
usr/src/test/crypto-tests/tests/common/cryptotest_kcf.c (Diff revision 1) cstyle for function signatures? This is true of this entire file.
-
usr/src/test/crypto-tests/tests/common/cryptotest_kcf.c (Diff revision 1) Don't we need to check the return value here? If we had an error, mech.rv will be uninitialized.
-
usr/src/test/crypto-tests/tests/common/cryptotest_pkcs.c (Diff revision 1) Same note on cstyle for function signatures for this file.
-
usr/src/test/crypto-tests/tests/common/testfuncs.c (Diff revision 1) Can we use snprintf please and keep track of buffer sizes?
-
-
usr/src/test/crypto-tests/tests/modes/aes/cbc/aes_cbc.c (Diff revision 1) Function signatures. If we don't want arguments, should be void, otherwise we can't distinguish between none and K&R.
-
usr/src/test/crypto-tests/tests/modes/aes/ccm/aes_ccm.c (Diff revision 1) Same notes on style and void.
-
usr/src/test/crypto-tests/tests/modes/aes/cmac/aes_cmac.c (Diff revision 1) Same notes on style and void.
-
usr/src/test/crypto-tests/tests/modes/aes/ctr/aes_ctr.c (Diff revision 1) Same notes on style and void.
-
usr/src/test/crypto-tests/tests/modes/aes/ecb/aes_ecb.h (Diff revision 1) Is this here, but commented out for a reason?
-
usr/src/test/crypto-tests/tests/modes/aes/ecb/aes_ecb.h (Diff revision 1) Same question on commented out?
-
usr/src/test/crypto-tests/tests/modes/aes/ecb/aes_ecb.h (Diff revision 1) Same question on commented out?
-
usr/src/uts/common/crypto/core/kcf_prov_lib.c (Diff revision 1) Presumably this comment should be removed with this being removed?
-
usr/src/uts/common/crypto/io/crypto.c (Diff revision 1) Seems like we need to get an answer to this XXX. What are the implications of getting a hold on software providers and otherwise?
-
usr/src/uts/common/crypto/io/crypto.c (Diff revision 1) We should make a decision and eliminate the XXX. What are the implications for callers based on the different options here?
-
Thanks for the changes here. There are some minor additional C style bits, but in general, I think this is looking good. From my perspective we still need to figure out one or two of the overflow issues and then have someone really scrutinize the actual CMAC implementation. If no one else comes along, I can try, though I don't have as much confidence in my analysis skills there.
-
usr/src/test/crypto-tests/tests/modes/aes/cbc/aes_cbc.c (Diff revisions 1 - 2) Opening '{' is usually on its own line.
-
usr/src/test/crypto-tests/tests/modes/aes/ccm/aes_ccm.c (Diff revisions 1 - 2) Opening '{' is usually on its own line.
-
usr/src/test/crypto-tests/tests/modes/aes/cmac/aes_cmac.c (Diff revisions 1 - 2) Opening '{' is usually on its own line.
-
usr/src/test/crypto-tests/tests/modes/aes/ctr/aes_ctr.c (Diff revisions 1 - 2) Opening '{' is usually on its own line.
-
usr/src/test/crypto-tests/tests/modes/aes/ecb/aes_ecb.c (Diff revisions 1 - 2) Opening '{' is usually on its own line.
-
usr/src/test/crypto-tests/tests/modes/aes/gcm/aes_gcm.c (Diff revisions 1 - 2) Opening '{' is usually on its own line.
-
I think this addresses most of the notes that I've had. I think we'll still want to make sure we have an in-depth review of the CMAC implementation itself.
Change Summary:
I realized I missed updating the comment in softAESCrypt.c, I've added that here
Diff: |
Revision 4 (+5399 -433) |
---|
Change Summary:
New update from Matt
-
Thanks for the follow ups and clarifications, Matt. I have a few minor questions and one thing I'm not 100% clear on is the relationship between the block size that we're doing the CMAC on and the AES key size. It seems like we're allowing any sized AES key, but restricting the block size is that right? From my rough reading I thought that the block size of the cipher had to match that of the desired cmac. So given that AES is a 128-bit block size, where does support for the 64-bit blocks come from?
-
usr/src/common/crypto/modes/cbc.c (Diff revisions 4 - 5) Is it worth an ASSERT that mode is either CBC_MODE or CMAC_MODE?
-
usr/src/common/crypto/modes/cbc.c (Diff revisions 4 - 5) If it's only approved for those two sizes, should we explicitly check the two rather than the >= check?
-
usr/src/common/crypto/modes/ctr.c (Diff revisions 4 - 5) I presume it's okay to manipulate these on failure because the caller would need to destroy the context and start anew?
People: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+5462 -337)
|
-
Nits, and one compilation problem we already discussed.
-
usr/src/common/crypto/modes/cbc.c (Diff revision 6) Needs sys/debug.h (pending build verification) or a rewrite.
-
usr/src/common/crypto/modes/cbc.c (Diff revision 6) WHY is max_remain different (and set to blocksize + 1)?
-
usr/src/lib/pkcs11/pkcs11_softtoken/common/softSignUtil.c (Diff revision 6) *pulSignatureLen is set by us, right?
-
Diff: |
Revision 7 (+5463 -337)
|
---|
Change Summary:
cstyle fixes
Diff: |
Revision 8 (+5463 -337)
|
---|
Change Summary:
Added comment about max_remain and block size.
Diff: |
Revision 9 (+5468 -337)
|
---|
-
-
usr/src/common/crypto/modes/cbc.c (Diff revisions 4 - 9) Why not just return NULL? If this happened on non-debug that means we'd get something very weird.
-
-
usr/src/common/crypto/modes/cbc.c (Diff revision 9) Hi, perhaps condense the duplication of the if-block and else-block's last three statements?
Or perhaps to align explicitly with the branching of RFC 4493 "Algorithm AES-CMAC" Step 4, only duplicate one XOR in producing a final M_last before continuing after the if-else for the last transformations of "Algorithm AES-CMAC" Step 6?
-
usr/src/common/crypto/modes/cbc.c (Diff revision 9) Maybe this comment goes better with the XOR below since the next two lines are strictly concerning K2 from K1?
Diff: |
Revision 10 (+5466 -338)
|
---|
-
I think there's a minor issue with the VERIFY, but otherwise it seems reasonable. Though again, I did not verify the correctness of the CMAC algorithm.
-
usr/src/common/crypto/modes/cbc.c (Diff revisions 4 - 10) I don't believe this is quite right. If mode & flags is zero gets xored with CBC_MODE|CMAC_MODE, then it'll result in a valid value.
Change Summary:
Updated verify check
Diff: |
Revision 11 (+5467 -338)
|
---|