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.

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

Show all issues

Where does this value come from? A standard somewhere? If so, can we define that?
Also, macros are conventionall in CAPS.

1. 

   Jason King May 10, 2017, 8:44 p.m.

   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.

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

Show all issues

Why are we ignoring the return value here?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   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. 

   Robert Mustacchi May 16, 2017, 1:24 a.m.

   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.

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

Show all issues

Why is the return value being ignored here?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   Same comment as above.

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

Show all issues

Surely we should assert / VERIFY that, no? What prevents a caller from reusing an allocated context and calling init_ctx again?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   Yeah, not a bad idea.

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

Show all issues

Can we list which specification this is?

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

Show all issues

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)

Show all issues

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. 

   Jason King May 10, 2017, 8:44 p.m.

   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. 

   Robert Mustacchi May 16, 2017, 1:24 a.m.

   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. 

   Matt Barden July 19, 2017, 9:04 p.m.

   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.

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

Show all issues

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

Show all issues

Why is this return value being ignored?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   See earlier comments

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

Show all issues

Why is this return value being ignored?

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

Show all issues

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)

Show all issues

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)

Show all issues

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)

Show all issues

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. 

   Jason King May 10, 2017, 8:44 p.m.

   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. 

   Robert Mustacchi May 16, 2017, 1:24 a.m.

   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.

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

Show all issues

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. 

   Jason King May 10, 2017, 8:44 p.m.

   Same as today -- the caller.  This might be better addressed as part of a more general discussion on the kernel crypto API.

2. 

   Robert Mustacchi May 16, 2017, 1:24 a.m.

   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.

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

Show all issues

Erm, shouldn't this be unnecessary?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   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)

usr/src/lib/pkcs11/pkcs11_softtoken/common/softAESCrypt.c (Diff revision 1)

Show all issues

Any particular reason this was moved?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   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

usr/src/lib/pkcs11/pkcs11_softtoken/common/softAESCrypt.c (Diff revision 1)

Show all issues

Should the comment be updated for CMAC?

usr/src/lib/pkcs11/pkcs11_softtoken/common/softAESCrypt.c (Diff revision 1)

Show all issues

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. 

   Jason King May 10, 2017, 8:44 p.m.

   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. 

   Robert Mustacchi May 16, 2017, 1:24 a.m.

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

3. 

   Matt Barden July 19, 2017, 9:04 p.m.

   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.

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.

1. 

   Jason King May 10, 2017, 8:44 p.m.

   That's probably a much larger change to pkcs11_softtoken that I'd prefer be done as a separate change.

2. 

   Robert Mustacchi May 16, 2017, 1:24 a.m.

   Yes, that's totally fine.

usr/src/test/crypto-tests/tests/common/cryptotest_kcf.c (Diff revision 1)

Show all issues

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

1. 

   Jason King May 10, 2017, 8:44 p.m.

   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. 

   Matt Barden July 19, 2017, 9:04 p.m.

   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.

usr/src/test/crypto-tests/tests/common/cryptotest_kcf.c (Diff revision 1)

Show all issues

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)

Show all issues

Same note on cstyle for function signatures for this file.

usr/src/test/crypto-tests/tests/common/testfuncs.c (Diff revision 1)

Show all issues

Can we use snprintf please and keep track of buffer sizes?

usr/src/test/crypto-tests/tests/common/testfuncs.c (Diff revision 1)

Any reason this is split?

usr/src/test/crypto-tests/tests/modes/aes/cbc/aes_cbc.c (Diff revision 1)

Show all issues

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)

Show all issues

Same notes on style and void.

usr/src/test/crypto-tests/tests/modes/aes/cmac/aes_cmac.c (Diff revision 1)

Show all issues

Same notes on style and void.

usr/src/test/crypto-tests/tests/modes/aes/ctr/aes_ctr.c (Diff revision 1)

Show all issues

Same notes on style and void.

usr/src/test/crypto-tests/tests/modes/aes/ecb/aes_ecb.h (Diff revision 1)

Show all issues

Is this here, but commented out for a reason?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   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.

