Paths

Table of Contentst

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

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

Details

Reviewers

olivier
gnn
bz
jhb
jmg

Commits

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

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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 12308
Build 12597: arc lint + arc unit

Event Timeline

cem created this revision.Oct 23 2017, 7:31 PM

Herald added a subscriber: ae. · View Herald TranscriptOct 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.

In D12770#265454, @olivier wrote:

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.

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.

Source	Destination	Config set aes-cbc result	Config set aes-gcm result
fbsd 11.1	fbsd 11.1	OK	OK
r324016	fbsd 11.1	OK	OK
r324016	r324016	OK	OK
r324017	fbsd 11.1	Invalid key length on source	OK
r324017 with D12770	fbsd 11.1	OK	OK
r324972 with D12770	fbsd 11.1	OK	NOK: netstat display "packets dropped; no transform" on destination

In D12770#265547, @olivier wrote:

r324972 with D12770 fbsd 11.1 OK NOK: netstat display "packets dropped; no transform" on destination

Did you try without aesni module loaded?

In D12770#265548, @ae wrote:

In D12770#265547, @olivier wrote:

r324972 with D12770 fbsd 11.1 OK NOK: netstat display "packets dropped; no transform" on destination

Did you try without aesni module loaded?

Here are the result:

source: head r324972	destination: 11.1	Result regarding aes-gcm
aesni	aesni	All packets refused at destination: netstat display "packets dropped; no transform" on destination
no aesni	no aesni	Very 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

@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
6316	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
6316	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
6316	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.

Switch to the more conservative change (just use hashsize)

This revision now requires review to proceed.Oct 30 2017, 4:26 PM

Harbormaster completed remote builds in B12308: Diff 34461.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)

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

If it restores the old behavior, I have no objection

olivier mentioned this in D13017: Add IPSec tests in tunnel mode.Nov 9 2017, 2:26 PM

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

Closed by commit rS325872: ipsec: Use the same keysize values for HMAC as prior to r324017 (authored by cem). · Explain WhyNov 15 2017, 10:42 PM

This revision was automatically updated to reflect the committed changes.