Page MenuHomeFreeBSD

Add AES-CCM encryption
ClosedPublic

Authored by sef on Feb 6 2019, 12:44 AM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sef created this revision.Feb 6 2019, 12:44 AM
sef added a reviewer: mmacy.Feb 6 2019, 7:35 PM
cem added a comment.Feb 6 2019, 10:01 PM

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 ↗(On Diff #53613)

Worth updating the comment to match

sys/opencrypto/cryptodev.h
381 ↗(On Diff #53613)

This should probably not be included in this changeset.

sys/opencrypto/cryptosoft.c
525 ↗(On Diff #53613)

/* FALLTHROUGH */ (style(9))

531 ↗(On Diff #53613)

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.

534 ↗(On Diff #53613)

This seems redundant due to line 550-551 below.

550–552 ↗(On Diff #53613)

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.

585 ↗(On Diff #53613)

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

631–632 ↗(On Diff #53613)

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

643–644 ↗(On Diff #53613)

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

675 ↗(On Diff #53613)

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.

681–685 ↗(On Diff #53613)

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

1244 ↗(On Diff #53613)

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 ↗(On Diff #53613)

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.

121–122 ↗(On Diff #53613)

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.

128 ↗(On Diff #53613)

Maybe AES_CCM_IV_LEN instead of GCM

tools/tools/crypto/cryptocheck.c
104–108 ↗(On Diff #53613)

This comment should be updated.

1171–1173 ↗(On Diff #53613)

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

1185–1188 ↗(On Diff #53613)

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.

1218 ↗(On Diff #53613)

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.

1269–1270 ↗(On Diff #53613)

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.

1305 ↗(On Diff #53613)

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

cem requested changes to this revision.Feb 6 2019, 10:01 PM
This revision now requires changes to proceed.Feb 6 2019, 10:01 PM
sef added inline comments.Feb 7 2019, 12:55 AM
sys/opencrypto/cryptodev.c
1028 ↗(On Diff #53613)

Changing it to "GCM/CCM".

sys/opencrypto/cryptodev.h
381 ↗(On Diff #53613)

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
531 ↗(On Diff #53613)

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

534 ↗(On Diff #53613)

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

585 ↗(On Diff #53613)

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

631–632 ↗(On Diff #53613)

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.

643–644 ↗(On Diff #53613)

I'll add the assert.

675 ↗(On Diff #53613)

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

681–685 ↗(On Diff #53613)

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.

1244 ↗(On Diff #53613)

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

sys/opencrypto/xform_aes_icm.c
84–90 ↗(On Diff #53613)

Done. I didn't change the others.

121–122 ↗(On Diff #53613)

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.

128 ↗(On Diff #53613)

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

tools/tools/crypto/cryptocheck.c
104–108 ↗(On Diff #53613)

Done

1171–1173 ↗(On Diff #53613)

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

1185–1188 ↗(On Diff #53613)

I got the constants from the man pages :).

1218 ↗(On Diff #53613)

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

1269–1270 ↗(On Diff #53613)

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

1305 ↗(On Diff #53613)

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 updated this revision to Diff 53631.Feb 7 2019, 12:55 AM

Feedback from cem.

sef updated this revision to Diff 53634.Feb 7 2019, 1:42 AM

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

sef updated this revision to Diff 53760.Feb 11 2019, 6:33 AM

Updating because I changed the CBC-MAC revision.

jhb added a comment.Feb 14 2019, 7:39 PM

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 accepted this revision.Feb 14 2019, 7:47 PM
jhb added inline comments.
sys/opencrypto/cryptosoft.c
647 ↗(On Diff #53760)

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
1185–1188 ↗(On Diff #53613)

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.

1218 ↗(On Diff #53613)

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

sef added inline comments.Feb 14 2019, 8:23 PM
tools/tools/crypto/cryptocheck.c
1185–1188 ↗(On Diff #53613)

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

sef updated this revision to Diff 53938.Feb 14 2019, 8:24 PM

Feedback from jhb.

cem added inline comments.Feb 14 2019, 8:39 PM
sys/opencrypto/cryptosoft.c
531 ↗(On Diff #53613)

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

534 ↗(On Diff #53613)

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.

631–632 ↗(On Diff #53613)

Thanks! :)

sys/opencrypto/xform_aes_icm.c
128 ↗(On Diff #53613)

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

tools/tools/crypto/cryptocheck.c
1269–1270 ↗(On Diff #53613)

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.

1305 ↗(On Diff #53613)

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 added a comment.Feb 14 2019, 8:43 PM

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 ↗(On Diff #53613)

(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 added a comment.Feb 14 2019, 9:04 PM

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

sys/opencrypto/cryptosoft.c
534 ↗(On Diff #53613)

Oh, I see.

tools/tools/crypto/cryptocheck.c
1269–1270 ↗(On Diff #53613)

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

1305 ↗(On Diff #53613)

Oh, I was looking at the wrong code.

sef updated this revision to Diff 53939.Feb 14 2019, 9:05 PM

cem's feedback.

cem accepted this revision.Feb 14 2019, 9:19 PM

LGTM, thanks

tools/tools/crypto/cryptocheck.c
1314 ↗(On Diff #53939)

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.

This revision is now accepted and ready to land.Feb 14 2019, 9:19 PM
sef added inline comments.Feb 14 2019, 9:21 PM
tools/tools/crypto/cryptocheck.c
1314 ↗(On Diff #53939)

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.

This revision was automatically updated to reflect the committed changes.