Add CBC-MAC authentication code

[sef]

Summary

This is software-only, and not useful in and of itself -- it needs to be hooked up to the rest of the crypto code. (Side note: I see that I kept the CRYPTO_AES_CCM_16 define in here; I can remove it & renumber, but I mostly just extracted the relevant diffs from D18520.)

This has been tested by building the kernel only, since nothing else really refers to the code yet.

[sef]

sys/opencrypto/cbc_mac.c

121

See above

129

The function definition was too long. The comments are formatted because of the listed items.

148–149

156–157

The code does the copy first. The check is done afterwards; that's just how I wrote the code. It'd have to be refactored to do the copy afterwards.

I agree it should never be greater than; I used >= out of personal habit.

166

Easily enough done. It wasn't clear to me when it was and wasn't used.

179–183

How so?

199

See above.

[sef]

Feedback from cem. This won't be the final version.

[sef]

Where are similar ones for AES-GCM that I can copy? :)

https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/mac/gcmtestvectors.zip

No, I meant, where are they in the src tree?

[cem]

Thanks!

In D18592#397876, @sef wrote:

No, I meant, where are [AES-GCM KAT test code] in the src tree?

I don't know there are any you can directly copy. These two I know of might be useful for inspiration:

• There is some correctness comparison against OpenSSL's implementation in tools/tools/crypto/cryptocheck.c, but that's not suitable for this revision (cryptodev interface) nor is it KAT vectors. It looks like OpenSSL documents a CCM Mode, but I don't know if we enable/build it or not.
• There is also tests/sys/opencrypto/cryptotest.py, which does test KAT vectors. It's also unsuitable for this revision (cryptodev) and I can't endorse anything about the way it is implemented, really.

sys/opencrypto/cbc_mac.c

22–26

Sure; now we have two copies of questionable code in the tree instead of just the one ;-). Please go ahead and fix it.

34

I wasn't complaining about the name of the constant; just the useless struct member, and separation between assignment and use.

The aes_cbc_mac_ctx interface is internal, not KPI, so I don't see any reason to include the tagLength for now rather than just removing the member and using the constant.

If it ever becomes necessary to support variable-length tags, it is trivial to find the places the constant is used.

40

Do you intend to commit that version or this one? :-)

41

The void 5 lines higher should tell you the function is void, and lack of return in a non-void function triggers a compiler error in the kernel build. So in addition to the explicit function type attached to any definition, it should be clear that any time a function has no explicit return, it is void.

Regardless, style(9) is pretty clear that return should be omitted for void functions, and provides 2-3 examples of that.

62–63

The previous condition allowed one to be zero, and this one doesn't permit either to be zero. I believe this function is valid with zero authData, for example, so maybe the assertion should replace && with || (De Morgan's from the previous expression)?

79

Nothing about the algorithm says L has to be a single octet. You've chosen to store it in a uint8_t, which is unobjectionable because of the algorithm's constraints on its range, but masking it down to a single octet is totally unacceptable. It implies that the value might be out of range, and that that would not only be possible, but accepted.

Please assert on nonceLen being in the spec range, and then it is impossible for L to be out of range. [2, 8] is well within [0, 255] and the confusing and incorrect logical AND can be removed. You could additionally assert on L being in range after the subtraction, but I suspect an optimizing compiler would just remove it due to the range implied by the earlier nonceLen assertion.

82

Thanks! I don't think we can rely on bitfields due to underspecification of their layout in standard C (not that some in-tree drivers don't already make assumptions), but the shifts are clear and the prevailing method.

129

Doh! Sorry. I did not notice the list.

148–150

Still assuming that any given Update() call will only provide AD or payload; not both. That assumption should be KASSERTed or fixed.

158

==?

(compiler should have thrown a warning and probably error if this was attempted to compile?)

166

explicit_bzero is only used when memory with sensitive data is released and might be reused by another context. So it is typically invoked to clean up state on Final(). It isn't needed when memory ownership does not change.

