5869 Need AES CMAC support in KCF+PKCS11

Review Request #39 - Created April 24, 2015 and discarded

Information
Gordon Ross
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.

Issues

  • 11
  • 2
  • 2
  • 15
Description From Last Updated
Is it really safe to not use the provided copy_block and such here? I feel like I must be missing ... Rich Lowe Rich Lowe
Same comment regarding copy_block. It looks like you're perhaps just making these functions not copy at all, in which case ... Rich Lowe Rich Lowe
Comment doesn't really make it clear how this differs from cbc_remainder_len Rich Lowe Rich Lowe
A comment here with this new line would be nice, and is this inserted mid-enum because of PKCS#11? Dan McDonald Dan McDonald
COMMENT! Dan McDonald Dan McDonald
What is this constant? Garrett D'Amore Garrett D'Amore
I don't like the CMAC being incorporated with CBC, to be honest. This is highly security sensitive code, and I ... Garrett D'Amore Garrett D'Amore
Just make a choice. Garrett D'Amore Garrett D'Amore
Prefer _NOTE() in the function body to ARGSUSED wild card. (As in _NOTE(ARGUNUSED(what)). Garrett D'Amore Garrett D'Amore
I think without the trailing slash is better. Garrett D'Amore Garrett D'Amore
Perhaps this bit can be moved into the two cases above, with a fallthrough? Garrett D'Amore Garrett D'Amore
Gordon Ross
Rich Lowe
Gordon Ross
Dan McDonald
Gordon Ross
Gordon Ross
Review request changed

Status: Discarded

Garrett D'Amore

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.

  1. That's the goal of the test suite

usr/src/common/crypto/modes/cbc.c (Diff revision 1)
 
 

What is this constant?

  1. part of the CMAC spec RFC 4493

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/cbc.c (Diff revision 1)
 
 

Just make a choice.

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)).

I think without the trailing slash is better.

Perhaps this bit can be moved into the two cases above, with a fallthrough?

Loading...