From nobody Wed Jan 12 15:58:18 2022 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 6462C1962801; Wed, 12 Jan 2022 15:58:27 +0000 (UTC) (envelope-from mike@karels.net) Received: from mail.karels.net (mail.karels.net [216.160.39.52]) by mx1.freebsd.org (Postfix) with ESMTP id 4JYsfp1TDVz3pZm; Wed, 12 Jan 2022 15:58:26 +0000 (UTC) (envelope-from mike@karels.net) Received: from mail.karels.net (localhost [127.0.0.1]) by mail.karels.net (8.16.1/8.16.1) with ESMTP id 20CFwJA9006224; Wed, 12 Jan 2022 09:58:19 -0600 (CST) (envelope-from mike@karels.net) Received: from [10.0.2.130] ([10.0.1.1]) by mail.karels.net with ESMTPSA id SaYnCJv63mFOGAAA4+wvSQ (envelope-from ); Wed, 12 Jan 2022 09:58:19 -0600 From: Mike Karels To: Robert Wing 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 Date: Wed, 12 Jan 2022 09:58:18 -0600 X-Mailer: MailMate (1.14r5818) Message-ID: <1C53233E-B706-4AE8-9928-5AEC09DBD25A@karels.net> In-Reply-To: <202201090145.2091j96M028719@gitrepo.freebsd.org> References: <202201090145.2091j96M028719@gitrepo.freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4JYsfp1TDVz3pZm X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of mike@karels.net designates 216.160.39.52 as permitted sender) smtp.mailfrom=mike@karels.net X-Spamd-Result: default: False [-3.10 / 15.00]; RCVD_VIA_SMTP_AUTH(0.00)[]; ARC_NA(0.00)[]; FREEFALL_USER(0.00)[mike]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; R_SPF_ALLOW(-0.20)[+ip4:216.160.39.52]; DMARC_NA(0.00)[karels.net]; NEURAL_HAM_LONG(-0.99)[-0.994]; RCVD_COUNT_THREE(0.00)[3]; NEURAL_HAM_MEDIUM(-0.91)[-0.911]; NEURAL_HAM_SHORT(-1.00)[-1.000]; RCVD_NO_TLS_LAST(0.10)[]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:209, ipnet:216.160.36.0/22, country:US]; MID_RHS_MATCH_FROM(0.00)[] X-ThisMailContainsUnwantedMimeParts: N Sorry for the delayed response; I=E2=80=99ve been out. This change seems wrong to me. The TCP_MD5SIG option clearly required MD= 5 signatures on all connections in the past, for many years, and this was clearly documented in the man page. 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 usi= ng signed packets. Redefining the option at this late date seems unsafe and= unwise. If there is a use case for a socket that requires MD5SIG for kno= wn peers and not for others, it seems to me that it would be better to add a= new option with those semantics. 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=3Deb18708ec8c7e1de6a05aba4= 1971659549991b10 > > commit eb18708ec8c7e1de6a05aba41971659549991b10 > Author: Robert Wing > AuthorDate: 2022-01-09 01:07:50 +0000 > Commit: Robert Wing > 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 do= n'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 ha= ndle > 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 bas= is 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 s= egments. > +However, during connection negotiation, a non-signed segment will be a= ccepted 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 tc= popt *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) =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.