(It's only needed because the C compiler may recognize that the released memory is not used after a regular bzero and "optimize" out the bzero. When bzero is used during an algorithm that later references the zeroed memory, the compiler is not free to optimize out the zeroing.)

179–183

E.g.,

AES_CBC_MAC_Update("a", length=1);
AES_CBC_MAC_Update("b", length=1);
AES_CBC_MAC_Update("c", length=1);
AES_CBC_MAC_Final();

Will incorrectly produce the MAC for the message "a\0\0…b\0\0…c\0\0", rather than for the message "abc".

[sef]

sys/opencrypto/cbc_mac.c

22–26

I just compared clang's code output for the code using uint64_t and uint32_t, for i386 and x86_64. The i386 code for both functions was pretty similar (same number of instructions, just slightly different ordering), while the x86_64 code using uint32_t was considerably worse than for uint64_t.

I haven't got easy access to the other platforms, but this does not appear to be performance fear grounded in reality.

40

If I commit both at the same time, would that confuse quantum encryption? :)

41

It's a pretty stupid rule -- there is absolutely no advantage to it, it harms readability, results in inherent inconsistency in style, and it ignores the growing number of stupidly-designed languages that set function return value to the last executed expression -- that is only mentioned in a comment when describing variardic functions. But easily changed.

62–63

I have gotten assert conditions backwards for over 30 years. This is another instance.

sys/opencrypto/cbc_mac.h

34–35

Ah, missed from my rename. Easily changed.

[sef]

This doesn't have all of cem's comments handled, so there'll be more; this is to get a snapshot in place before I start doing some network changes at home.

[cem]

Thanks, interdiff changes from the last version look good to me; some outstanding requests-for-changes remain.

sys/opencrypto/cbc_mac.c

22–26

It looks like GCC -O2 -funroll-loops (or -fpeel-loops) handles this well at any copy size:

https://reviews.freebsd.org/P241

But Clang handles it poorly without the hand-unrolling (1 byte -> 8 bytes at a time), and GCC doesn't make the optimization at -O2 without -funroll-loops or -fpeel-loops (and just keeps the small code size loop).

I'm okay with leaving it as-is.

sys/opencrypto/cbc_mac.h

55

aes_key isn't used either :-)

[sef]

In D18592#398160, @cem wrote:

I don't know there are any you can directly copy. These two I know of might be useful for inspiration:

Then I don't think it's something I can really do.

• There is some correctness comparison against OpenSSL's implementation in tools/tools/crypto/cryptocheck.c, but that's not suitable for this revision (cryptodev interface) nor is it KAT vectors. It looks like OpenSSL documents a CCM Mode, but I don't know if we enable/build it or not.

I *do* have cryptocheck changes later.

[sef]

sys/opencrypto/cbc_mac.c

158

Not sure what you mean by that.

179–183

Obviously I hadn't thought about it being called that way. The only one I'd thought about was the auth data not being a block multiple. That's an annoying rewrite of the code -- _Final will have to check for data left over as well.

sys/opencrypto/cryptodev.h

206

I noted in the initial message for this change review that I didn't strip out the value for CRYPTO_AES_CCM_16, but could if really necessary.

207–209

I know I had a reason for doing it this way, but I'm blanking on it for now. I'll leave this incomplete until I either remember why, or change it.

[sef]

More response to feedback from cem. Still incomplete; I have to refactor the code a bit to deal with unexpected usage patterns.

[cem]

In D18592#399382, @sef wrote:

In D18592#398160, @cem wrote:

I don't know there are any you can directly copy. These two I know of might be useful for inspiration:

Then I don't think it's something I can really do.

• There is some correctness comparison against OpenSSL's implementation in tools/tools/crypto/cryptocheck.c, but that's not suitable for this revision (cryptodev interface) nor is it KAT vectors. It looks like OpenSSL documents a CCM Mode, but I don't know if we enable/build it or not.

I *do* have cryptocheck changes later.

Ok, punt on this for now.

More response to feedback from cem. Still incomplete; I have to refactor the code a bit to deal with unexpected usage patterns.

I'll wait for the upcoming revision, thanks.

sys/opencrypto/cbc_mac.c

158

On the commented revision you have if (ctx->authDataCount = ctx->authDataLength) — should have been == instead of =, which you fixed in the recent update.

Modern versions of Clang and GCC typically produce a warning when = is used in if conditions, and we build the kernel with -Werror, so I suspect the submitted patch was never compiled successfully. But it's also possible we have disabled warnings / -Werror on this file.

