Date: Wed, 12 Jan 2022 08:35:50 -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_eOVx=WwVqO4rHja_3r3sqmj94theYRgqmQaMxU9X5=JQ@mail.gmail.com> In-Reply-To: <1C53233E-B706-4AE8-9928-5AEC09DBD25A@karels.net> References: <202201090145.2091j96M028719@gitrepo.freebsd.org> <1C53233E-B706-4AE8-9928-5AEC09DBD25A@karels.net>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] On Wed, Jan 12, 2022 at 6:58 AM Mike Karels <mike@karels.net> wrote: > Sorry for the delayed response; I’ve been out. > No worries. This change seems wrong to me. The TCP_MD5SIG option clearly required MD5 > signatures on all connections in the past, for many years, You're right, since 2017. Before that, TCP_MD5SIG did not require MD5 signature when establishing a connection either. > That meant that connections were limited to peers in the Security > Association Database. A program like a routing daemon could then be sure that it was talking to a > known peer using > signed packets This is still the case, if you've configured a peer to use MD5, it will use MD5. > Redefining the option at this late date seems unsafe and unwise. The option hasn't been redefined. > If there is a use case for a socket that requires MD5SIG for known > peers and not for others, it seems to me that it would be better to add a > new option with those semantics. > The use case is a bgp daemon socket that wants to handle/establish connections with MD5 and non-MD5 peers. For peers configured with MD5, the established connection will be protected by MD5 signatures (i.e., the TCP_MD5SIG option will be set). If a peer is not configured for MD5, then the established connection will not be protected with MD5 signatures (i.e., TCP_MD5SIG will not be set). For what it's worth, this behavior is consistent with OpenBSD. -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=eb18708ec8c7e1de6a05aba41971659549991b10 > > > > 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-signed > > 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 not > > +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 match > > - * 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 has > a > > + * security association, the packet will be dropped. > > */ > > if (ltflags & TF_SIGNATURE) { > > - if ((to->to_flags & TOF_SIGNATURE) == 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) != 0) > > + goto done; > > + } else { > > + if (TCPMD5_ENABLED() && > > + TCPMD5_INPUT(m, NULL, NULL) != ENOENT) > > + goto done; > > } > > - if (!TCPMD5_ENABLED() || > > - TCPMD5_INPUT(m, th, to->to_signature) != 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 |= 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 == 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 6:58 AM Mike Karels <<a href="mailto:mike@karels.net" target="_blank">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">Sorry for the delayed response; I’ve been out.<br></blockquote><div><br></div><div>No worries.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> This change seems wrong to me. The TCP_MD5SIG option clearly required MD5<br> signatures on all connections in the past, for many years, </blockquote><div><br></div><div>You're right, since 2017. Before that, TCP_MD5SIG did not require MD5 signature when establishing a connection either.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> That meant that connections were limited to peers in the Security Association Database.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">A program like a routing daemon could then be sure that it was talking to a known peer using<br> signed packets </blockquote><div><br></div><div>This is still the case, if you've configured a peer to use MD5, it will use MD5.<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">Redefining the option at this late date seems unsafe and unwise.</blockquote><div><br></div><div>The option hasn't been redefined.<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">If there is a use case for a socket that requires MD5SIG for known<br> peers and not for others, it seems to me that it would be better to add a<br> new option with those semantics.<br></blockquote><div><br></div><div>The use case is a bgp daemon socket that wants to handle/establish connections with MD5 and non-MD5 peers.<br></div><div><br></div><div>For peers configured with MD5, the established connection will be protected by MD5 signatures (i.e., the TCP_MD5SIG option will be set). If a peer is not configured for MD5, then the established connection will not be protected with MD5 signatures (i.e., TCP_MD5SIG will not be set).<br></div><div><br></div><div>For what it's worth, this behavior is consistent with OpenBSD.<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> On 8 Jan 2022, at 19:45, Robert Wing wrote:<br> <br> > The branch main has been updated by rew:<br> ><br> > URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=eb18708ec8c7e1de6a05aba41971659549991b10" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=eb18708ec8c7e1de6a05aba41971659549991b10</a><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 don't<br> > contain an MD5 signature. Relax this behavior to accept a non-signed<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 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 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 segments.<br> > +However, during connection negotiation, a non-signed segment will be accepted if<br> > +an SADB entry does not exist between hosts.<br> > +When a non-signed segment is accepted, the established connection is not<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 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 match<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 has a<br> > + * security association, the packet will be dropped.<br> > */<br> > if (ltflags & TF_SIGNATURE) {<br> > - if ((to->to_flags & TOF_SIGNATURE) == 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) != 0)<br> > + goto done;<br> > + } else {<br> > + if (TCPMD5_ENABLED() &&<br> > + TCPMD5_INPUT(m, NULL, NULL) != ENOENT)<br> > + goto done;<br> > }<br> > - if (!TCPMD5_ENABLED() ||<br> > - TCPMD5_INPUT(m, th, to->to_signature) != 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 |= 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, u_char *buf)<br> > KMOD_TCPSTAT_INC(tcps_sig_err_buildsig);<br> > return (ENOENT);<br> > }<br> > + if (buf == 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> </blockquote></div></div>help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAF3%2Bn_eOVx=WwVqO4rHja_3r3sqmj94theYRgqmQaMxU9X5=JQ>