Page MenuHomeFreeBSD

AES CCM-CBC cryptography code
AbandonedPublic

Authored by sef on May 15 2018, 6:00 PM.

Details

Reviewers
jhb
jmg
eadler
Group Reviewers
Src Committers
Summary

As part of my on-going port of ZFS crypto to FreeBSD, I had to either drop compatibility or write aes-ccm code. I naturally chose the latter. I've tested this patch by using cryptocheck (diffs for that included as well, of course). It is currently software only, and therefore very slow.

Test Plan

Build, install, run. Use "cryptocheck -a aes-ccm 512" to verify it matches OpenSSL.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sef created this revision.May 15 2018, 6:00 PM
sef updated this revision to Diff 42595.May 15 2018, 6:27 PM

Per Alexander, I created a different, more-contextful, diff.

mav added a subscriber: mav.May 15 2018, 6:56 PM
araujo added a subscriber: araujo.May 15 2018, 11:30 PM
sef updated this revision to Diff 42788.May 21 2018, 3:29 AM

I wrote AESNI code for AES-CCM+CBC-MAC.

mav added a comment.May 21 2018, 4:39 PM

Great to see it! I did only first on-surface look so far, so comments are mostly cosmetic.

sys/crypto/aesni/aesni.c
207

I suppose this printf() should not be in final version.

931

style(9) tells: "sizeof's are written with parenthesis always".

sys/crypto/aesni/aesni_ccm.c
287

This line is > 80 chars.

sys/opencrypto/xform_aes_icm.c
140

Again tool long line.

tools/tools/crypto/cryptocheck.c
146

Does this belong to this patch? It that code broken, or what?

sef updated this revision to Diff 42801.May 21 2018, 5:10 PM

Fix the fixable parts of mav's feedback. (cryptocheck.c remains the same, per my response to him.)

Mostly LGTM. Is there a reason to keep these #if 0's in tools/tools/crypto/cryptocheck.c?

sef added a comment.May 24 2018, 8:23 PM

Mostly LGTM. Is there a reason to keep these #if 0's in tools/tools/crypto/cryptocheck.c?

As I said to mav, the file doesn't compile without them. I can remove them, since it's not built as part of buildworld, but then I have to keep putting them back when I want to use it for testing.

tools/tools/crypto/cryptocheck.c
146

Yeah, it wouldn't compile with those ciphers. OpenSSL version differences.

sef updated this revision to Diff 42971.May 25 2018, 4:39 AM

Even though it exits, I should have a va_end() because otherwise someone will use it as an example.

sef updated this revision to Diff 43001.May 25 2018, 5:24 PM

Somehow my copy of tcpdump's README got tweaked and in the diff. I've removed it.

jmg added a comment.May 27 2018, 10:24 PM

Per comments, there are a number of improvements that should be made.

I was unable to review that the code matches an implementation, as the code does not state what implementation it implements. Even if I review it, a professional cryptographer needs to be paid to review the code before it is committed/enabled for general use.

sys/crypto/aesni/aesni.c
837

What standard did you reference to decide that CCM's nonce should be limited to 12 bytes?

sys/crypto/aesni/aesni_ccm.c
87

why is this code duplicated? Why isn't aesni_enc used?

91

This is not a safe cast. You're taking a void * pointer which has zero alignment restrictions and applying it to a 16 byte alignment restricted pointer.

106

this function makes me a bit nervous. Just because it doesn't use existing primitives.

136

Start instead of Tart?

150

using _mm_setzero_si128 is probably better/faster.

151

to what standard is this formatted to? It looks like it's NIST SP800-38C, but I cannot assume this. Don't want to waste time assuming something that's incorrect.

I cannot verify anymore of the algorithm w/o knowing what algorithm you have implemented.

440

why doesn't this just use the key expansion schedule functions that are provided?

sys/opencrypto/ccm-cbc.c
23

Please fix this so that the function does NOT unencrypted data to dst, and then does a later decryption pass. This is a great way to accidentally leak plaintext. The function does not appear to do this, so possibly a comment that was not updated.

59

I have not verified that this is correct. Why is this done significantly differently than the way gmac does it?

Per above, as I don't know what algorithm was implemented, I have not reviewed the implementation.

sys/opencrypto/cryptodev.c
487–496

why the blank line here?

tools/tools/crypto/cryptocheck.c
146

is this because you're using/testing an old version, or that the version in -current can't compile these?

sef added a comment.EditedMay 28 2018, 12:41 AM

I was unable to review that the code matches an implementation, as the code does not state what implementation it implements. Even if I review it, a professional cryptographer needs to be paid to review the code before it is committed/enabled for general use.

