Add AES-CCM encryption

[sef]

Summary

This adds the encryption part of AES-CCM. Note that change for that is very small -- it's the changes to sys/opencrypto/xform_aes_icm.c -- so I also added the changes to bring it into OCF. In order to test it, I also put in my changes for cryptocheck. (As a side note, would it make sense for cryptocheck to change the kern.cryptodevallowsoft sysctl itself?)

Depends on D18592

(I am not sure if I need to do anything to get that dependency otherwise noted. I created this diff based on the code in my branch for D18592, so it has to go in first.)

Test Plan

cryptocheck -a aes-ccm -d soft 32768

[cem]

Thanks, mostly looks good! The feedback is a bit verbose but I try to go over crypto code with a fine-toothed comb. I'll try to get some time to look at the latest version of the previous change.

This adds the encryption part of AES-CCM. Note that change for that is very small -- it's the changes to sys/opencrypto/xform_aes_icm.c -- so I also added the changes to bring it into OCF. In order to test it, I also put in my changes for cryptocheck.

Small revisions don't cost anything and are easier to understand one at a time; I would prefer the smaller pieces.

(As a side note, would it make sense for cryptocheck to change the kern.cryptodevallowsoft sysctl itself?)

I think that's probably fine (if it changes it back), or at least it could detect it and warn more explicitly than whatever the error message is.

Depends on D18592

(I am not sure if I need to do anything to get that dependency otherwise noted. I created this diff based on the code in my branch for D18592, so it has to go in first.)

You can set a parent/child relation in the Phabricator web interface if you want. I don't think it's super important to track there vs in plaintext. I also don't know if there's a way to set it when you initially create a revision with arc.

sys/opencrypto/cryptodev.c

1028

Worth updating the comment to match

sys/opencrypto/cryptodev.h

381

This should probably not be included in this changeset.

sys/opencrypto/cryptosoft.c

525

/* FALLTHROUGH */ (style(9))

532

Seems like this should use one of the AES_CCM_IV_LEN or AES_GCM_IV_LEN. I'd also suggest adding a _Static_assert that the two macro values are identical.

535

This seems redundant due to line 550-551 below.

552–571

After this change, we're missing a check that the MAC matches the cipher. I.e., CRYPTO_AES_NIST_GCM_16 + CRYPTO_AES_CCM_CBC_MAC (or the other way around) is invalid, but undetected.

604

This condition is tautologically true (due to lines 539 and 551 above) and can be removed (or asserted, if you prefer).

654–655

Can you refresh my memory on why this is needed? (Maybe that suggests it's worth a comment in the code.)

666–667

It may not be implemented for CCM (yet), but this deserves at least a comment mentioning decrypt_multi. I'd suggest adding an assert, something like:

if (isccm) {
    KASSERT(exf->encrypt_multi == NULL, "assume CCM is single-block only");
    exf->decrypt(...);
}

Preemptively implementing decrypt_multi would also be harmless, if maybe silly:

if (isccm) {
    if (exf->decrypt_multi == NULL)
        exf->decrypt(...);
    else
        exf->decrypt_multi(..., len);
}

(If someone were to implement multi-block mode for CCM and forget to change this decrypt() call to a decrypt_multi(), len may exceed blksz and only the first blksz bytes of len would be decrypted here.)

701

exf->reinit != NULL (style(9))

But is it possible for reinit to be NULL for CCM? I wouldn't think so or you wouldn't be adding this logic. So maybe KASSERT is more appropriate than the condition.

710–714

You're probably more familiar with CCM than me. Do we need to decrypt a second time?

1273

Nitpicky and not especially important, but I'd probably put the CCM cases adjacent to each other.

sys/opencrypto/xform_aes_icm.c

84–90

I know the GCM/ICM example above does not, but please use C99 initializers. If you want them all to match, I'd be happy change icm/gcm first.

123–124

style(9) puts whitespace between these lines.

Casting caddr_t directly to a struct pointer may produce warnings. If it does, I'd use (void *) in place of the explicit struct cast. Not a big deal, I get that you cribbed it from icm/gcm directly above.

130

Maybe AES_CCM_IV_LEN instead of GCM

tools/tools/crypto/cryptocheck.c

104–111

This comment should be updated.

1174–1176

I'd probably convert many of these const char * -> const void * to avoid a lot of the casts required for OpenSSL.

1188–1191

For reasons that are not clear to me, the OpenSSL manual page for EVP_CIPHER_CTX_ctrl documents these as you have named them. But the canonical names in the header and used internally are EVP_CTRL_AEAD_SET_IVLEN / EVP_CTRL_AEAD_SET_TAG. Ok as-is, but I wouldn't object to the AEAD names.

1221

The canonical spelling of this seems to be EVP_CTRL_AEAD_GET_TAG. If you prefer, there is also the CCM-specific spelling EVP_CTRL_CCM_GET_TAG. But either way, GCM is probably not the best option.

1272–1273

The ioctl error case does not do FSESSION, and I think both exit cleanups should be consistent.

Of course, FSESSION is redundant with close() — otherwise a userspace program could leak kernel resources after it exits, which is not permitted.

1308

If iv_len will just be the default OpenSSL IV length, why do we set it in openssl_ccm_encrypt()? GCM doesn't.

[sef]

sys/opencrypto/cryptodev.c

1028

Changing it to "GCM/CCM".

sys/opencrypto/cryptodev.h

381

This is actually in D18592. I can change it back to the #if 0; I have it this way because I had a kernel config that defined it. As was mentioned elsewhere, it should probably be a sysctl, but I shouldn't do that in this change request. I can do it later.

sys/opencrypto/cryptosoft.c

532

I don't disagree but that wasn't my line of code.

535

Huh? I'm not sure what you mean there..

604

Holdover, and I *thought* about changing it, but hadn't gotten around to it. Until now.

654–655

I'll add a comment to the effect, but: CBC auth is done on the unencrypted data, while GCM auth is done on the encrypted data. This is a really bad algorithm design.

666–667

I'll add the assert.

701

Hm, no, it's not possible for it to be NULL for ccm. So let's make it an assert.

710–714

Sadly yes. Because we have to authenticate the data first, before it can be decrypted. GCM uses the encrypted data for authentication, so it's both faster and more secure; CCM+CBC uses the plain-text data for authentication.

1273

Easy enough to do. I was grouping them based on encryption vs authentication.

sys/opencrypto/xform_aes_icm.c

84–90

Done. I didn't change the others.

123–124

I think I had a blank line there before, but it was before I changed my emacs macros so it had a tab in it, and got deleted when I was cleaning such things up. Fixed.

130

Gonna blame bad eyes here -- the C and G looked alike.

tools/tools/crypto/cryptocheck.c

104–111

Done

1174–1176

I was cribbing from the openssl_gcm_encrypt function, which uses const char *.

1188–1191

I got the constants from the man pages :).

