Bugs: |
|
---|
5869 Need AES CMAC support in KCF+PKCS11
Review Request #39 — Created April 24, 2015 and discarded
Information | |
---|---|
gwr | |
illumos-gate | |
5869 | |
Reviewers | |
general | |
5869 Need AES CMAC support in KCF+PKCS11
It turns out Matt could not easily update this one, so I had him open his own RB.
Please see: https://www.illumos.org/rb/r/40/Sorry if I've caused you reviewers to have to add your questions and comments again.
We have a test framework for these changes, and we'll have that out for review too before this integrates into illumos.
However, we'd really like review comments on the KCF and PKCS11 changes now.
- 11
- 0
- 2
- 2
- 15
Description | From | Last Updated |
---|---|---|
A comment here with this new line would be nice, and is this inserted mid-enum because of PKCS#11? |
|
|
COMMENT! |
|
|
What is this constant? |
|
|
I don't like the CMAC being incorporated with CBC, to be honest. This is highly security sensitive code, and I ... |
|
|
Just make a choice. |
|
|
Is it really safe to not use the provided copy_block and such here? I feel like I must be missing ... |
|
|
Prefer _NOTE() in the function body to ARGSUSED wild card. (As in _NOTE(ARGUNUSED(what)). |
|
|
Same comment regarding copy_block. It looks like you're perhaps just making these functions not copy at all, in which case ... |
|
|
Comment doesn't really make it clear how this differs from cbc_remainder_len |
|
|
I think without the trailing slash is better. |
|
|
Perhaps this bit can be moved into the two cases above, with a fallthrough? |
|
-
-
usr/src/common/crypto/aes/aes_impl.h (Diff revision 1) Is changing the value of any following item in this enumeration really safe and compatible?
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) I'm guessing the value of this is defined by the algorithm. Is there a reference or something, or a better name, or is this a name that would be well known to anyone familiar with AES-CMAC?
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) Presumably this is somethingy ou should resolve before you want to integrate
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) Again, something to resolve before you want to integrate
-
usr/src/common/crypto/modes/ccm.c (Diff revision 1) Is it really safe to not use the provided
copy_block
and such here? I feel like I must be missing something (but it's probably that I'm not familiar with the design of the framework). -
usr/src/common/crypto/modes/gcm.c (Diff revision 1) Same comment regarding
copy_block
.It looks like you're perhaps just making these functions not copy at all, in which case why take the parameters?
-
usr/src/common/crypto/modes/modes.h (Diff revision 1) Comment doesn't really make it clear how this differs from
cbc_remainder_len
Description: |
|
---|
-
Overall design question: Why is CMAC a special case of CBC, instead of a special case of CCM (where you don't encrypt and just take the residue)?
-
usr/src/common/crypto/aes/aes_impl.h (Diff revision 1) A comment here with this new line would be nice, and is this inserted mid-enum because of PKCS#11?
-
Description: |
|
---|
-
Generally looks pretty good, but I've not verified against PKCS#11 nor have I gone through the algorithm with a super-fine toothed comb.
As you're modifying existing CBC modes, I think its really very important to have a complete test suite here, that provides known answer tests and covers the various edge cases. I think you need to supply that for both the new CMAC mode plus the other AES modes that you've affected.
-
-
usr/src/common/crypto/modes/cbc.c (Diff revision 1) I don't like the CMAC being incorporated with CBC, to be honest. This is highly security sensitive code, and I think copy-paste-modify may actually be superior in this cae than inline modification, even though a lot of code is shared.
-
-
usr/src/common/crypto/modes/ccm.c (Diff revision 1) Prefer _NOTE() in the function body to ARGSUSED wild card. (As in _NOTE(ARGUNUSED(what)).
-
usr/src/lib/pkcs11/pkcs11_kernel/Makefile.com (Diff revision 1) I think without the trailing slash is better.
-
usr/src/lib/pkcs11/pkcs11_softtoken/common/softEncryptUtil.c (Diff revision 1) Perhaps this bit can be moved into the two cases above, with a fallthrough?