Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220322191436.110963-1-Jason@zx2c4.com> <1648009787.fah6dos6ya.none@localhost>
In-Reply-To: <1648009787.fah6dos6ya.none@localhost>
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
Date:   Tue, 22 Mar 2022 22:47:53 -0600
Message-ID: <CAHmME9rsvxczJrhPwRX6nyrh9NB2AuJqkEKrTLx-G-T1J6_czQ@mail.gmail.com>
Subject: Re: [PATCH] random: allow writes to /dev/urandom to influence fast init
To:     "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca>
Cc:     Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Jann Horn <jannh@google.com>,
        Dominik Brodowski <linux@dominikbrodowski.net>,
        Guenter Roeck <linux@roeck-us.net>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        "Theodore Ts'o" <tytso@mit.edu>,
        Sandy Harris <sandyinchina@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hey Alex,

Thanks a bunch for doing that research.

On Tue, Mar 22, 2022 at 10:30 PM Alex Xu (Hello71) <alex_y_xu@yahoo.ca> wrote:
> Several programs use it for testing purposes, without writing any
> entropy to /dev/random or /dev/urandom, including rauc, wireguard

The WireGuard use case is sitting in my tree but unpushed, and indeed
it's done as a hack. I have no qualms about changing that before
pushing to net or net-next.

> - kata-containers is a lightweight VM implementation. Its guest-side
>   agent offers a gRPC endpoint which will write the provided data to
>   /dev/random, then call RNDADDTOENTCNT with the length of the data,
>   then call RNDRESEEDRNG.

Sounds like this usage is safe, and that this patch wouldn't really
fix much there.

> - aws-nitro-enclaves-sdk-c is an SDK for building lightweight VMs to be
>   used with AWS Nitro Enclaves. kmstool-enclave is a sample application
>   provided, which writes "up to 256 bytes" (from where?) to /dev/random,
>   then calls RNDADDTOENTCNT, then repeats the process until it reaches
>   1024 bytes.

Looks like another safe use case, which the patch here doesn't help.
Actually this patch might _hurt_ that use case if some of those writes
are short (less than 7 bytes or so). So that might be a data point
that indicates we shouldn't merge this patch, and instead should go
with the "do nothing" route.

> - sandy-harris/maxwell is a "jitter entropy" daemon, similar to haveged.
>   It writes 4 bytes of "generated entropy" to /dev/random, then calls
>   RNDADDTOENTCNT, then repeats.

Okay bingo. The existence of this means that this patch will
definitely introduce a new vulnerability. It means that an attacker
can brute force all of Sandy's (CC'd now) inputs 32 bits at a time. So
I don't think I can merge this patch as-is.

A potential fix for this would be to change:

+               if (unlikely(crng_init == 0 && !will_credit))
+                       crng_pre_init_inject(block, len, false);

into

+               if (unlikely(crng_init == 0 && !will_credit && count
>= 16 && capable(CAP_SYS_ADMIN)))
+                       crng_pre_init_inject(block, len, false);

Or something like that. But that doesn't account for the case where
what's written to /dev/urandom is 300 bytes long but only has 3 bits
of unknown data. So it's better as far as heuristics go, but it
doesn't definitely solve the problem, which stems from the fact of
separating entropy writing from entropy crediting into separate calls.
(Plus as I already mentioned to David, the patch still wouldn't help
the crng_init=1 case.)

> - guix is, among other things, a "GNU/"Linux distribution. The provided
>   base services write the seed file to /dev/urandom, then call
>   RNDADDTOENTCNT, then write 512 bytes from /dev/hwrng to /dev/urandom,
>   then call RNDADDTOENTCNT, then "immediately" read 512 bytes from
>   /dev/urandom and write it to the seed file. On shutdown, 512 bytes are
>   read from /dev/urandom and written to the seed file.

That also sounds like a safe usage of RNDADDTOENTCNT.

> I don't have any particular expertise with the random subsystem or
> conclusions to make from this data, but I hope this helps inform the
> discussion.

Very much so, thanks again. What I take away from your results is:

- RNDADDTOENTCNT is in active use in a safe way. Sure, RNDADDENTROPY
is still much better, but RNDADDTOENTCNT isn't entirely broken in the
above configurations either.
- This patch would make RNDADDTOENTCNT unsafe for some of the above
configurations in a way that it currently isn't unsafe.
- Plenty of things are seeding the RNG correctly, and buildroot's
shell script is just "doing it wrong".

On that last point, I should reiterate that buildroot's shell script
still isn't actually initializing the RNG, despite what it says in its
echo; there's never been a way to initialize the RNG from a shell
script, without calling out to various special purpose ioctl-aware
binaries.

Jason