Then there is no point to responding to anything in this, since I cannot afford to hire a cryptographer, and all my attempts to get someone with actual crypto experience to work on it were ignored or responded with "it's easy, you can just see what the aes-gcm code does."

Since I never found any reference to a different AES-CCM standard, I assumed that there was only one. I used https://tools.ietf.org/html/rfc3610 as the reference for implementation, and compared it to a dozen or more code snippets google found for me.

mav added a comment.May 29 2018, 3:13 PM
In D15446#329261, @jmg wrote:

Even if I review it, a professional cryptographer needs to be paid to review the code before it is committed/enabled for general use.

That's something new to me. Did we ever do that before? Who and how shall do that? I can understand when secteam@ review is requested for crypto code, that is reasonable, but who is a "professional cryptographer"? I think it was kind of agreed before that every review in this area usually starts from "I am not a cryptographer, but ..." disclaimer.

sef updated this revision to Diff 43210.May 31 2018, 5:37 PM

Rewrote the AESNI decryption function to not leak the data. Added a link to the RFC I used as the specification. Cleaned up some code a bit. Became the first consumer of the aesencdec.h inline function.

jmg added a comment.Jun 2 2018, 5:17 PM
In D15446#329550, @mav wrote:
In D15446#329261, @jmg wrote:

Even if I review it, a professional cryptographer needs to be paid to review the code before it is committed/enabled for general use.

That's something new to me. Did we ever do that before? Who and how shall do that? I can understand when secteam@ review is requested for crypto code, that is reasonable, but who is a "professional cryptographer"? I think it was kind of agreed before that every review in this area usually starts from "I am not a cryptographer, but ..." disclaimer.

Yes, we have done this before. When I did the GCM work under contract for the FreeBSD Foundation, they paid for a third party reviewer to go over the code and make sure it was correct. So, yes, we have done this before.

sef added a comment.Jun 2 2018, 5:19 PM

Yes, we have done this before. When I did the GCM work under contract for the FreeBSD Foundation, they paid for a third party reviewer to go over the code and make sure it was correct. So, yes, we have done this before.

Oh, so *I* don't have to go find one and pay for it. That was not clear. Phew.

jmg added a comment.Jun 2 2018, 5:21 PM
In D15446#329285, @sef wrote:

I was unable to review that the code matches an implementation, as the code does not state what implementation it implements. Even if I review it, a professional cryptographer needs to be paid to review the code before it is committed/enabled for general use.

Then there is no point to responding to anything in this, since I cannot afford to hire a cryptographer, and all my attempts to get someone with actual crypto experience to work on it were ignored or responded with "it's easy, you can just see what the aes-gcm code does."

This is where the Foundation should be able to help.

Since I never found any reference to a different AES-CCM standard, I assumed that there was only one. I used https://tools.ietf.org/html/rfc3610 as the reference for implementation, and compared it to a dozen or more code snippets google found for me.

Then why wasn't the RFC noted in the comments then? There is the NIST version as well, which is what I was looking at originally.

The resources that were used to write the code should be commented in the code such that future people looking at the code can understand what it's doing and that if something is wrong, why it is wrong.

jmg added a comment.Jun 2 2018, 5:23 PM
In D15446#330696, @sef wrote:

Yes, we have done this before. When I did the GCM work under contract for the FreeBSD Foundation, they paid for a third party reviewer to go over the code and make sure it was correct. So, yes, we have done this before.

Oh, so *I* don't have to go find one and pay for it. That was not clear. Phew.

Well, I can provide references for people I know that are available for crypto review, but you still have to work w/ the Foundation and the reviewer.

sef added a comment.Jun 2 2018, 5:33 PM

Then why wasn't the RFC noted in the comments then? There is the NIST version as well, which is what I was looking at originally.

As I said, I didn't think there was more than one. I've now put the URL to the RFC in two of the files, at least. Can put it more places as well.

sef added inline comments.Jun 3 2018, 1:39 AM
sys/crypto/aesni/aesni_ccm.c
87

I've changed it, but I didn't use it because the calling conventions of aesni_enc() is horrible -- passing in nr-1 is not at all good programming style.

91

I refer you to line 280 of aesni_ghash.c, which does the same thing.

106

It does pretty much exactly what it says it does, so I don't know why it would make anyone nervous.

136

Fixed now :).

440

Because it's nothing but test code, and I was limiting what external functions I was using while writing it. All the STANDALONE and CRYPTO_DEBUG code can go away, but I am still tweaking the code and have found the ability to debug and time the encryption and decryption code all on their own helpful.

eadler resigned from this revision.Jun 18 2018, 2:31 AM
sef abandoned this revision.Aug 28 2019, 7:32 PM