Add deprecation warnings for IPsec algorithms deprecated in RFC 8221.
ClosedPublic
Actions

Authored by jhb on May 21 2019, 9:40 PM.

Details

Reviewers

bjk
cem
gnn

Commits

rS348205: Add deprecation warnings for IPsec algorithms deprecated in RFC 8221.

Summary

All of these algorithms are either explicitly marked MUST NOT, or they
are implicitly MUST NOTs by virtue of not being included in IETF's
list of protocols at all despite having assignments from IANA.

Specifically, this adds warnings for the following ciphers:

des-cbc
blowfish-cbc
cast128-cbc
des-deriv
des-32iv
camellia-cbc

Warnings for the following authentication algorithms are also added:

hmac-md5
keyed-md5
keyed-sha1
hmac-ripemd160

Test Plan

used setkey with configurations using the deprecated warnings and verified I got warnings. Also used setkey with some other configurations such as GCM and confirmed I did not get any deprecation warnings.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 24372
Build 23192: arc lint + arc unit

Event Timeline

jhb created this revision.May 21 2019, 9:40 PM

Herald added a subscriber: ae. · View Herald TranscriptMay 21 2019, 9:40 PM

Harbormaster completed remote builds in B24368: Diff 57652.May 21 2019, 9:40 PM

jhb added inline comments.May 21 2019, 10:10 PM

sys/netipsec/key.c
8598 ↗	(On Diff #57652)	Bah, I will drop this.
sys/netipsec/xform_ah.c
195	RFC 8221 doesn't explicitly mention ripemd160 either way (I think's it just a bit obscure). It's use was documented in RFC 2857, but other hits in google suggest it was popular before SHA2.
sys/netipsec/xform_esp.c
176	RFC 8221 does not explicitly name Camellia either. Note that RFC 4429 lists CTR and CCM modes for Camellia in addition to CBC, but we only implement CBC. @bjk had maybe suggested keeping Camellia in the thread on arch@.

Drop extra blank line.

Harbormaster completed remote builds in B24372: Diff 57656.May 21 2019, 10:12 PM

Harbormaster completed remote builds in B24372: Diff 57656.

jhb added inline comments.May 21 2019, 10:26 PM

sys/netipsec/xform_ah.c
111	BTW, I used rate limiting for the warnings because the set key configurations I tested would spam the console twice for each configuration as there were separate configurations for each direction. The rate limit reduced this to one warning per setkey invocation.

Did you also want something for ipcomp?

I wasn't planning on doing ipcomp yet, but might propose a diff for that next.

cem accepted this revision.May 21 2019, 11:50 PM

cem added inline comments.

sys/netipsec/xform_ah.c
111	I think this should be built into `gone_in()`, honestly. Possibly even a limit of "once" (although I can understand an argument that there is some value in printing at least once more after the deluge of boot messages has scrolled by).
195	There are Reasons not to use shorter hashes (<256 bit) in general; I don't know if they apply here. But I don't feel too bad about removing a hash that isn't even mentioned in the update RFC.
sys/netipsec/xform_esp.c
97–98	It's a little odd that there are per file rather than per algorithm (maybe an argument for building rate-limiting in to `gone_in` instead). For example, someone who has two deprecated algorithms configured may get only one warning. This is a bit of a tortured hypothetical, though.
176	I am happy to remove an obscure-ish cipher. I think we could be open to keeping it if enough users really must have it for some reason (like FCP 101). (Only for Camellia. DES, Blowfish, and Cast128 should go.)

This revision is now accepted and ready to land.May 21 2019, 11:51 PM

Thanks.

jhb added inline comments.May 22 2019, 12:18 AM

sys/netipsec/xform_ah.c
111	I think it's a bit hard to build into gone_in though I appreciate the sentiment. (For example if a user tries one bad algo and then switches to another bad one, you want them to keep getting messages.) I don't know how gone_in could really cope with that. It could maybe track the last string passed in, but that's kind of crappy as well and in the case of IPsec that would result in spam if you are using a bad hash + bad cipher.
195	Yes, I'm probably inclined to still remove this one whereas with Camellia there may be a reason to keep it.
sys/netipsec/xform_esp.c
97–98	You can't combine two ciphers or two auth algorithms in a single SA is part of why I did it, but I guess I could split out to N different 'lastwarn' variables.
176	The main reason I asked about Camellia is that bjk@ raised it during the thread on arch@, so I want to give Ben a chance to chime in. Camellia roughly speaking just seems to be AES with different S-boxes? Ben did say it might be popular in .jp. When digging through some other RFCs for IKE (IIRC), there are still mentions of modern versions of Camellia (there were new IKE cipher numbers for implicit IV variants of AES-GCM and Camellia-GCM in a recent RFC I saw). Of course, we only have a CBC mode for Camellia. I do not plan on adding any other modes for Camellia myself. RFC 8221 does list chacha2 + poly1035 as a "SHOULD" btw, so if there's any effort into adding new modes to IPsec it should be that IMO.

cem added inline comments.May 22 2019, 12:31 AM

sys/netipsec/xform_esp.c
97–98	I think it's vaguely plausible `N` different SAs could be initialized within 1s as part of an rc.d start script or similar, but I don't feel strongly about it.
176	Yeah, I am giving some charity to Camellia for those reasons as well. Agree Chacha-poly is the popular/well-vetted AES-alternative du jour and that's probably a better future direction, though I don't have any interest in IPsec myself. (My understanding is that S-box algorithms are difficult to implement in constant time and that one major advantage of Chacha over software AES and perhaps Camellia is ease of constant-time implementation.)

I think it would be good to have this committed into 11.3 release. So you will be able to see how many users will complain that they need this support, if any.