sys/opencrypto/cryptodev.h

206

It seems easy enough to remove here and add in the revision that adds CCM so I'd certainly prefer that.

207–209

Thanks!

[jhb]

In regard to KAT vectors. I think it would be fine to use the approach in the python tester. It pulls the test vectors from a security/nist-kat port, so you would need to update the port to include the relevant test vectors (if it doesn't already) and then modify the python script Conrad mentioned earlier to exercise the new vectors. I think you can probably look at how the script handles GCM to get a sense of what is needed.

[jhb]

To avoid the need for 3 separate constants you could just make the software crypto code switch on the key size to pick an auth_hash *. This would require the key during session setup, but that's ok I think. GCM already requires it I believe. It would be ok I think to have the .type members of the 3 auth_hash's all use the same value. Eventually in my ocf_rework branch the goal would be just to say that you use AES-CCM (or AES-GCM) as your cipher and the corresponding MAC is implied without needing to be specified explicitly (so the 3 constants then become unused entirely). However, we aren't there yet, so we probably need at least one constant for now.

[cem]

In D18592#399571, @jhb wrote:

[The Python KAT tester] pulls the test vectors from a security/nist-kat port, so you would need to update the port to include the relevant test vectors (if it doesn't already)

It doesn't have them yet, last time I looked.

and then modify the python script ... to exercise the new vectors. I think you can probably look at how the script handles GCM to get a sense of what is needed.

The script wraps /dev/crypto in a crappy way that Sean would have to add to; see the adjacent cryptodev.py and cryptodevh.py. It's... ugly. In the past there were multiple ways it could report success without validating anything, e.g. r323843 and r323878. So be careful it is actually testing anything when you add CCM tests. I don't object to more-of-the-same, if you're careful, though I'd much rather replace it with something that wasn't crap. :-)

[sef]

More changes due to feedback. Biggest one being a change to the _Update and _Final routines.

NB: This is mostly tested simply by compilation, as I've still got some more feedback to incorporate.

[sef]

sys/opencrypto/cryptodev.h

207–209

Oh. I did it that way because of CRYPTO_AES_${SIZE}_NIST_GMAC.

[sef]

Prod?

[jhb]

sys/opencrypto/cryptodev.h

207–209

Yes, that's the bug (there wasn't a good reason for GCM to do it). It would be really nice to not have it if we can avoid it.

[sef]

sys/opencrypto/cryptodev.h

207–209

It's also what CRYPTO_SHA2_${SIZE} does. So it's not just GCM. Are there any of the crypto or auth ones that do? (He asks while rewriting the code to see how annoying or not it is :).)

[sef]

sys/opencrypto/cryptodev.h

207–209

Well, any auth ones that do; the crypto ones have min and max keys, while the auth struct only has key size.

[jhb]

sys/opencrypto/cryptodev.h

207–209

But AEAD (GCM and CCM) are different. They don't actually have separate keys. You use the same key for both the cipher and the auth. I think it's a bug that the CRYPTO_AES_*_NIST_GMAC constants exist at all and plan to remove them in my OCF rework branch. There should only be the GCM (cipher with tag) and GMAC (hash) algorithms in my mind with the right axf pointer chosen based on the key size. Similarly for CCM and CBC-MAC. I know we can't yet quite get there today in the current OCF, but I think we could easily use a single constant for the auth side of CCM for now and have drivers and consumers just switch on the key size to choose the right axf pointer.

https://github.com/bsdjhb/freebsd/compare/master...ocf_rework shows the WIP

[sef]

sys/opencrypto/cryptodev.h

207–209

I've got two responses to this, equally valid:

First, I can surely rip out all of the specific key size ones, and just have a generic one, because the code isn't plumbed up -- and won't be until the code that changes the OCF interfaces. But...

Second, that's telling me to write this code to an API that isn't currently in the system, while this is intended to go into the current system. And in the existing one, I have to have the different keysize variants as distinct entries, because the setup code will fail if the key sizes don't match.

I'm blocked on doing the other parts of this, since it was requested to be broken down into multiple smaller bits; that in turn is blocking the ZFS work we want to move forward with.

So which is it: do I write this for a future kernel change, or do I write this for the existing one?

