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

[cem]

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@

[olivier]

This comment was removed by olivier.

[olivier]

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

[cem]

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.

[olivier]

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

[ae]

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?

[olivier]

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

[olivier]

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]

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

[ae]

sys/netipsec/key.c

6316

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

[ae]

sys/netipsec/key.c

6316

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

[olivier]

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]

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.

[cem]

Switch to the more conservative change (just use hashsize)

[cem]

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

[ae]

If it restores the old behavior, I have no objection