From nobody Wed Jan 12 22:26:58 2022 X-Original-To: dev-commits-src-all@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 0485F196097E; Wed, 12 Jan 2022 22:27:01 +0000 (UTC) (envelope-from mike@mail.karels.net) Received: from mail.karels.net (mail.karels.net [216.160.39.52]) by mx1.freebsd.org (Postfix) with ESMTP id 4JZ2H761vlz3Gtf; Wed, 12 Jan 2022 22:26:59 +0000 (UTC) (envelope-from mike@mail.karels.net) Received: from mail.karels.net (localhost [127.0.0.1]) by mail.karels.net (8.16.1/8.16.1) with ESMTPS id 20CMQwlW007751 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Wed, 12 Jan 2022 16:26:58 -0600 (CST) (envelope-from mike@mail.karels.net) Received: (from mike@localhost) by mail.karels.net (8.16.1/8.16.1/Submit) id 20CMQwMw007750; Wed, 12 Jan 2022 16:26:58 -0600 (CST) (envelope-from mike) Message-Id: <202201122226.20CMQwMw007750@mail.karels.net> To: Rob Wing cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org From: Mike Karels Reply-to: mike@karels.net Subject: Re: git: eb18708ec8c7 - main - syncache: accept packet with no SA when TCP_MD5SIG is set In-reply-to: Your message of Wed, 12 Jan 2022 08:35:50 -0900. List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <7748.1642026418.1@mail.karels.net> Date: Wed, 12 Jan 2022 16:26:58 -0600 X-Rspamd-Queue-Id: 4JZ2H761vlz3Gtf X-Spamd-Bar: ++ Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=none (mx1.freebsd.org: domain of mike@mail.karels.net has no SPF policy when checking 216.160.39.52) smtp.mailfrom=mike@mail.karels.net X-Spamd-Result: default: False [2.30 / 15.00]; HAS_REPLYTO(0.00)[mike@karels.net]; 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]; REPLYTO_ADDR_EQ_FROM(0.00)[]; DMARC_NA(0.00)[karels.net]; AUTH_NA(1.00)[]; NEURAL_SPAM_SHORT(1.00)[1.000]; MID_RHS_MATCH_FROMTLD(0.00)[]; NEURAL_SPAM_LONG(1.00)[1.000]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; R_SPF_NA(0.00)[no SPF record]; FORGED_SENDER(0.30)[mike@karels.net,mike@mail.karels.net]; RCVD_NO_TLS_LAST(0.10)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:209, ipnet:216.160.36.0/22, country:US]; FROM_NEQ_ENVFROM(0.00)[mike@karels.net,mike@mail.karels.net]; RCVD_COUNT_TWO(0.00)[2] X-ThisMailContainsUnwantedMimeParts: N Rob wrote: > On Wed, Jan 12, 2022 at 6:58 AM Mike Karels 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 > > > 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 > > 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. > >