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.
Details
- Reviewers
jhb jmg eadler - Group Reviewers
Src Committers
Build, install, run. Use "cryptocheck -a aes-ccm 512" to verify it matches OpenSSL.
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
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? |
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?
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. |
Even though it exits, I should have a va_end() because otherwise someone will use it as an example.
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? |
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.
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.
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.
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.
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.
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.
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.
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.
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. |