[jhb]

sys/opencrypto/cryptodev.h

207–209

I'm saying it's fine to have multiple struct auth_xform that differ per key-size. I would just have a single constant and have code that's using the constant do something like:

case CRYPTO_AES_CCM_CBC_MAX:
     switch (keysize) {
     case 16:
         axf = &auth_<mumble>_128;
         break;
     case 24:
         axf = &auth_<mumble>_192;
         break;
     case 32:
         axf = &auth_<mumble>_256;
         break;

Does that make sense? Even with the current API I feel like GCM could/should have used that approach IMO. I think it also makes life less complicated in consumers and drivers as you don't have to add a bunch of silly checks for things like "does the specific CCM MAC constant match the key length of the cipher key" because you are using the cipher key length directly with a single constant that doesn't on its own indicate a key size.

[sef]

sys/opencrypto/cryptodev.h

207–209

My recollection at having tried different things in the past is that if I use the single entry with auth, the existing setup code will look at the key size given in the crp, and compare it with the key size given in the structure, and error out if they're not the same. My recollection could be entirely wrong of course!

[sef]

sys/opencrypto/cryptodev.h

207–209

Either my recollection was wrong, or the code change -- it looks for "thash->keysize != 0 && sop->mackeylen > thash->keysize" which means it should work. Coming up as soon as I have some reliable networking again!

[sef]

Switch to using a single type macro.

[cem]

In D18592#407465, @sef wrote:

Switch to using a single type macro.

Thanks!

[sef]

! In D18592#407482, @cem wrote:
Thanks!

Anything else? My plan here is to add CBC-MAC authentication, then AES-CCM encryption, then to hook them both into OCF (software only). (Given how small the AES-CCM changes are, I could do that and the OCF integration with the same change review.)

[cem]

In D18592#400076, @sef wrote:

NB: This is mostly tested simply by compilation, as I've still got some more feedback to incorporate.

Is this still the case or do you figure you've finished that? I was waiting for that to wrap up before I took another look.

[sef]

In D18592#407486, @cem wrote:

In D18592#400076, @sef wrote:

NB: This is mostly tested simply by compilation, as I've still got some more feedback to incorporate.

Is this still the case or do you figure you've finished that? I was waiting for that to wrap up before I took another look.

It's not plumbed into the rest of OCF, since that was requested to be a different review.

[cem]

In D18592#407487, @sef wrote:

In D18592#407486, @cem wrote:

In D18592#400076, @sef wrote:

NB: This is mostly tested simply by compilation, as I've still got some more feedback to incorporate.

Is this still the case or do you figure you've finished that? I was waiting for that to wrap up before I took another look.

It's not plumbed into the rest of OCF, since that was requested to be a different review.

I meant the second half of the sentence — are you still working on feedback or not?

[sef]

In D18592#407488, @cem wrote:

I meant the second half of the sentence — are you still working on feedback or not?

Oh! I think I've got all the feedback, but that's what *I* was asking -- did I miss anything? :)

[jhb]

I'm generally happy with this. The #if 0 -> #ifdef CRYPTO_DEBUG change still seems unrelated, but I don't care strongly about it either way.

[sef]

In D18592#408160, @jhb wrote:

I'm generally happy with this. The #if 0 -> #ifdef CRYPTO_DEBUG change still seems unrelated, but I don't care strongly about it either way.

Yay ok. I'll work on the CCM review next; as I said before, the actual CCM changes are trivial, so I'll probably do a combined CCM + OCF-integration one. (Software only.)

[sef]

Per discussion in my other review, revert my change for the crypto debug macro ifdef.

[sef]

Ping? Is this one ok to go?

[cem]

Looks good except for one remaining item.

sys/opencrypto/cbc_mac.c

220

Open question - What is the 0xff for? 15 - nonceLength must be in [2, 8].

[sef]

sys/opencrypto/cbc_mac.c

220

Ok, got that one, and the other unnecessary 0xff. (Updating both revisions next.)

[sef]

Feedback from cem (thanks!).

[cem]

sys/opencrypto/cbc_mac.c

94

this & 0xff made sense before :-)

sure, without it the value is silently truncated to typeof(*dst) == uint8_t, but the bitwise AND made the truncation slightly more obvious.