Add support for AES algorithms to IPSec. While here clean up many of the magic numbers that plague this code.
Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
The nonce change and some of the define changes I mentioned are required to be fixed before I'd approve this change.
sys/netipsec/xform_esp.c | ||
---|---|---|
252 | This shouldn't include the + 4... I though we forced the klen of GMAC to be the same, but it looks like we don't generate an error. The + 4 is probably more vestiges of OpenBSD's layering violations. | |
262 | This does not look correct. AESGCM must not be using a random IV. Per 3.1 of RFC4106: As there are only 64bits here, a random IV is really bad, because after a 600 million packets, there is a 1% chance that two of them collided, and this will allow an attacker to get the XOR difference between them. At 3.3 billion, it increases to 25% chance. | |
345 | Can we do esph != NULL here? | |
357 | can !espx be espx == NULL? Is this check for GCM even necessary now that we've fixed tehe blocksize issue? or is the block size issue not fixed yet? | |
411 | espx != NULL please | |
704 | esph != NULL | |
sys/opencrypto/cryptodev.h | ||
81 | space should be a tab | |
107 | more spaces after define should be tabs | |
117 | I'd prefer to keep the RIJNDAEL128_BLOCK_LEN here, as there is no difference between the two. | |
118 | This looks bogus. AES has a block length of 16 full stop. Only when AES is used in a different mode, such as CTR aka ICM can this be true. | |
126 | This is also bogus. An IV length of 12 only applies to GCM. In almost all modes of AES, the IV length is 16. Also, for GCM it is not an IV, but a nonce. | |
148 | This is bad. We should not promote easily crackable ciphers. This should be a minimum of 12 or 16. I plan on removing some of the more questionable ciphers. | |
sys/opencrypto/xform.c | ||
260 | This is incorrect. None of our XTS implementations support using a block length of 1. They currently require a block size of 16. |
I'll update the patch again this evening or tomorrow, but I'd be interested in comments on my feedback as well so I can get this all in at once. Thanks!
sys/netipsec/xform_esp.c | ||
---|---|---|
262 | This happens once per session, not once per packet, so getting to 600 million might take a while. If a counter is what's necessary would the current time in nanoseconds be sufficient? | |
sys/opencrypto/cryptodev.h | ||
118 | If you take a look in xform.c you will see that the old magic number was 1. That's the blocksize according to the structure definition. What's the right name for this? | |
148 | Again this is all based on the original magic numbers in xform.c |
sbin/setkey/setkey.8 | ||
---|---|---|
32 | .Dd also needs to be updated when the contents change. |
Did you test functionability of this AES-GCM implementation with other systems that already have implemented AES-GCM in IPSec? AFAIK, OpenBSD and Linux have it.
This revision is too far removed from the number of fixes required to make this code acceptable to the HEAD of the tree. A newer patch set will be proposed in the coming days.
PB unfortunately shows me no diffs; so I am inclined to say "reject". From what I read in the description this should be at least two separate things. Someone please poke me when PB is working again and I am happy to review.