Re: [Buildroot] [PATCH] package/urandom-scripts: hash old seed with new seed when saving

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Jason, All,

On 2022-03-22 21:52 -0600, Jason A. Donenfeld spake thusly:
> Writing into /dev/urandom doesn't actually credit any entropy bits. And
> while it adds that data to the entropy pool, it won't actually be
> immediately used when reading from /dev/urandom subsequently. This is
> how the kernel's /dev/urandom has always worked, unfortunately.

Thanks for this patch. I will blindly trust whatever you state about the
kernel RNG.
    https://www.zx2c4.com/projects/linux-rng-5.17-5.18/

Still, I have a few comments about this change, see below...

> As a result of this behavior, which may be understandably surprising to
> you, writing a good seed file into /dev/urandom and then saving a new

s/to you//

(no "personal" address in a commit message)

> seed file immediately after is dangerous, because the new seed file may
> wind up being entirely deterministic, even if the old seed file was
> quite good.
> 
> I fixed this in systemd with

s/I fixed this/This has been fixed/

(but "we" as you used later is ok)

[--SNIP--]
> As a final note, while this commit improves upon the status quo by
> removing a vulnerability, this shell script still does not actually
> initialize the RNG like it says it does. For initialization via a seed
> file, the RNDADDENTROPY ioctl must be used.

Is there a way to do that within a shell script? If so, would you be
kind enough to send a followup patch, please?

> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  package/urandom-scripts/S20urandom | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/package/urandom-scripts/S20urandom b/package/urandom-scripts/S20urandom
> index e4fd125721..f971ea905a 100644
> --- a/package/urandom-scripts/S20urandom
> +++ b/package/urandom-scripts/S20urandom
[--SNIP--]
> @@ -44,7 +34,9 @@ save_random_seed() {
>  	if touch "$URANDOM_SEED" 2> /dev/null; then
>  		old_umask=$(umask)
>  		umask 077
> -		dd if=/dev/urandom of="$URANDOM_SEED" bs="$pool_size" count=1 2> /dev/null
> +		dd if=/dev/urandom of="$URANDOM_SEED.new" bs="$pool_size" count=1 2> /dev/null
> +		cat "$URANDOM_SEED" "$URANDOM_SEED.new" 2>/dev/null | sha256sum | cut -d ' ' -f 1 > "$URANDOM_SEED"

    $ shellcheck package/urandom-scripts/S20urandom
    In package/urandom-scripts/S20urandom line 38:
                cat "$URANDOM_SEED" "$URANDOM_SEED.new" 2>/dev/null | sha256sum | cut -d ' ' -f 1 > "$URANDOM_SEED"
                    ^-------------^ SC2094: Make sure not to read and write the same file in the same pipeline.

> +		rm "$URANDOM_SEED.new"
>  		status=$?

This status now only gets the result of the 'rm', which is not very
interesting. I guess what we really need is whether the initial 'dd'
that extracts from the pool succeeded. If that fails, then we should
probably not update the saved seed, should we?

Here's what I suggest instead:

    save_random_seed() {
        printf 'Saving random seed: '
        status=1
        if touch "$URANDOM_SEED" 2> /dev/null; then
            old_umask=$(umask)
            umask 077
            dd if=/dev/urandom of="$URANDOM_SEED.tmp" bs="$pool_size" count=1 2> /dev/null
            new_seed_len="$(wc -c "$URANDOM_SEED.tmp" 2> /dev/null |cut -d ' ' -f 1)"
            if [ "$new_seed_len" -eq "$pool_size" ]; then
                cat "$URANDOM_SEED" "$URANDOM_SEED.tmp" 2>/dev/null \
                | sha256sum \
                | cut -d ' ' -f 1 > "$URANDOM_SEED.new"
                mv "$URANDOM_SEED.new" "$URANDOM_SEED"
                echo "OK"
                status=0
            else
                echo "FAIL"
            fi
            rm -f "$URANDOM_SEED.tmp"
            umask "$old_umask"
        else
            echo "SKIP (read-only file system detected)"
        fi
        return "$status"
    }

What do you think about that? Since this is a bit critical, I did not
want to just do that change when applying.

I do note that urandom should always return something, but this script
will also need to work on older kernels, sometimes way back to oldish
3.x series, and my recollection of how urandom worked back then is a bit
fuzzy. Are we sure it will always have returned enough? If so, then we
can ditch the sanity check altogether.

But then, if we failed to read anything from urandom, we'd just hash the
old seed, which should not decrease the entropy it had, as the hash has
the same size as the seed.

Thoughts?

Regards,
Yann E. MORIN.

>  		umask "$old_umask"
>  		if [ "$status" -eq 0 ]; then
> -- 
> 2.35.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot