Page MenuHomeFreeBSD

ipsec: Use the same keysize values for HMAC as prior to r324017
ClosedPublic

Authored by cem on Oct 23 2017, 7:31 PM.

Details

Summary

The HMAC construction natively permits any key size between 0 and the input
block length. Before r324017, the auth_hash 'keysize' member was the hash
output length, which was used by ipsec for key sizes. (Non-ipsec consumers
need the ability to use other keysizes, hence, r324017.)

The ipsec SADB code blindly uses the auth_hash 'keysize' member for both
minimum and maximum key size, which is wrong (from an HMAC perspective).
For now, just switch it to 'hashsize', which matches the existing
expectations.

Instead it should probably use the range [0, keysize]. But there may be
other broken code in ipsec that rejects hashes with too small a minimum
key size.

Reported by: olivier@

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

cem created this revision.Oct 23 2017, 7:31 PM
olivier accepted this revision.Oct 25 2017, 1:10 PM
This comment was removed by olivier.
This revision is now accepted and ready to land.Oct 25 2017, 1:10 PM

After more test there is still a problem somewhere: I'm digging it and will add more information.

cem added a comment.Oct 25 2017, 4:00 PM

After more test there is still a problem somewhere: I'm digging it and will add more information.

Thanks, let me know what you find! The alternative patch should restore the prior behavior for key_getsizes_ah exactly:

-*min = *max = ah->keysize;
+*min = *max = ah->hashsize;

But maybe the issue is somewhere else? Let me know if I can help.

olivier added a comment.EditedOct 25 2017, 10:12 PM

After more test: Your patch correctly fix aes-cbc key length.

I was confuse because I hit a new regression between r324017 and r324972 regarding aes-gcm not related to this review.

Config set aes-cbc:

flush;
spdflush;
spdadd 198.18.0.0/16 198.19.0.0/16 any -P out ipsec esp/tunnel/198.18.1.208-198.18.1.210/require;
spdadd 198.19.0.0/16 198.18.0.0/16 any -P in ipsec esp/tunnel/198.18.1.210-198.18.1.208/require;
add 198.18.1.208 198.18.1.210 esp 0x1000 -E rijndael-cbc "1234567890123456" -A hmac-sha1 "12345678901234567890";
add 198.18.1.210 198.18.1.208 esp 0x1001 -E rijndael-cbc "1234567890123456" -A hmac-sha1 "12345678901234567890";

Config set aes-gcm:

flush;
spdflush;
spdadd 198.18.0.0/16 198.19.0.0/16 any -P out ipsec esp/tunnel/198.18.1.208-198.18.1.210/require;
spdadd 198.19.0.0/16 198.18.0.0/16 any -P in ipsec esp/tunnel/198.18.1.210-198.18.1.208/require;
add 198.18.1.208 198.18.1.210 esp 0x1000 -E aes-gcm-16 "12345678901234567890";
add 198.18.1.210 198.18.1.208 esp 0x1001 -E aes-gcm-16 "12345678901234567890";

Test results: Configuration load and packet encryption.

SourceDestinationConfig set aes-cbc resultConfig set aes-gcm result
fbsd 11.1fbsd 11.1OKOK
r324016fbsd 11.1OKOK
r324016r324016OKOK
r324017fbsd 11.1Invalid key length on sourceOK
r324017 with D12770fbsd 11.1OKOK
r324972 with D12770fbsd 11.1OKNOK: netstat display "packets dropped; no transform" on destination
ae added a comment.Oct 25 2017, 10:22 PM
r324972 with D12770fbsd 11.1OKNOK: netstat display "packets dropped; no transform" on destination

Did you try without aesni module loaded?

In D12770#265548, @ae wrote:
r324972 with D12770fbsd 11.1OKNOK: netstat display "packets dropped; no transform" on destination

Did you try without aesni module loaded?

Here are the result:

source: head r324972destination: 11.1Result regarding aes-gcm
aesniaesniAll packets refused at destination: netstat display "packets dropped; no transform" on destination
no aesnino aesniVery few packets accepted, almost ALL packets are corrupted once decrypted

Without aesni loaded on both side, netstat on destination display this:

esp:
        55 packets dropped; no TDB
        1504160 packets dropped; bad encryption detected
        1516933 packets in
        806979096 bytes in
        ESP output histogram:
                aes-gcm-16: 1516878

I've found the commit that broke AES-GCM: r324037 "aesni(4): Add support for x86 SHA intrinsics".
But how is possible that I'm impacted without loading aesni module ?!?

[root@SM1]~# kldstat -v | grep aesni
=> Nothing displayed
cem added a comment.Oct 30 2017, 4:34 AM

@ae , is this change ok? Or would you prefer we set min/max to the exact same values as before (now ah->hashsize)?

ae added inline comments.Oct 30 2017, 8:04 AM
sys/netipsec/key.c
6317 ↗(On Diff #34263)

If we set minsize to zero, will this check now become functional and will reject all algorithms?

ae added reviewers: gnn, bz, jhb, jmg.Oct 30 2017, 8:09 AM
ae added inline comments.Oct 30 2017, 12:03 PM
sys/netipsec/key.c
6317 ↗(On Diff #34263)

I think this may affect IKE daemons, Olivier seems does his tests without IKE.

I confirm: I'm using static key only on my tests.

And about my (unrelated) aes-gcm problem without aesni… It seems my old bug (July 2015) resurfaced and if it's the same, this impact even all FreeBSD 11.X branch: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=201447

cem added inline comments.Oct 30 2017, 3:50 PM
sys/netipsec/key.c
6317 ↗(On Diff #34263)

Shouldn't this check be if (_BITS(maxkeysize) < V_ipsec_ah_keymin) instead?

I am happy to make the more conservative change for now, though.

cem updated this revision to Diff 34461.Oct 30 2017, 4:26 PM

Switch to the more conservative change (just use hashsize)

This revision now requires review to proceed.Oct 30 2017, 4:26 PM
cem retitled this revision from ipsec: Allow a range of keysizes supported by HMAC to ipsec: Use the same keysize values for HMAC as prior to r324017.Oct 30 2017, 4:27 PM
cem edited the summary of this revision. (Show Details)
cem edited the test plan for this revision. (Show Details)
cem added a comment.Nov 1 2017, 7:32 PM

If this is consensus, I'd like to go ahead and commit this change. Any objection?

ae added a comment.Nov 2 2017, 9:40 AM

If it restores the old behavior, I have no objection

olivier accepted this revision.Nov 15 2017, 8:35 PM
This revision is now accepted and ready to land.Nov 15 2017, 8:35 PM
This revision was automatically updated to reflect the committed changes.