Page MenuHomeFreeBSD

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

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

Details

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

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

jhb created this revision.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 ↗(On Diff #57652)

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 ↗(On Diff #57652)

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

jhb updated this revision to Diff 57656.May 21 2019, 10:12 PM
  • Drop extra blank line.
Harbormaster completed remote builds in B24372: Diff 57656.
jhb added inline comments.May 21 2019, 10:26 PM
sys/netipsec/xform_ah.c
111 ↗(On Diff #57656)

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.

bz added a comment.May 21 2019, 10:28 PM

Did you also want something for ipcomp?

jhb added a comment.May 21 2019, 10:46 PM

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
195 ↗(On Diff #57652)

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.

111 ↗(On Diff #57656)

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).

sys/netipsec/xform_esp.c
97–98 ↗(On Diff #57656)

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 ↗(On Diff #57652)

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
gnn accepted this revision.May 21 2019, 11:54 PM
gnn added a subscriber: gnn.

Thanks.

jhb added inline comments.May 22 2019, 12:18 AM
sys/netipsec/xform_ah.c
195 ↗(On Diff #57652)

Yes, I'm probably inclined to still remove this one whereas with Camellia there may be a reason to keep it.

111 ↗(On Diff #57656)

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.

sys/netipsec/xform_esp.c
97–98 ↗(On Diff #57656)

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 ↗(On Diff #57652)

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 ↗(On Diff #57656)

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 ↗(On Diff #57652)

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.)

ae added a comment.May 22 2019, 6:55 AM

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.

jhb updated this revision to Diff 57798.May 23 2019, 8:41 PM
  • Add individual warn variables.
This revision now requires review to proceed.May 23 2019, 8:41 PM
jhb marked 2 inline comments as done.May 23 2019, 8:42 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 23 2019, 10:07 PM
This revision was automatically updated to reflect the committed changes.