usr/src/test/crypto-tests/tests/modes/aes/ecb/aes_ecb.h (Diff revision 1)

Show all issues

Same question on commented out?

usr/src/test/crypto-tests/tests/modes/aes/ecb/aes_ecb.h (Diff revision 1)

Show all issues

Same question on commented out?

usr/src/uts/common/crypto/core/kcf_prov_lib.c (Diff revision 1)

Show all issues

Presumably this comment should be removed with this being removed?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   I've moved it to stay with the function that was moved.

usr/src/uts/common/crypto/io/crypto.c (Diff revision 1)

Show all issues

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. 

   Matt Barden July 19, 2017, 9:04 p.m.

   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. 

   Jason King Sept. 15, 2017, 5:49 p.m.

   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.

usr/src/uts/common/crypto/io/crypto.c (Diff revision 1)

Show all issues

We should make a decision and eliminate the XXX. What are the implications for callers based on the different options here?

1. 

   Jason King May 10, 2017, 8:44 p.m.

   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. 

   Matt Barden July 19, 2017, 9:04 p.m.

   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. 

   Jason King Sept. 15, 2017, 5:49 p.m.

   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.

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)

Show all issues

Opening '{' is usually on its own line.

usr/src/test/crypto-tests/tests/modes/aes/ccm/aes_ccm.c (Diff revisions 1 - 2)

Show all issues

Opening '{' is usually on its own line.

usr/src/test/crypto-tests/tests/modes/aes/cmac/aes_cmac.c (Diff revisions 1 - 2)

Show all issues

Opening '{' is usually on its own line.

usr/src/test/crypto-tests/tests/modes/aes/ctr/aes_ctr.c (Diff revisions 1 - 2)

Show all issues

Opening '{' is usually on its own line.

usr/src/test/crypto-tests/tests/modes/aes/ecb/aes_ecb.c (Diff revisions 1 - 2)

Show all issues

Opening '{' is usually on its own line.

usr/src/test/crypto-tests/tests/modes/aes/gcm/aes_gcm.c (Diff revisions 1 - 2)

Show all issues

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.

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)

Show all issues

Is it worth an ASSERT that mode is either CBC_MODE or CMAC_MODE?

1. 

   Jason King Sept. 6, 2017, 2:46 p.m.

   Wouldn't hurt.  I'll add it.

usr/src/common/crypto/modes/cbc.c (Diff revisions 4 - 5)

Show all issues

If it's only approved for those two sizes, should we explicitly check the two rather than the >= check?

1. 

   Jason King Sept. 6, 2017, 2:46 p.m.

   Yes

usr/src/common/crypto/modes/ctr.c (Diff revisions 4 - 5)

Show all issues

I presume it's okay to manipulate these on failure because the caller would need to destroy the context and start anew?

1. 

   Jason King Sept. 6, 2017, 2:46 p.m.

   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. 

   Matt Barden Sept. 7, 2017, 6:49 p.m.

   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. 

   Robert Mustacchi Sept. 15, 2017, 4:54 p.m.

   That makes sense to me, Matt.

Nits, and one compilation problem we already discussed.

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

Show all issues

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

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

Show all issues

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

1. 

   Matt Barden Sept. 7, 2017, 6:58 p.m.

   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.

usr/src/lib/pkcs11/pkcs11_softtoken/common/softSignUtil.c (Diff revision 6)

*pulSignatureLen is set by us, right?

1. 

   Jason King Sept. 7, 2017, 5:07 p.m.

   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.

usr/src/test/crypto-tests/tests/common/testfuncs.c (Diff revision 6)

Show all issues

memcmp() != 0, please.

usr/src/common/crypto/modes/cbc.c (Diff revisions 4 - 9)

Show all issues

Why not just return NULL? If this happened on non-debug that means we'd get something very weird.

1. 

   Jason King Sept. 15, 2017, 5:14 p.m.

   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.

Thanks for addressing my issues.

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

Show all issues

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)

Show all issues

Maybe this comment goes better with the XOR below since the next two lines are strictly concerning K2 from K1?

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)

Show all issues

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.

Thanks for fixing that. The updated assert looks right.

Ship It!