netdev - Re: [PATCH net] tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iLvSSLRzRr0vPDNBfe_ukGecQZp4AN4ZODYjpebq643NQ@mail.gmail.com>
Date:   Tue, 30 Jun 2020 17:34:43 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net] tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key()

On Tue, Jun 30, 2020 at 5:27 PM Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
>
> ----- On Jun 30, 2020, at 7:50 PM, Eric Dumazet edumazet@...gle.com wrote:
>
> > On Tue, Jun 30, 2020 at 4:47 PM Mathieu Desnoyers
> > <mathieu.desnoyers@...icios.com> wrote:
> >>
> >> ----- On Jun 30, 2020, at 7:41 PM, Eric Dumazet edumazet@...gle.com wrote:
> >>
> >> > MD5 keys are read with RCU protection, and tcp_md5_do_add()
> >> > might update in-place a prior key.
> >> >
> >> > Normally, typical RCU updates would allocate a new piece
> >> > of memory. In this case only key->key and key->keylen might
> >> > be updated, and we do not care if an incoming packet could
> >> > see the old key, the new one, or some intermediate value,
> >> > since changing the key on a live flow is known to be problematic
> >> > anyway.
> >>
> >> What makes it acceptable to observe an intermediate bogus key during the
> >> transition ?
> >
> > If you change a key while packets are in flight, the result is that :
> >
> > 1) Either your packet has the correct key and is handled
> >
> > 2) Or the key do not match, packet is dropped.
> >
> > Sender will retransmit eventually.
>
> This train of thoughts seem to apply to incoming traffic, what about outgoing ?


Outgoing path is protected by the socket lock.

You can not change the TCP MD5 key while xmit is in progress.

>
> >
> > If this was not the case, then we could not revert the patch you are
> > complaining about :)
>
> Please let me know where I'm incorrect with the following scenario:
>
> - Local peer is a Linux host, which supports only a single MD5 key
>   per socket at any given time.
> - Remote peer is a Ericsson/Redback device allowing 2 passwords (old/new)
>   to co-exist until both sides are OK with the new password.
>
> The local peer updates the MD5 password for a socket in parallel with
> packets flow. If we guarantee that no intermediate bogus key state is
> observable, the flow going out of the Linux peer should always be seen as
> valid, with either the old or the new key.

Linux has one key.

If your peer sends a packet with the old key, you will drop the
packet, no matter how hard you try.


>
> Allowing an intermediate bogus key to be observable means this introduces
> a race condition during which the remote peer can observe a corrupted key.

Race condition is there, even if you change the whole TCP MD5 key atomically.

If another cpu receives a packet while you are changing the key, you
definitely can not predict
if the packet is going to be dropped or not.


>
> Thanks,
>
> Mathieu
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com