netdev - Re: Flaw in "random32: update the net random state on interrupt and activity"

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] [day] [month] [year] [list]

Message-ID: <CANn89iJ-TC43k58BR63hQ-MEy1Civ=T5VWk50pBb+9QG9Kyy7A@mail.gmail.com>
Date:   Tue, 11 Aug 2020 09:01:58 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     George Spelvin <lkml@....org>, Willy Tarreau <w@....eu>,
        Netdev <netdev@...r.kernel.org>,
        Amit Klein <aksecurity@...il.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Andrew Lutomirski <luto@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        "Theodore Ts'o" <tytso@....edu>,
        Marc Plumb <lkml.mplumb@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: Flaw in "random32: update the net random state on interrupt and activity"

On Sat, Aug 8, 2020 at 7:07 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Sat, Aug 8, 2020 at 3:28 PM George Spelvin <lkml@....org> wrote:
> >
> > It's not a theoretical hole, it's a very real one.  Other than the cycles
> > to do the brute-force part, it's not even all that complicated.  The
> > theory part is that it's impossible to patch.
>
> We'll just disagree.
>
> I'm really fed up with security holes that are brought on by crypto
> people not being willing to just do reasonable things.
>
> > *If* you do the stupid thing.  WHICH YOU COULD JUST STOP DOING.
>
> We're not feeding all the same bits to the /dev/random that we're
> using to also update the pseudo-random state, so I think you're
> overreacting. Think of it as "/dev/random gets a few bits, and prandom
> gets a few other bits".
>
> The fact that there is overlap between the bits is immaterial, when
> other parts are disjoint. Fractonal bits of entropy and all that.
>
> > The explain it to me.  What is that actual *problem*?  Nobody's described
> > one, so I've been guessing.  What is this *monumentally stupid* abuse of
> > /dev/random allegedly fixing?
>
> The problem is that the networking people really want unguessable
> random state. There was a remote DNS spoofing poisoning attack because
> the UDP ports ended up being guessable.
>
> And that was actually reported to us back in early March.
>

Another typical use of prandom_u32() is the one in
tcp_conn_request(), when processing a SYN packet.

My fear was that adding much more cpu cycles to prandom_u32() would
reduce our ability to cope with a SYN flood attack,
but looking more closely to tcp_conn_request(), there might be a way
to remove the prandom_u32() call
when we generate a syncookie, reflecting incoming skb hash (if already
populated)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 184ea556f50e35141a4be5940c692db41e09f464..fc698a8ea13b1b6a6bd952308d11414eadfa4eaf
100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6740,10 +6740,12 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
                isn = cookie_init_sequence(af_ops, sk, skb, &req->mss);
                if (!tmp_opt.tstamp_ok)
                        inet_rsk(req)->ecn_ok = 0;
+               tcp_rsk(req)->txhash = skb->hash ?: 1;
+       } else {
+               tcp_rsk(req)->txhash = net_tx_rndhash();
        }

        tcp_rsk(req)->snt_isn = isn;
-       tcp_rsk(req)->txhash = net_tx_rndhash();
        tcp_openreq_init_rwin(req, sk, dst);
        sk_rx_queue_set(req_to_sk(req), skb);
        if (!want_cookie) {


BTW we could add a trace event so that the answer to 'who is using
prandom_u32' could be easily answered.

diff --git a/include/trace/events/random.h b/include/trace/events/random.h
index 32c10a515e2d5438e8d620a0c2313aab5f849b2b..9570a10cb949b5792c4290ba8e82a077ac655069
100644
--- a/include/trace/events/random.h
+++ b/include/trace/events/random.h
@@ -307,6 +307,23 @@ TRACE_EVENT(urandom_read,
                  __entry->pool_left, __entry->input_left)
 );

+TRACE_EVENT(prandom_u32,
+
+       TP_PROTO(unsigned int ret),
+
+       TP_ARGS(ret),
+
+       TP_STRUCT__entry(
+               __field(   unsigned int, ret)
+       ),
+
+       TP_fast_assign(
+               __entry->ret = ret;
+       ),
+
+       TP_printk("ret=%u" , __entry->ret)
+);
+
 #endif /* _TRACE_RANDOM_H */

 /* This part must be outside protection */
diff --git a/lib/random32.c b/lib/random32.c
index 3d749abb9e80d54d8e330e07fb8b773b7bec2b83..932345323af092a93fc2690b0ebbf4f7485ae4f3
100644
--- a/lib/random32.c
+++ b/lib/random32.c
@@ -39,6 +39,7 @@
 #include <linux/random.h>
 #include <linux/sched.h>
 #include <asm/unaligned.h>
+#include <trace/events/random.h>

 #ifdef CONFIG_RANDOM32_SELFTEST
 static void __init prandom_state_selftest(void);
@@ -82,6 +83,7 @@ u32 prandom_u32(void)
        u32 res;

        res = prandom_u32_state(state);
+       trace_prandom_u32(res);
        put_cpu_var(net_rand_state);

        return res;