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, 11: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, 11: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, 4: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, 11: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, 11: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, 11: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, 4: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 20, 2017, 12:04 a.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, 11: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, 11: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, 4: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, 11: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, 4: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, 11: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, 11: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, 11: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, 4:24 a.m.

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

3. 

   Matt Barden July 20, 2017, 12:04 a.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, 11: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, 4: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, 11: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 20, 2017, 12:04 a.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, 11: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, 11: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 20, 2017, 12:04 a.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.

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, 11: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 20, 2017, 12:04 a.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?

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.