Re: git: eb18708ec8c7 - main - syncache: accept packet with no SA when TCP_MD5SIG is set
Date: Sat, 15 Jan 2022 13:43:41 UTC
On 12 Jan 2022, at 22:26, Mike Karels wrote:
> 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.
I would have hoped they’d all validate peer address to look up a
config snippet
and not find that peer then and close the socket..
>
>>> 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.
True. But for a while a single listen socket could only get either
all-encrypted or non-encrypted which in most cases probably never
worked.
And that never was the original behaviour in FreeBSD.
So the actual “change in behaviour” was introduced a while ago and
now been restored again.
I believe after accept a routing daemon could check the socket option
being on the accepted connection to see what actually happened? (I have
not checked if that actually works currently). I would agree to say
that the flag is a bit convoluted.
>>> 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.
Yes, but again matches the previous-previous case which was there from
the beginning.
>> 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.
>>>