5869 Need AES CMAC support in KCF+PKCS11

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

gwr
illumos-gate
5869
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? danmcd danmcd
COMMENT! danmcd danmcd
What is this constant? gdamore gdamore
I don't like the CMAC being incorporated with CBC, to be honest. This is highly security sensitive code, and I ... gdamore gdamore
Just make a choice. gdamore gdamore
Is it really safe to not use the provided copy_block and such here? I feel like I must be missing ... richlowe richlowe
Prefer _NOTE() in the function body to ARGSUSED wild card. (As in _NOTE(ARGUNUSED(what)). gdamore gdamore
Same comment regarding copy_block. It looks like you're perhaps just making these functions not copy at all, in which case ... richlowe richlowe
Comment doesn't really make it clear how this differs from cbc_remainder_len richlowe richlowe
I think without the trailing slash is better. gdamore gdamore
Perhaps this bit can be moved into the two cases above, with a fallthrough? gdamore gdamore
gwr
richlowe
  1. 
      
  2. 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?

    1. (I'll just answer these here, I suppose)
      I moved it to the end just to be safe.

  3. 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?

    1. I think this is from the spec. (will let Matt B. confirm).
      Yes, a spec. reference might be nice...

    2. It's defined in RFC 4493 ("Constants: const_Zero is 0x00000000000000000000000000000000\nconst_Rb is 0x00000000000000000000000000000087")

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

    Presumably this is somethingy ou should resolve before you want to integrate

    1. Already resolved; I believe we came to the conclusion that It could technically be used for DES, etc, even if it isn't currently, so we may as well pass them in.

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

    Again, something to resolve before you want to integrate

    1. We decided to keep the bzero

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

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

  8. usr/src/common/crypto/modes/modes.h (Diff revision 1)
     
     

    Comment doesn't really make it clear how this differs from cbc_remainder_len

    1. cbc_remainder_len contains the number of bytes currently stored in cbc_remainder, while max_remain is the upper limit for these bytes. (for CBC it's 15, and for CMAC it's 16, as we want to store a full unencrypted block for cmac_final.)

  9. 
      
gwr
danmcd
  1. 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)?

    1. Just looking at CCM, it looks to be much more invasive to add CMAC as a special case; To me it seems more involved than "do CBC in place, then something different at the end". More along the lines of "surgically isolate the CBC-MAC step - with the changes I made to CBC - so that it can be run alone, making sure not to break other things in the process".

      Requested comments had already been added :)

  2. 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?

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

    COMMENT!

  4. 
      
gwr
gwr
Review request changed

Status: Discarded

gdamore
  1. 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

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

    What is this constant?

    1. part of the CMAC spec RFC 4493

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

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

    Just make a choice.

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

  6. I think without the trailing slash is better.

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

  8. 
      
Loading...