Page MenuHomeFreeBSD

Prepare crypto framework for IPSec ESN support
ClosedPublic

Authored by jaz_semihalf.com on May 14 2020, 1:45 PM.

Details

Summary

This permits requests (netipsec ESP and AH protocol) to provide the
IPSec ESN (Extended Sequence Numbers) in a separate buffer.

As with separate output buffer and separate AAD buffer not all drivers
support this feature. Consumer must request use of this feature via new
session flag.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jaz_semihalf.com created this revision.

FYI, I've thought more about ESN and also about what I would need to make TLS work well for TLS 1.1 (and to help with KTLS receive), and my plan is to add a flag to permit the entire AAD to be provided in a separate buffer via a crp_aad pointer as an optional feature (via a new feature flag in csp_flag similar to what I've done for separate in/out buffers in D24545). This is similar to what we do with IVs where an IV can either be inline via crp_iv_start or it can be stored in crp_iv, and it is not specific to ESN but is instead a more generic feature that can be used by different consumers. I was thinking of just having a pointer to the AAD though rather than an inline array as unlike IV's there isn't a clear upper bound on the size of AAD (T6 hardware supports AAD up to 512 bytes for example and 512 bytes is too large of an array to stick inline in crp).

Hi @jhb,

Thank you for your feedback.

Providing separate crp_aad buffer will not work for ESN. It would, if we would take into consideration only combined modes (aes-gcm, aes-ccm), where ESN could be treated as part of AAD (SPI + ESN (32 high order bits) + Seq nr (32 low order bits)).
[ RFC4106, Chapter 5 AAD Construction]

Nevertheless for not-combined modes ESN (high order 32-bits) is not part of AAD. Quote from rfc4303 RFC4303: "the ICV computation encompasses the SPI, Sequence Number, Payload Data, Padding (if present), Pad Length, and  Next Header. ... If the ESN option is enabled for the SA, the high-order 32 bits of the sequence number are appended after the Next Header field for purposes of this computation, but are not transmitted."

Therefore, I am afraid, that trying to provide a more generic feature that could be used by different consumers and not only ESN may not be possible/useful in this case.
Please let me know if I missed something. I am looking forward for your further feedback.

Thank you,
Grzegorz

I think for the AEAD case a separate crp_aad will still be the simplest path and will minimize changes to other drivers to support both ESN and TLS for CSP_MODE_AEAD. I haven't looked, but I'm curious what Chacha20 + Poly1035 for IPsec uses. Hopefully it follows the AEAD route. Just adding a new request flag alone is probably not ideal as other drivers might claim the session and fail to support the new flag. I think at a minimum you will have to add a new flag for ESN support in the non-AEAD case in csp_flags so that drivers that don't support the mode won't claim it. It looks like a flag though might be better than a new CSP_MODE_ESN that implies CSP_MODE_ETA since you can use ESN's with auth-only sessions.

Hi @jhb

Thank you for your feedback.

  1. Fortunately for Chacha20 + Poly1305: the AAD construction is the same as for AES-GCM, AES-CCM modes (RFC7634 - AAD).
  1. I think I get your point but just to make sure that I've understand correctly:
  • For AEAD case we should use separate buffer via crp_aad pointer, which will be used instead crp_aad_start in case of Ipsec ESN. This will work only for drivers which "advertise" (via *_probesession e.g. aesni_probesession) support for an optional feature via csp_flag e.g. something like CSP_F_AAD_SEPARATE. For drivers that do not advertise CSP_F_AAD_SEPARATE the *_probesession will simply fail with EINVAL.
  • For ETA case we should use both: crp_aad_start as it is, and additionally use separate buffer via crp_aad pointer. This will work only for drivers which "advertise" support for an optional feature via csp_flag e.g. something like CSP_F_ESN. In this case the crp_aad will hold only high-order 32 bits of the seq number and not entire AAD.

Could you please confirm above? Looking forward to your feedback.

Best regards,
Grzegorz

Hi @jhb

