Page MenuHomeFreeBSD

Add support for IPSec ESN and pass relevant information to crypto layer
ClosedPublic

Authored by jaz_semihalf.com on Nov 14 2019, 12:31 PM.
Tags
None
Referenced Files
F80193696: D22369.id77831.diff
Fri, Mar 29, 1:50 AM
Unknown Object (File)
Dec 31 2023, 2:32 AM
Unknown Object (File)
Dec 31 2023, 2:32 AM
Unknown Object (File)
Dec 31 2023, 2:31 AM
Unknown Object (File)
Dec 31 2023, 2:31 AM
Unknown Object (File)
Dec 31 2023, 2:31 AM
Unknown Object (File)
Dec 23 2023, 1:05 AM
Unknown Object (File)
Dec 13 2023, 3:10 AM
Subscribers

Details

Summary

Implement support for including IPSec ESN (Extended Sequence Number) to
both encrypt and authenticate mode (eg. AES-CBC and SHA256) and combined
mode (eg. AES-GCM). Both ESP and AH protocols are updated. Additionally
pass relevant information about ESN to crypto layer.

For the ETA mode the ESN is stored in separate crp_esn buffer because
the high-order 32 bits of the sequence number are appended after the
Next Header (RFC 4303).

For the AEAD modes the high-order 32 bits of the sequence number
[e.g. RFC 4106, Chapter 5 AAD Construction] are included as part of
crp_aad (SPI + ESN (32 high order bits) + Seq nr (32 low order bits)).

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

jaz_semihalf.com retitled this revision from Pass information about ESN to crypto layer to Add support for IPSec ESN and pass relevant information to crypto layer.
jaz_semihalf.com edited the summary of this revision. (Show Details)

Overall looks fine to me. I can't recall if I had suggested an explicit crp_flag to know when crp_esn was valid (rather than assuming that all operations in an ESN session have it, but perhaps it's ok to use the session flag. With some other flags like SEPARATE_AAD or separate output buffers clients want the option on a per-request basis, but with ESN the client (IPsec) will always be setting it.

sys/netipsec/xform_esp.c
363

Moving this is fine but looks like a no-op?

659

Alternatively, you could move the free(crp->crp_aad, M_XDATA) under the existing if (crp != NULL) below since free(NULL, ..) is defined to be a nop.

999

As below, you can just call free() unconditionally. There is also no need to clear crp_aad.

1036

This can just be unconditional like the rest of this block since free(NULL) is ok.

FYI, I've tried to set parent/child relationships for all the ESN-related reviews (you can see the graph in the "Stack" tab).

Address @jhb comments mostly regarding to freeing crp->crp_aad.

By mistake previous diff was updated without full context- fix this by uploading diff with full context.

jaz_semihalf.com added inline comments.
sys/netipsec/xform_esp.c
363

This was leftover from previous approach - I've undo this change. Thank you.

This patch contains many magic numbers 4, 8 and 32 that should either be removed by the proper use of sizeof() or turned into #defines that are named.

sys/netipsec/xform_ah.c
664

Please change the magic number (4) to either a #define or sizeof(seqh)

1047

Same as previous memcpy

sys/netipsec/xform_esp.c
229

Push else up to } else and please check indenting in this section.

372

Add braces to make this clearer as to which operation goes where.

374

Convert magic number (4) to a #define or const.

795

#dedfine for magic number

jaz_semihalf.com marked an inline comment as done.

Address @gnn comments: get rid of various magic numbers and improve formatting.

jhb added inline comments.
sys/netipsec/xform_esp.c
383

Minor nit: blank line before here.

920

Here as well.

This revision is now accepted and ready to land.Oct 6 2020, 5:52 PM