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.

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

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.

Same comment as above.

Yeah, not a bad idea.

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.

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.

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.

See earlier comments

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.

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.

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

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.

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)

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

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"

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

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.

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

Yes, that's totally fine.

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.

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.

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.

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

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.

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.

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.

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?

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.

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.

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.

Wouldn't hurt.  I'll add it.

Yes

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.

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

That makes sense to me, Matt.

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.

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.

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.