Re: git: eb18708ec8c7 - main - syncache: accept packet with no SA when TCP_MD5SIG is set
Date: Wed, 12 Jan 2022 22:26:58 UTC
Rob wrote:
> 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.
Yes, but if you haven't configured the SA, the result is different, and
a routing daemon could receive a connection that wasn't from a known peer.
> > Redefining the option at this late date seems unsafe and unwise.
> The option hasn't been redefined.
It has, in the sense that connections from unknown peers without MD5
signatures are now permitted. A daemon relying on the previous behavior
wouldn't bother checking whether MD5 was enabled on incoming connections.
> > 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.
Which is different than the previous use case, where a daemon could be
sure that incoming connections were from a peer in the SADB and used MD5.
> 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.
That's a useful data point, thanks.
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.
> >