Thank you for your feedback.

  1. Fortunately for Chacha20 + Poly1305: the AAD construction is the same as for AES-GCM, AES-CCM modes (RFC7634 - AAD).
  1. I think I get your point but just to make sure that I've understand correctly:
  • For AEAD case we should use separate buffer via crp_aad pointer, which will be used instead crp_aad_start in case of Ipsec ESN. This will work only for drivers which "advertise" (via *_probesession e.g. aesni_probesession) support for an optional feature via csp_flag e.g. something like CSP_F_AAD_SEPARATE. For drivers that do not advertise CSP_F_AAD_SEPARATE the *_probesession will simply fail with EINVAL.
  • For ETA case we should use both: crp_aad_start as it is, and additionally use separate buffer via crp_aad pointer. This will work only for drivers which "advertise" support for an optional feature via csp_flag e.g. something like CSP_F_ESN. In this case the crp_aad will hold only high-order 32 bits of the seq number and not entire AAD.

Could you please confirm above? Looking forward to your feedback.

Yes, I think that all sounds good. I might even work on the generic crp_aad support this week for some KTLS related work. The use of crp_aad to hold the ETA case for ESN is kind of interesting, but I think I probably prefer to just use the crp_esn field you have in the existing patch so it is a bit clearer. In theory a driver might even support the initial AAD via crp_aad while still supporting ESN for IPsec. More likely though, I think it will be simpler to implement the right thing in drivers if they are able to assume that crp_aad is the AAD region when it is present. In the case of separate AAD, it might be simpler to use crp_aad != NULL instead of a new CRYPTO_F flag since it will be a pointer one can check against NULL (the way the per-op keys work) unlike the IV case where the array is inline. Also, using crp_esn means the two patches will compose a bit easier, so you can start work on the ESN thing now if I do work on crp_aad this week and we won't have any major conflicts (the only one would probably be the CSP_F_* values which is trivial to resolve). Does that make sense?

Yes it make sense and thank you for feedback. Just please let me know when you will have some patches around KTLS and crp_aad ready.

jaz_semihalf.com retitled this revision from Add CRYPTO_F_ESN flags and appropriate field in cryptop to Prepare crypto framework for IPSec ESN support.
jaz_semihalf.com edited the summary of this revision. (Show Details)

Looks good to me overall.

sys/opencrypto/crypto.c
755 ↗(On Diff #76978)

Minor nit: I would probably leave this split out as drivers tend to leave it split out, but I'm don't have a strong opinion either way.

This revision is now accepted and ready to land.Sep 17 2020, 5:25 PM

@jhb thank you for your review and approval. I just noticed that by accident you weren't added as reviewer for other patches related to ESN. I've just fixed that and will appreciate your review.

One thing I forgot is that the new session flag needs to be documented in crypto_session.9, and crp_esn should be documented in crypto_request.9.

Document new session flag in crypto_session.9 and crp_esn in crypto_request.9.

This revision now requires review to proceed.Oct 3 2020, 10:02 PM
share/man/man9/crypto_request.9
306 ↗(On Diff #77830)
307 ↗(On Diff #77830)
309 ↗(On Diff #77830)
321 ↗(On Diff #77830)
322 ↗(On Diff #77830)
323 ↗(On Diff #77830)

Apologies for the weird multi-suggestion, phab's "suggest edits" doesn't let you adjust a paragraph easily. The basic idea is to simplify this slightly to something like:

"AEAD modes supply the ESN in a separate AAD buffer (see RFC 4106, Chapter 5 AAD Construction)."

share/man/man9/crypto_session.9
205 ↗(On Diff #77830)

(I think bz@ dinged me before about IPSec vs IPsec)

209 ↗(On Diff #77830)

For manpages, new sentences always start on a new line. Also, a few more s/IPSec/IPsec/.

212 ↗(On Diff #77830)

I think you can leave out this paragraph as it's already covered well enough in crypto_request.9. Here we just want to explain what the session flag is for.

Address @jhb comments regarding documentation description

jhb added inline comments.
share/man/man9/crypto_request.9
306 ↗(On Diff #77927)

Sorry, I misread this as singular before instead of plural.

This revision is now accepted and ready to land.Oct 6 2020, 5:47 PM
This revision was automatically updated to reflect the committed changes.