5869 Need AES CMAC support in KCF+PKCS11

Review Request #445 — Created April 25, 2017 and submitted


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. rm rm
Hi, perhaps condense the duplication of the if-block and else-block's last three statements? Or perhaps to align explicitly with the ... seeemef@mac.com seeemef@mac.com
Maybe this comment goes better with the XOR below since the next two lines are strictly concerning K2 from K1? seeemef@mac.com seeemef@mac.com
  1. Thanks for putting this together. The test suite is useful and important.

    I didn't go through and verify that the CMAC algorithms were correct. We should get someone else to go through and do that. There are some XXXs and the like that we should make sure are cleaned up before this goes back.

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

    1. I believe it comes from RFC 4493. The capitalization matches what is used there (but can obviously be updated). I'll add the RFC to the comment and UC it.

  3. usr/src/common/crypto/modes/cbc.c (Diff revision 1)
    Why are we ignoring the return value here?
    1. This is probably better as a VERIFY or VERIFY3S -- the only way crypto_put_output_data can fail is if the output buffer is of an unsupported type (which strongly implies a missing piece in any implementation of a new type).

    2. I think we need to do something here and all the others. I get that it's currently factored such that we should expect it to never fail, but what you don't want is refactoring to occur in crypto_put_output_data which then misses these cases and then we're silently ignoring return values. I think it'd be really good to see something here and for all of them.

  4. usr/src/common/crypto/modes/cbc.c (Diff revision 1)
    Why is the return value being ignored here?
    1. Same comment as above.

  5. 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?
    1. Yeah, not a bad idea.

  6. usr/src/common/crypto/modes/cbc.c (Diff revision 1)
    Can we list which specification this is?
  7. 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.
  8. 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.
    1. This seems to be vagueness in the RFC. A short examination of other implementations all appear to use the same constant with larger AES block sizes. We could optionally confine CMAC to 128-bit input blocks in the short term if there are compatability/interop concerns.

    2. I'd suggest that we start with confining to 128-bit unless we know of an analysis for the other key sizes and understand what the impact of that would be.

    3. After reading the nist spec for CMAC (at http://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38b.pdf section 5.3), I believe the RFC is slightly misleading in its description of const_Rb. the value "Rb" is determined by the block size, not the key size (the word "key" likely refers to the "subkey", which is xor'd with the final plaintext block); const_Rb is R128.

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

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

    Why is this return value being ignored?

    1. See earlier comments

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

    Why is this return value being ignored?

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

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

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

  15. 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.?
    1. That was part of the existing kernel crypto api implementation (the function was just moved to this file so it could be shared between kernel & userland), so I really don't know.

    2. OK, fair enough. If it's an int due to existing reasons, so be it. We can tackle the question of what should it potentially be later.

  16. 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.
    1. Same as today -- the caller. This might be better addressed as part of a more general discussion on the kernel crypto API.

    2. OK, well, we should make sure that this is being done somewhere then. In general we probably want that overflow logic in the lowest level. Particularly given that the types aren't changing at this instant, then we need to be extra careful to avoid overflow.

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

    Erm, shouldn't this be unnecessary?

    1. Actually it is required -- the CRYPTO_DATA_RAW case breaks out of the switch statement, though it'd probably be just as well to move it within that case block (though it may be that one of the compilers is complaining about lack of a return)

  18. Any particular reason this was moved?
    1. Just convention as far as I can tell -- most code lists the system (i.e. #include <filea.h>) includes before the local (i.e. #include "fileb.h") include

  19. Should the comment be updated for CMAC?
  20. 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.

    1. Would the following rewording be clearer? "CMAC does its own padding the input data when its *_final function is called, so avoid any intermediate attempts to pad"

    2. If that's what it's actually doing, then yes, this woudl be clearer.

    3. That's not quite right. the following code does two things: 1. store any remaining bytes for later use. 2. validate the length of input and output buffers, and copy some data to the output buffer.
      AES CMAC behaves differently than others for the former, as it needs to store up to a full block of data for use during cmac_mode_final, and it doesn't output any data until cmac_mode_final. Letting the underlying code handle this simplifies the implementation greatly.

  21. 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.
    1. That's probably a much larger change to pkcs11_softtoken that I'd prefer be done as a separate change.

    2. Yes, that's totally fine.

  22. cstyle for function signatures? This is true of this entire file.

    1. Wow.. for some reason git-pbchk is only flagging the copyright years -- I just reran to verify my sanity, and I didn't see them anywhere in the exception list. I'll get all of these cleaned up.

    2. I have no idea why git-pbchk refuses to flag these errors, and so I missed them as well. Strangely, it will catch other errors - if you, for example, have an improper block comment, it will flag that.

  23. Don't we need to check the return value here? If we had an error, mech.rv will be uninitialized.

  24. Same note on cstyle for function signatures for this file.
  25. Can we use snprintf please and keep track of buffer sizes?
  26. Any reason this is split?
  27. Function signatures. If we don't want arguments, should be void, otherwise we can't distinguish between none and K&R.

  28. Same notes on style and void.
  29. Same notes on style and void.

  30. Same notes on style and void.

  31. Is this here, but commented out for a reason?

    1. Appears to be the same data as ECB_DATA -- my guess is while copying the test data from the NIST standard, it wasn't realized until afterwards they were the same (and the commenting out was to validate it). I'll remove both.

  32. Same question on commented out?

  33. Same question on commented out?

  34. Presumably this comment should be removed with this being removed?
    1. I've moved it to stay with the function that was moved.

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

    1. I don't know what the implications are, but the "alternative" way that PKCS uses to gather the list of mechanisms specifically returns only hardware providers; this was an attempt at sticking to that intent, but I don't know if there are really any issues.

    2. I would think the biggest implications is that one could (at the cost of increased latency) make the crypto operations happen in kernel-land instead of userland -- so force the kernel to consume additional CPU and memory to perform the crypto operations. There isn't a good reason I can think of (at least currently) to allow userland access to the kernel software crypto implementations -- the kernel software provider and pkcs11_softtoken both use the code in usr/src/common/crypto for all the crypto operations.

  36. 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?
    1. Agreed. Doing a bit more digging (though perhaps someone that has more historical insight could offer better information), it appears that the kernel crypto API was never documented. I don't know though if this is one of those things where we might be concerned about stuff using it regardless and thus potential breakage. We can certaintly decide on a policy, though it might be nice to understand what the original intentions were.

    2. I don't know if there's any "traditional" response to receiving either of these errors. This was meant more as an aesthetic question - is this an invalid mechanism, or is it a bad argument?

    3. Since it looks like the code here is trying to mirror what PKCS#11 does, the general answer would be to follow what PKCS#11 does. However, I don't really see anything equivalent to this function in PKCS#11. The C_GetMechanismInfo function does specify CKR_MECHANISM_INVALID (the analogous return value to CRYPTO_MECHANISM_INVALID) as a possible return value. It's probably the closest analogy that I can think of, so I think CRYPTO_MECHANISM_INVALID would be ok here.

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

  2. Opening '{' is usually on its own line.
  3. Opening '{' is usually on its own line.
  4. Opening '{' is usually on its own line.
  5. Opening '{' is usually on its own line.

  6. Opening '{' is usually on its own line.

  7. Opening '{' is usually on its own line.

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

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

    1. The CMAC standard (NIST 800-38B) defines the mode for both AES and 3DES (or TDEA is NIST likes to call it). The phrasing of the standard (section 5.3) implies the constants are tied to the block length vs the particular algorithm in use. It happens to work out that AES uses the 128-bit value while 3DES uses the 64-bit value.

    2. Apologies for the late reply, I didn't know you'd followed up until Jason pushed his new code. Jason is correct; NIST has approved CMAC for use with both AES and DES, and the block size of the algorithm directly affects which constant is used, as the constants are used to "pre-process" the final block of the message before encrypting. Because the CMAC code is technically not exclusive to AES, I decided that removing AES-specific knowledge from it and introducing the ability to use DES (should someone choose to implement it at the necessary higher levels) would be appropriate.

      Note that while the spec (and parts of the code) refer to "Sub-keys" w.r.t CMAC (which understandably leads to confusion), these are not AES sub-keys (and bear no relation to them), but simply the term used to refer to the "pre-processing" done to the final message block.

  2. 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?
    1. Wouldn't hurt. I'll add it.

  3. 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?
  4. 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?
    1. I believe so -- there's no way I'm aware of to do any sort of recovery on failure, so the only option would be to start over.

    2. even if that's the case, for debugging purposes we probably shouldn't modify these on error. (I seem to recall the existence of recoverable errors, but I don't believe anyone actually tries.)

    3. That makes sense to me, Matt.

  1. Nits, and one compilation problem we already discussed.

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

    Needs sys/debug.h (pending build verification) or a rewrite.

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

    WHY is max_remain different (and set to blocksize + 1)?

    1. It's the difference in how we need to handle CMAC vs CBC. with CBC, as soon as we have a full block of data, we encrypt it; with CMAC, because the very last message block (and only this block) needs to be pre-processed before we encrypt it, and we only know we've hit the end of the message once final() is called, we need to store up to an entire block of plaintext data, and only encrypt once we see the beginnings of a new block. This means that CBC will only ever store block_size - 1 bytes of plaintext in cbc_remainder, and CMAC will store block_size bytes. This is what max_remain refers to: The maximum amount of data we can store before we'll encrypt.

  4. *pulSignatureLen is set by us, right?

    1. Yes and no. PKCS#11 allows you to query the size required by passing in a NULL pointer for pSignature. It will set the length required and return CKR_OK. If pSignature is non-NULL soft_aes_sign_verify_common will validate the length and if it is not large enough, it will set *pulSignatureLen to the size required and return CKR_BUFFER_TOO_SMALL. So the memcpy should only happen if pSignature is not NULL and *pulSignatureLen is sufficiently large to hold the output.

  5. memcmp() != 0, please.

  2. 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.
    1. We could change it to a VERIFY if that'd be better -- this is a function private to cbc.c -- the exposed interfaces(cbc_init_ctx() and cmac_init_ctx() are the only consumers and explicitly pass in the value of mode (as opposed to setting based on passed in parameters). Any invalid value of mode here would be a programming rather than user error.

  1. Thanks for addressing my issues.

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

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

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

  2. 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.
  1. Thanks for fixing that. The updated assert looks right.

  1. Ship It!
Review request changed

Status: Closed (submitted)