1221

Guess what I copied from :). Changed it to AEAD.

1272–1273

Going back to, "that's what the gcm test case does."

1308

Because technically CCM can have a variable IV length. I did not have it variable for my implementation because the OCF framework doesn't really have enough communication for that (larger IV length means shorter message length, and vice versa). I can change it, if you want, but in this case, that is what was in my head when I wrote this.

[sef]

Feedback from cem.

[sef]

I changed my #ifdef CRYPT_DEBUG back to #if 0 in the parent branch/review, so this has the new version of that.

[sef]

Updating because I changed the CBC-MAC revision.

[sef]

Ping?

[jhb]

I would probably not fiddle with the sysctl in crypto check FWIW. I originally wrote it to test ccr0, not "soft". Will look at other parts of the patch in a bit.

[jhb]

sys/opencrypto/cryptosoft.c

647

In my rework branch I will probably address this by splitting ccm out into its own function. (It already has a separate swcr_gcm,) This is probably fine for now.

tools/tools/crypto/cryptocheck.c

1188–1191

I think the CCM ones are the right ones. OpenSSL has some special EVP's that are combined CBC with HMAC that use the AEAD ones and I think OpenSSL just reuses the constants for CCM/GCM.

1221

I would use CCM here to match your earlier use of CCM FWIW.

[sef]

tools/tools/crypto/cryptocheck.c

1188–1191

Ok, I'll change it to EVP_CTRL_AEAD_SET_TAG. (Diff incoming)

[sef]

Feedback from jhb.

[cem]

sys/opencrypto/cryptosoft.c

532

Understood, but it's now part of CCM via case CRYPTO_AES_CCM_16 fallthrough.

535

At lines 550-551, we exit if we didn't find both a crde and crda. Therefore, we only need to set isccm for one of the crds — crde or crda. It is redundant to set it twice.

654–655

Thanks! :)

sys/opencrypto/xform_aes_icm.c

130

Yeah, they're tough for me to tell apart too. Thanks!

tools/tools/crypto/cryptocheck.c

1272–1273

Yeah, I mean, cribbing a prior example doesn't totally remove the expectation of critical thought. It's good to follow an existing pattern when it makes sense, but if it doesn't, it doesn't.

1308

Sure, it can. But this code doesn't really check the property you want (EVP_CIPHER_iv_length(cipher) == AES_CCM_IV_LEN) but instead sets the iv length to the value it already had (just a convoluted form of set_iv_len(cipher, get_iv_len(cipher))). I would be supportive of either:

1. Removing the explicit set iv_length and adding a check to verify openssl's EVP_CIPHER_iv_length() matches AES_CCM_IV_LEN, or
2. Removing EVP_CIPHER_iv_length() and always explicitly setting iv length to AES_CCM_IV_LEN.

But the current construction doesn't make sense to me.

[cem]

All of the changes since the initial patch LGTM; I count 4 issues from my initial pass that I'd consider unresolved for now (all commented on in the previous update, should be pretty clear). Like before, mostly LGTM, minus the few relatively minor issues.

sys/opencrypto/cryptodev.h

381

(The #ifdef CRYPTO_DEBUG was from D18592, but the #define CRYPTO_DEBUG 1 was new in this revision and what my comment was referring to.)

Looks good now

[sef]

I was going to upload a new diff, but just got email that you've got more comments :).

sys/opencrypto/cryptosoft.c

535

Oh, I see.

tools/tools/crypto/cryptocheck.c

1272–1273

_None_ of the code in cryptocheck.c does that. But I'll change it.

1308

Oh, I was looking at the wrong code.

[sef]

cem's feedback.

[cem]

LGTM, thanks

tools/tools/crypto/cryptocheck.c

1314

I'd probably remove the verbose condition — it would be surprising if it is ever printed (given it is not today) and we want to know about it when it is, rather than silently aborting the test.

[sef]

tools/tools/crypto/cryptocheck.c

1314

I wondered about making it a warn, but the (again :)) gcm function does a sanity check, and only prints out a message if verbose. So that's what I went with. I do think both should be warns.