Date: Fri, 14 Jan 2022 09:20:47 -0900 From: Rob Wing <rew@freebsd.org> To: Mike Karels <mike@karels.net> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: eb18708ec8c7 - main - syncache: accept packet with no SA when TCP_MD5SIG is set Message-ID: <CAF3%2Bn_f8Uh_ttjWQS11dLoMtA_-Mxaon6hLRkq-3V_%2B5h9kFgw@mail.gmail.com> In-Reply-To: <202201122226.20CMQwMw007750@mail.karels.net> References: <CAF3%2Bn_eOVx=WwVqO4rHja_3r3sqmj94theYRgqmQaMxU9X5=JQ@mail.gmail.com> <202201122226.20CMQwMw007750@mail.karels.net>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Wed, Jan 12, 2022 at 1:27 PM Mike Karels <mike@karels.net> wrote: > Rob wrote: > > For what it's worth, this behavior is consistent with OpenBSD. > > That's a useful data point, thanks. > There is value in having the same semantics as OpenBSD for the TCP_MD5SIG option <https://man.openbsd.org/tcp.4>. If OpenBSD's semantics of the TCP_MD5SIG option were different, I likely would have taken a different approach. And also given that FreeBSD and OpenBSD's TCP_MD5SIG option had the same behavior until we diverged in 2017. ..that's how I ended up going in this direction -Rob > > Mike > > > -Rob > > > > > > > > Mike > > > > > > On 8 Jan 2022, at 19:45, Robert Wing wrote: > > > > > > > The branch main has been updated by rew: > > > > > > > > URL: > > > > https://cgit.FreeBSD.org/src/commit/?id=3Deb18708ec8c7e1de6a05aba41971659= > > 549991b10 > > > > > > > > commit eb18708ec8c7e1de6a05aba41971659549991b10 > > > > Author: Robert Wing <rew@FreeBSD.org> > > > > AuthorDate: 2022-01-09 01:07:50 +0000 > > > > Commit: Robert Wing <rew@FreeBSD.org> > > > > CommitDate: 2022-01-09 01:32:14 +0000 > > > > > > > > syncache: accept packet with no SA when TCP_MD5SIG is set > > > > > > > > When TCP_MD5SIG is set on a socket, all packets are dropped that > > > don't > > > > contain an MD5 signature. Relax this behavior to accept a > non-signe= > > d > > > > packet when a security association doesn't exist with the peer. > > > > > > > > This is useful when a listen socket set with TCP_MD5SIG wants to > > > handle > > > > connections protected with and without MD5 signatures. > > > > > > > > Reviewed by: bz (previous version) > > > > Sponsored by: nepustil.net > > > > Sponsored by: Klara Inc. > > > > Differential Revision: https://reviews.freebsd.org/D33227 > > > > --- > > > > share/man/man4/tcp.4 | 6 +++++- > > > > sys/netinet/tcp_syncache.c | 30 ++++++++++++++++++------------ > > > > sys/netipsec/xform_tcp.c | 5 +++++ > > > > 3 files changed, 28 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/share/man/man4/tcp.4 b/share/man/man4/tcp.4 > > > > index 17138fa224ba..d103293132ba 100644 > > > > --- a/share/man/man4/tcp.4 > > > > +++ b/share/man/man4/tcp.4 > > > > @@ -34,7 +34,7 @@ > > > > .\" From: @(#)tcp.4 8.1 (Berkeley) 6/5/93 > > > > .\" $FreeBSD$ > > > > .\" > > > > -.Dd June 27, 2021 > > > > +.Dd January 8, 2022 > > > > .Dt TCP 4 > > > > .Os > > > > .Sh NAME > > > > @@ -339,6 +339,10 @@ This entry can only be specified on a per-host > > > basis at this time. > > > > .Pp > > > > If an SADB entry cannot be found for the destination, > > > > the system does not send any outgoing segments and drops any inbound > > > segments. > > > > +However, during connection negotiation, a non-signed segment will be > > > accepted if > > > > +an SADB entry does not exist between hosts. > > > > +When a non-signed segment is accepted, the established connection > is n= > > ot > > > > +protected with MD5 digests. > > > > .It Dv TCP_STATS > > > > Manage collection of connection level statistics using the > > > > .Xr stats 3 > > > > diff --git a/sys/netinet/tcp_syncache.c b/sys/netinet/tcp_syncache.c > > > > index 7dd8443cad65..32ca3bc2209b 100644 > > > > --- a/sys/netinet/tcp_syncache.c > > > > +++ b/sys/netinet/tcp_syncache.c > > > > @@ -1514,19 +1514,25 @@ syncache_add(struct in_conninfo *inc, struct > > > tcpopt *to, struct tcphdr *th, > > > > > > > > #if defined(IPSEC_SUPPORT) || defined(TCP_SIGNATURE) > > > > /* > > > > - * If listening socket requested TCP digests, check that > received > > > > - * SYN has signature and it is correct. If signature doesn't > matc= > > h > > > > - * or TCP_SIGNATURE support isn't enabled, drop the packet. > > > > + * When the socket is TCP-MD5 enabled check that, > > > > + * - a signed packet is valid > > > > + * - a non-signed packet does not have a security association > > > > + * > > > > + * If a signed packet fails validation or a non-signed packet > ha= > > s > > > a > > > > + * security association, the packet will be dropped. > > > > */ > > > > if (ltflags & TF_SIGNATURE) { > > > > - if ((to->to_flags & TOF_SIGNATURE) =3D=3D 0) { > > > > - TCPSTAT_INC(tcps_sig_err_nosigopt); > > > > - goto done; > > > > + if (to->to_flags & TOF_SIGNATURE) { > > > > + if (!TCPMD5_ENABLED() || > > > > + TCPMD5_INPUT(m, th, to->to_signature) !=3D > 0) > > > > + goto done; > > > > + } else { > > > > + if (TCPMD5_ENABLED() && > > > > + TCPMD5_INPUT(m, NULL, NULL) !=3D ENOENT) > > > > + goto done; > > > > } > > > > - if (!TCPMD5_ENABLED() || > > > > - TCPMD5_INPUT(m, th, to->to_signature) !=3D 0) > > > > - goto done; > > > > - } > > > > + } else if (to->to_flags & TOF_SIGNATURE) > > > > + goto done; > > > > #endif /* TCP_SIGNATURE */ > > > > /* > > > > * See if we already have an entry for this connection. > > > > @@ -1724,11 +1730,11 @@ skip_alloc: > > > > } > > > > #if defined(IPSEC_SUPPORT) || defined(TCP_SIGNATURE) > > > > /* > > > > - * If listening socket requested TCP digests, flag this in the > > > > + * If incoming packet has an MD5 signature, flag this in the > > > > * syncache so that syncache_respond() will do the right thing > > > > * with the SYN+ACK. > > > > */ > > > > - if (ltflags & TF_SIGNATURE) > > > > + if (to->to_flags & TOF_SIGNATURE) > > > > sc->sc_flags |=3D SCF_SIGNATURE; > > > > #endif /* TCP_SIGNATURE */ > > > > if (to->to_flags & TOF_SACKPERM) > > > > diff --git a/sys/netipsec/xform_tcp.c b/sys/netipsec/xform_tcp.c > > > > index b53544cd00fb..ce2552f0a205 100644 > > > > --- a/sys/netipsec/xform_tcp.c > > > > +++ b/sys/netipsec/xform_tcp.c > > > > @@ -269,6 +269,11 @@ tcp_ipsec_input(struct mbuf *m, struct tcphdr > *th, > > > u_char *buf) > > > > KMOD_TCPSTAT_INC(tcps_sig_err_buildsig); > > > > return (ENOENT); > > > > } > > > > + if (buf =3D=3D NULL) { > > > > + key_freesav(&sav); > > > > + KMOD_TCPSTAT_INC(tcps_sig_err_nosigopt); > > > > + return (EACCES); > > > > + } > > > > /* > > > > * tcp_input() operates with TCP header fields in host > > > > * byte order. We expect them in network byte order. > > > > [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 12, 2022 at 1:27 PM Mike Karels <<a href="mailto:mike@karels.net">mike@karels.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Rob wrote:<br>> For what it's worth, this behavior is consistent with OpenBSD.<br> <br> That's a useful data point, thanks.<br></blockquote><div><br></div><div>There is value in having the same semantics as OpenBSD for the <a href="https://man.openbsd.org/tcp.4">TCP_MD5SIG option</a>.</div><div><br></div><div>If OpenBSD's semantics of the TCP_MD5SIG option were different, I likely would have taken a different approach. And also given that FreeBSD and OpenBSD's TCP_MD5SIG option had the same behavior until we diverged in 2017.<br></div><div><br></div><div>..that's how I ended up going in this direction<br></div><div><br></div><div>-Rob<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Mike<br> <br> > -Rob<br> <br> <br> > ><br> > > Mike<br> > ><br> > > On 8 Jan 2022, at 19:45, Robert Wing wrote:<br> > ><br> > > > The branch main has been updated by rew:<br> > > ><br> > > > URL:<br> > > <a href="https://cgit.FreeBSD.org/src/commit/?id=3Deb18708ec8c7e1de6a05aba41971659=" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=3Deb18708ec8c7e1de6a05aba41971659=</a><br>; > 549991b10<br> > > ><br> > > > commit eb18708ec8c7e1de6a05aba41971659549991b10<br> > > > Author: Robert Wing <rew@FreeBSD.org><br> > > > AuthorDate: 2022-01-09 01:07:50 +0000<br> > > > Commit: Robert Wing <rew@FreeBSD.org><br> > > > CommitDate: 2022-01-09 01:32:14 +0000<br> > > ><br> > > > syncache: accept packet with no SA when TCP_MD5SIG is set<br> > > ><br> > > > When TCP_MD5SIG is set on a socket, all packets are dropped that<br> > > don't<br> > > > contain an MD5 signature. Relax this behavior to accept a non-signe=<br> > d<br> > > > packet when a security association doesn't exist with the peer.<br> > > ><br> > > > This is useful when a listen socket set with TCP_MD5SIG wants to<br> > > handle<br> > > > connections protected with and without MD5 signatures.<br> > > ><br> > > > Reviewed by: bz (previous version)<br> > > > Sponsored by: <a href="http://nepustil.net" rel="noreferrer" target="_blank">nepustil.net</a><br> > > > Sponsored by: Klara Inc.<br> > > > Differential Revision: <a href="https://reviews.freebsd.org/D33227" rel="noreferrer" target="_blank">https://reviews.freebsd.org/D33227</a><br>; > > > ---<br> > > > share/man/man4/tcp.4 | 6 +++++-<br> > > > sys/netinet/tcp_syncache.c | 30 ++++++++++++++++++------------<br> > > > sys/netipsec/xform_tcp.c | 5 +++++<br> > > > 3 files changed, 28 insertions(+), 13 deletions(-)<br> > > ><br> > > > diff --git a/share/man/man4/tcp.4 b/share/man/man4/tcp.4<br> > > > index 17138fa224ba..d103293132ba 100644<br> > > > --- a/share/man/man4/tcp.4<br> > > > +++ b/share/man/man4/tcp.4<br> > > > @@ -34,7 +34,7 @@<br> > > > .\" From: @(#)tcp.4 8.1 (Berkeley) 6/5/93<br> > > > .\" $FreeBSD$<br> > > > .\"<br> > > > -.Dd June 27, 2021<br> > > > +.Dd January 8, 2022<br> > > > .Dt TCP 4<br> > > > .Os<br> > > > .Sh NAME<br> > > > @@ -339,6 +339,10 @@ This entry can only be specified on a per-host<br> > > basis at this time.<br> > > > .Pp<br> > > > If an SADB entry cannot be found for the destination,<br> > > > the system does not send any outgoing segments and drops any inbound<br> > > segments.<br> > > > +However, during connection negotiation, a non-signed segment will be<br> > > accepted if<br> > > > +an SADB entry does not exist between hosts.<br> > > > +When a non-signed segment is accepted, the established connection is n=<br> > ot<br> > > > +protected with MD5 digests.<br> > > > .It Dv TCP_STATS<br> > > > Manage collection of connection level statistics using the<br> > > > .Xr stats 3<br> > > > diff --git a/sys/netinet/tcp_syncache.c b/sys/netinet/tcp_syncache.c<br> > > > index 7dd8443cad65..32ca3bc2209b 100644<br> > > > --- a/sys/netinet/tcp_syncache.c<br> > > > +++ b/sys/netinet/tcp_syncache.c<br> > > > @@ -1514,19 +1514,25 @@ syncache_add(struct in_conninfo *inc, struct<br> > > tcpopt *to, struct tcphdr *th,<br> > > ><br> > > > #if defined(IPSEC_SUPPORT) || defined(TCP_SIGNATURE)<br> > > > /*<br> > > > - * If listening socket requested TCP digests, check that received<br> > > > - * SYN has signature and it is correct. If signature doesn't matc=<br> > h<br> > > > - * or TCP_SIGNATURE support isn't enabled, drop the packet.<br> > > > + * When the socket is TCP-MD5 enabled check that,<br> > > > + * - a signed packet is valid<br> > > > + * - a non-signed packet does not have a security association<br> > > > + *<br> > > > + * If a signed packet fails validation or a non-signed packet ha=<br> > s<br> > > a<br> > > > + * security association, the packet will be dropped.<br> > > > */<br> > > > if (ltflags & TF_SIGNATURE) {<br> > > > - if ((to->to_flags & TOF_SIGNATURE) =3D=3D 0) {<br> > > > - TCPSTAT_INC(tcps_sig_err_nosigopt);<br> > > > - goto done;<br> > > > + if (to->to_flags & TOF_SIGNATURE) {<br> > > > + if (!TCPMD5_ENABLED() ||<br> > > > + TCPMD5_INPUT(m, th, to->to_signature) !=3D 0)<br> > > > + goto done;<br> > > > + } else {<br> > > > + if (TCPMD5_ENABLED() &&<br> > > > + TCPMD5_INPUT(m, NULL, NULL) !=3D ENOENT)<br> > > > + goto done;<br> > > > }<br> > > > - if (!TCPMD5_ENABLED() ||<br> > > > - TCPMD5_INPUT(m, th, to->to_signature) !=3D 0)<br> > > > - goto done;<br> > > > - }<br> > > > + } else if (to->to_flags & TOF_SIGNATURE)<br> > > > + goto done;<br> > > > #endif /* TCP_SIGNATURE */<br> > > > /*<br> > > > * See if we already have an entry for this connection.<br> > > > @@ -1724,11 +1730,11 @@ skip_alloc:<br> > > > }<br> > > > #if defined(IPSEC_SUPPORT) || defined(TCP_SIGNATURE)<br> > > > /*<br> > > > - * If listening socket requested TCP digests, flag this in the<br> > > > + * If incoming packet has an MD5 signature, flag this in the<br> > > > * syncache so that syncache_respond() will do the right thing<br> > > > * with the SYN+ACK.<br> > > > */<br> > > > - if (ltflags & TF_SIGNATURE)<br> > > > + if (to->to_flags & TOF_SIGNATURE)<br> > > > sc->sc_flags |=3D SCF_SIGNATURE;<br> > > > #endif /* TCP_SIGNATURE */<br> > > > if (to->to_flags & TOF_SACKPERM)<br> > > > diff --git a/sys/netipsec/xform_tcp.c b/sys/netipsec/xform_tcp.c<br> > > > index b53544cd00fb..ce2552f0a205 100644<br> > > > --- a/sys/netipsec/xform_tcp.c<br> > > > +++ b/sys/netipsec/xform_tcp.c<br> > > > @@ -269,6 +269,11 @@ tcp_ipsec_input(struct mbuf *m, struct tcphdr *th,<br> > > u_char *buf)<br> > > > KMOD_TCPSTAT_INC(tcps_sig_err_buildsig);<br> > > > return (ENOENT);<br> > > > }<br> > > > + if (buf =3D=3D NULL) {<br> > > > + key_freesav(&sav);<br> > > > + KMOD_TCPSTAT_INC(tcps_sig_err_nosigopt);<br> > > > + return (EACCES);<br> > > > + }<br> > > > /*<br> > > > * tcp_input() operates with TCP header fields in host<br> > > > * byte order. We expect them in network byte order.<br> > ><br> </blockquote></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAF3%2Bn_f8Uh_ttjWQS11dLoMtA_-Mxaon6hLRkq-3V_%2B5h9kFgw>