Re: git: eb18708ec8c7 - main - syncache: accept packet with no SA when TCP_MD5SIG is set

Sorry for the delayed response; I’ve been out.

This change seems wrong to me.  The TCP_MD5SIG option clearly required MD5
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 using
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 known
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=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.