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

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

[Jason A. Donenfeld]

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.

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
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
<https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
and fortunately it's possible to do the same thing in shell script here.
Specifically, instead of just saving new /dev/urandom output straight
up, we hash the new /dev/urandom together with the old seed, in order to
produce the new seed. This way the amount of entropy in the new seed
will stay the same or get better, but not appreciably regress.

At the same time, the pool size check in this script is useless. Writing
to /dev/urandom never credits bits anyway, so no matter what, writing
into /dev/urandom is useful and not harmful. There's also not much of a
point in seeding with more than 256 bits, which is what the hashing
operation above produces. So this commit removes the file size check.

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.

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
@@ -17,26 +17,16 @@ else
 	pool_size=512
 fi
 
-check_file_size() {
-	[ -f "$URANDOM_SEED" ] || return 1
-	# Try to read two blocks but exactly one will be read if the file has
-	# the correct size.
-	size=$(dd if="$URANDOM_SEED" bs="$pool_size" count=2 2> /dev/null | wc -c)
-	test "$size" -eq "$pool_size"
-}
-
 init_rng() {
-	if check_file_size; then
-		printf 'Initializing random number generator: '
-		dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
-		status=$?
-		if [ "$status" -eq 0 ]; then
-			echo "OK"
-		else
-			echo "FAIL"
-		fi
-		return "$status"
+	printf 'Initializing random number generator: '
+	dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
 	fi
+	return "$status"
 }
 
 save_random_seed() {
@@ -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"
+		rm "$URANDOM_SEED.new"
 		status=$?
 		umask "$old_umask"
 		if [ "$status" -eq 0 ]; then
-- 
2.35.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Jason A. Donenfeld]

Hi Christoph, Yann,

I realized I should have CC'd you on the below. Sorry about that.

Jason

On Tue, Mar 22, 2022 at 9:52 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> 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.
>
> 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
> 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
> <https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
> and fortunately it's possible to do the same thing in shell script here.
> Specifically, instead of just saving new /dev/urandom output straight
> up, we hash the new /dev/urandom together with the old seed, in order to
> produce the new seed. This way the amount of entropy in the new seed
> will stay the same or get better, but not appreciably regress.
>
> At the same time, the pool size check in this script is useless. Writing
> to /dev/urandom never credits bits anyway, so no matter what, writing
> into /dev/urandom is useful and not harmful. There's also not much of a
> point in seeding with more than 256 bits, which is what the hashing
> operation above produces. So this commit removes the file size check.
>
> 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.
>
> 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
> @@ -17,26 +17,16 @@ else
>         pool_size=512
>  fi
>
> -check_file_size() {
> -       [ -f "$URANDOM_SEED" ] || return 1
> -       # Try to read two blocks but exactly one will be read if the file has
> -       # the correct size.
> -       size=$(dd if="$URANDOM_SEED" bs="$pool_size" count=2 2> /dev/null | wc -c)
> -       test "$size" -eq "$pool_size"
> -}
> -
>  init_rng() {
> -       if check_file_size; then
> -               printf 'Initializing random number generator: '
> -               dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
> -               status=$?
> -               if [ "$status" -eq 0 ]; then
> -                       echo "OK"
> -               else
> -                       echo "FAIL"
> -               fi
> -               return "$status"
> +       printf 'Initializing random number generator: '
> +       dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
> +       status=$?
> +       if [ "$status" -eq 0 ]; then
> +               echo "OK"
> +       else
> +               echo "FAIL"
>         fi
> +       return "$status"
>  }
>
>  save_random_seed() {
> @@ -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"
> +               rm "$URANDOM_SEED.new"
>                 status=$?
>                 umask "$old_umask"
>                 if [ "$status" -eq 0 ]; then
> --
> 2.35.1
>
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Nicolas Cavallari]

On 23/03/2022 04:52, Jason A. Donenfeld wrote:
> 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.
> 
> 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
> 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
> <https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
> and fortunately it's possible to do the same thing in shell script here.
> Specifically, instead of just saving new /dev/urandom output straight
> up, we hash the new /dev/urandom together with the old seed, in order to
> produce the new seed. This way the amount of entropy in the new seed
> will stay the same or get better, but not appreciably regress.
> 
> At the same time, the pool size check in this script is useless. Writing
> to /dev/urandom never credits bits anyway, so no matter what, writing
> into /dev/urandom is useful and not harmful. There's also not much of a
> point in seeding with more than 256 bits, which is what the hashing
> operation above produces. So this commit removes the file size check.
> 
> 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.
> 
> 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
> @@ -17,26 +17,16 @@ else
>   	pool_size=512
>   fi
>   
> -check_file_size() {
> -	[ -f "$URANDOM_SEED" ] || return 1
> -	# Try to read two blocks but exactly one will be read if the file has
> -	# the correct size.
> -	size=$(dd if="$URANDOM_SEED" bs="$pool_size" count=2 2> /dev/null | wc -c)
> -	test "$size" -eq "$pool_size"
> -}
> -
>   init_rng() {
> -	if check_file_size; then
> -		printf 'Initializing random number generator: '
> -		dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
> -		status=$?
> -		if [ "$status" -eq 0 ]; then
> -			echo "OK"
> -		else
> -			echo "FAIL"
> -		fi
> -		return "$status"
> +	printf 'Initializing random number generator: '
> +	dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null

This will fail if the seed does not exist.  Either because it's the 
first boot or because the system is read-only.

The [ -f ] test is still useful in the first case, because init_rng must 
return 0 for save_random_seed to be called during boot. And if 
save_random_seed is not called during boot the seed might never be 
created (you cannot expect stop to be called).

> +	status=$?
> +	if [ "$status" -eq 0 ]; then
> +		echo "OK"
> +	else
> +		echo "FAIL"
>   	fi
> +	return "$status"
>   }
>   
>   save_random_seed() {
> @@ -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"
> +		rm "$URANDOM_SEED.new"
 >   		status=$?

This breaks the error check.  Either these commands must be chained by 
&& or each of them should be tested.

Also, i'm not sure why stderr is supressed for cat.  In this case it is 
not supposed to fail or print anything to stderr.
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Yann E. MORIN]

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

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

[Nicolas Cavallari]

On 23/03/2022 10:13, Yann E. MORIN wrote:
>> 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?

No busybox applet is using RNDADDENTROPY, so it cannot probably be done 
in a shell script :(

It's also unfortunate that the urandom(4) man page's example script 
tells people to write to /dev/urandom exactly the way S20urandom does.
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Jason A. Donenfeld]

Hi Yann,

On Wed, Mar 23, 2022 at 3:13 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > 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)

Ack.

>
> > 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/

Ack.

> > 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?

No, there isn't, at all. But I'm writing some code now that I hope to
release as a standalone utility, and upstream into busybox/util-linux
that should make it accessible. When that happens, I'll keep this
mailing list informed, and we can move forward then.

> Here's what I suggest instead:

Your suggestion looks good, with a few small tweaks that I'll put in v2.

> 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.

Since this isn't crediting anything anyway, I don't think the sanity
check matters. The randomness might be total junk. It might not. We
might get enough. There might be a bug. We don't care, because there's
nothing we can do if it fails from shell, and we're not crediting, so
it's not a security risk so long as we don't regress.

v2 coming up.

Jason
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Jason A. Donenfeld]

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.

As a result of this behavior, which may be understandably surprising,
writing a good seed file into /dev/urandom and then saving a new 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.

This has been fixed in systemd with
<https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
and fortunately it's possible to do the same thing in shell script here.
Specifically, instead of just saving new /dev/urandom output straight
up, we hash the new /dev/urandom together with the old seed, in order to
produce the new seed. This way the amount of entropy in the new seed
will stay the same or get better, but not appreciably regress.

At the same time, the pool size check in this script is useless. Writing
to /dev/urandom never credits bits anyway, so no matter what, writing
into /dev/urandom is useful and not harmful. There's also not much of a
point in seeding with more than 256 bits, which is what the hashing
operation above produces. So this commit removes the file size check.

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.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 package/urandom-scripts/S20urandom | 39 +++++++++++++-----------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/package/urandom-scripts/S20urandom b/package/urandom-scripts/S20urandom
index e4fd125721..c6b2ebd48f 100644
--- a/package/urandom-scripts/S20urandom
+++ b/package/urandom-scripts/S20urandom
@@ -17,43 +17,38 @@ else
 	pool_size=512
 fi
 
-check_file_size() {
-	[ -f "$URANDOM_SEED" ] || return 1
-	# Try to read two blocks but exactly one will be read if the file has
-	# the correct size.
-	size=$(dd if="$URANDOM_SEED" bs="$pool_size" count=2 2> /dev/null | wc -c)
-	test "$size" -eq "$pool_size"
-}
-
 init_rng() {
-	if check_file_size; then
-		printf 'Initializing random number generator: '
-		dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
-		status=$?
-		if [ "$status" -eq 0 ]; then
-			echo "OK"
-		else
-			echo "FAIL"
-		fi
-		return "$status"
+	printf 'Initializing random number generator: '
+	dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
+	status=$?
+	if [ "$status" -eq 0 ]; then
+		echo "OK"
+	else
+		echo "FAIL"
 	fi
+	return "$status"
 }
 
 save_random_seed() {
 	printf 'Saving random seed: '
-	if touch "$URANDOM_SEED" 2> /dev/null; then
+	status=1
+	if touch "$URANDOM_SEED.new" 2> /dev/null; then
 		old_umask=$(umask)
 		umask 077
-		dd if=/dev/urandom of="$URANDOM_SEED" bs="$pool_size" count=1 2> /dev/null
-		status=$?
+		dd if=/dev/urandom of="$URANDOM_SEED.tmp" bs="$pool_size" count=1 2> /dev/null
+		cat "$URANDOM_SEED" "$URANDOM_SEED.tmp" 2>/dev/null \
+			| sha256sum \
+			| cut -d ' ' -f 1 > "$URANDOM_SEED.new" && \
+		mv "$URANDOM_SEED.new" "$URANDOM_SEED" && status=0
+		rm -f "$URANDOM_SEED.tmp"
 		umask "$old_umask"
 		if [ "$status" -eq 0 ]; then
 			echo "OK"
 		else
 			echo "FAIL"
 		fi
+
 	else
-		status=$?
 		echo "SKIP (read-only file system detected)"
 	fi
 	return "$status"
-- 
2.35.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Jason A. Donenfeld]

Hi Yann,

On Wed, Mar 23, 2022 at 2:06 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > 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?
>
> No, there isn't, at all. But I'm writing some code now that I hope to
> release as a standalone utility, and upstream into busybox/util-linux
> that should make it accessible. When that happens, I'll keep this
> mailing list informed, and we can move forward then.

Initial draft is here:

https://git.zx2c4.com/seedrng/about/
https://git.zx2c4.com/seedrng/tree/seedrng.c

I'll probably play with that for a few more days to make sure it's
solid. Probably the long term solution here is putting something like
that into busybox, and having scripts call that instead.

Jason
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Yann E. MORIN]

Jason, All,

+Peter: candidate for backporting as a security fix

On 2022-03-23 14:07 -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.
> 
> As a result of this behavior, which may be understandably surprising,
> writing a good seed file into /dev/urandom and then saving a new 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.
> 
> This has been fixed in systemd with
> <https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
> and fortunately it's possible to do the same thing in shell script here.
> Specifically, instead of just saving new /dev/urandom output straight
> up, we hash the new /dev/urandom together with the old seed, in order to
> produce the new seed. This way the amount of entropy in the new seed
> will stay the same or get better, but not appreciably regress.
> 
> At the same time, the pool size check in this script is useless. Writing
> to /dev/urandom never credits bits anyway, so no matter what, writing
> into /dev/urandom is useful and not harmful. There's also not much of a
> point in seeding with more than 256 bits, which is what the hashing
> operation above produces. So this commit removes the file size check.
> 
> 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.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Applied to master, thanks.

Regards,
Yann E. MORIN.

> ---
>  package/urandom-scripts/S20urandom | 39 +++++++++++++-----------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/package/urandom-scripts/S20urandom b/package/urandom-scripts/S20urandom
> index e4fd125721..c6b2ebd48f 100644
> --- a/package/urandom-scripts/S20urandom
> +++ b/package/urandom-scripts/S20urandom
> @@ -17,43 +17,38 @@ else
>  	pool_size=512
>  fi
>  
> -check_file_size() {
> -	[ -f "$URANDOM_SEED" ] || return 1
> -	# Try to read two blocks but exactly one will be read if the file has
> -	# the correct size.
> -	size=$(dd if="$URANDOM_SEED" bs="$pool_size" count=2 2> /dev/null | wc -c)
> -	test "$size" -eq "$pool_size"
> -}
> -
>  init_rng() {
> -	if check_file_size; then
> -		printf 'Initializing random number generator: '
> -		dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
> -		status=$?
> -		if [ "$status" -eq 0 ]; then
> -			echo "OK"
> -		else
> -			echo "FAIL"
> -		fi
> -		return "$status"
> +	printf 'Initializing random number generator: '
> +	dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
> +	status=$?
> +	if [ "$status" -eq 0 ]; then
> +		echo "OK"
> +	else
> +		echo "FAIL"
>  	fi
> +	return "$status"
>  }
>  
>  save_random_seed() {
>  	printf 'Saving random seed: '
> -	if touch "$URANDOM_SEED" 2> /dev/null; then
> +	status=1
> +	if touch "$URANDOM_SEED.new" 2> /dev/null; then
>  		old_umask=$(umask)
>  		umask 077
> -		dd if=/dev/urandom of="$URANDOM_SEED" bs="$pool_size" count=1 2> /dev/null
> -		status=$?
> +		dd if=/dev/urandom of="$URANDOM_SEED.tmp" bs="$pool_size" count=1 2> /dev/null
> +		cat "$URANDOM_SEED" "$URANDOM_SEED.tmp" 2>/dev/null \
> +			| sha256sum \
> +			| cut -d ' ' -f 1 > "$URANDOM_SEED.new" && \
> +		mv "$URANDOM_SEED.new" "$URANDOM_SEED" && status=0
> +		rm -f "$URANDOM_SEED.tmp"
>  		umask "$old_umask"
>  		if [ "$status" -eq 0 ]; then
>  			echo "OK"
>  		else
>  			echo "FAIL"
>  		fi
> +
>  	else
> -		status=$?
>  		echo "SKIP (read-only file system detected)"
>  	fi
>  	return "$status"
> -- 
> 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

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

[David Laight]

From: Yann E. MORIN
> Sent: 24 March 2022 08:25
> 
> +Peter: candidate for backporting as a security fix

Probably not - the security fix is the code that actually
initialises the RNG.

	David

> On 2022-03-23 14:07 -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.
> >
> > As a result of this behavior, which may be understandably surprising,
> > writing a good seed file into /dev/urandom and then saving a new 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.
> >
> > This has been fixed in systemd with
> > <https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
> > and fortunately it's possible to do the same thing in shell script here.
> > Specifically, instead of just saving new /dev/urandom output straight
> > up, we hash the new /dev/urandom together with the old seed, in order to
> > produce the new seed. This way the amount of entropy in the new seed
> > will stay the same or get better, but not appreciably regress.
> >
> > At the same time, the pool size check in this script is useless. Writing
> > to /dev/urandom never credits bits anyway, so no matter what, writing
> > into /dev/urandom is useful and not harmful. There's also not much of a
> > point in seeding with more than 256 bits, which is what the hashing
> > operation above produces. So this commit removes the file size check.
> >
> > 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.
> >
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> Applied to master, thanks.
> 
> Regards,
> Yann E. MORIN.
> 
> > ---
> >  package/urandom-scripts/S20urandom | 39 +++++++++++++-----------------
> >  1 file changed, 17 insertions(+), 22 deletions(-)
> >
> > diff --git a/package/urandom-scripts/S20urandom b/package/urandom-scripts/S20urandom
> > index e4fd125721..c6b2ebd48f 100644
> > --- a/package/urandom-scripts/S20urandom
> > +++ b/package/urandom-scripts/S20urandom
> > @@ -17,43 +17,38 @@ else
> >  	pool_size=512
> >  fi
> >
> > -check_file_size() {
> > -	[ -f "$URANDOM_SEED" ] || return 1
> > -	# Try to read two blocks but exactly one will be read if the file has
> > -	# the correct size.
> > -	size=$(dd if="$URANDOM_SEED" bs="$pool_size" count=2 2> /dev/null | wc -c)
> > -	test "$size" -eq "$pool_size"
> > -}
> > -
> >  init_rng() {
> > -	if check_file_size; then
> > -		printf 'Initializing random number generator: '
> > -		dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
> > -		status=$?
> > -		if [ "$status" -eq 0 ]; then
> > -			echo "OK"
> > -		else
> > -			echo "FAIL"
> > -		fi
> > -		return "$status"
> > +	printf 'Initializing random number generator: '
> > +	dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
> > +	status=$?
> > +	if [ "$status" -eq 0 ]; then
> > +		echo "OK"
> > +	else
> > +		echo "FAIL"
> >  	fi
> > +	return "$status"
> >  }
> >
> >  save_random_seed() {
> >  	printf 'Saving random seed: '
> > -	if touch "$URANDOM_SEED" 2> /dev/null; then
> > +	status=1
> > +	if touch "$URANDOM_SEED.new" 2> /dev/null; then
> >  		old_umask=$(umask)
> >  		umask 077
> > -		dd if=/dev/urandom of="$URANDOM_SEED" bs="$pool_size" count=1 2> /dev/null
> > -		status=$?
> > +		dd if=/dev/urandom of="$URANDOM_SEED.tmp" bs="$pool_size" count=1 2> /dev/null
> > +		cat "$URANDOM_SEED" "$URANDOM_SEED.tmp" 2>/dev/null \
> > +			| sha256sum \
> > +			| cut -d ' ' -f 1 > "$URANDOM_SEED.new" && \
> > +		mv "$URANDOM_SEED.new" "$URANDOM_SEED" && status=0
> > +		rm -f "$URANDOM_SEED.tmp"
> >  		umask "$old_umask"
> >  		if [ "$status" -eq 0 ]; then
> >  			echo "OK"
> >  		else
> >  			echo "FAIL"
> >  		fi
> > +
> >  	else
> > -		status=$?
> >  		echo "SKIP (read-only file system detected)"
> >  	fi
> >  	return "$status"
> > --
> > 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

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Yann E. MORIN]

David, All,

On 2022-03-24 09:15 +0000, David Laight spake thusly:
> From: Yann E. MORIN
> > Sent: 24 March 2022 08:25
> > 
> > +Peter: candidate for backporting as a security fix
> Probably not - the security fix is the code that actually
> initialises the RNG.

Not sure I understood... As Jason explained, we have so far been saving
a seed from an RNG that is probably partially deterministic; that is the
security issue.

The way we are seeding the RNG fundamentally does not change, because
we are not crediting any entropy with whatever we put in there. So,
whether we write something, anything, or nothing at all, has no impact
on the strength of the RNG.

As far as I understood it, at least.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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

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

[David Laight]

From: Yann E. MORIN
> Sent: 24 March 2022 10:09
> 
> David, All,
> 
> On 2022-03-24 09:15 +0000, David Laight spake thusly:
> > From: Yann E. MORIN
> > > Sent: 24 March 2022 08:25
> > >
> > > +Peter: candidate for backporting as a security fix
> > Probably not - the security fix is the code that actually
> > initialises the RNG.
> 
> Not sure I understood... As Jason explained, we have so far been saving
> a seed from an RNG that is probably partially deterministic; that is the
> security issue.

I'm pretty sure the seed is also saved during shutdown.
So a normal startup doesn't rely on the seed saved by the startup script.
(Although is would be better to use a background script
to save after (say) 30 minutes of operation.)

> The way we are seeding the RNG fundamentally does not change, because
> we are not crediting any entropy with whatever we put in there. So,
> whether we write something, anything, or nothing at all, has no impact
> on the strength of the RNG.

Right - the problem is that RNG isn't being given any entropy
at boot time.
So it is very weak.
The only point in saving/loading a seed is to give the RNG some
entropy at boot time.
Otherwise programs run from startup scripts that read the RNG
to get session keys don't get very good values at all.
That is the real security problem.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Yann E. MORIN]

David, All,

On 2022-03-24 10:25 +0000, David Laight spake thusly:
> From: Yann E. MORIN
> > On 2022-03-24 09:15 +0000, David Laight spake thusly:
> > > From: Yann E. MORIN
> > > > +Peter: candidate for backporting as a security fix
> > > Probably not - the security fix is the code that actually
> > > initialises the RNG.
> > Not sure I understood... As Jason explained, we have so far been saving
> > a seed from an RNG that is probably partially deterministic; that is the
> > security issue.
> I'm pretty sure the seed is also saved during shutdown.
> So a normal startup doesn't rely on the seed saved by the startup script.
> (Although is would be better to use a background script
> to save after (say) 30 minutes of operation.)

The problem is that a lot, if not most, embedded devices are never
properly shut-down; instead, they are, intentionally or not, brutally
electrically powered off.

In that case, the seed that is saved at boot is what is going to be
reused on the next boot. And currently, that seed is weak, because the
output of the RNG at  boot is mostly predicatble.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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

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

[David Laight]

From: Yann E. MORIN
> Sent: 24 March 2022 10:40
> 
> David, All,
> 
> On 2022-03-24 10:25 +0000, David Laight spake thusly:
> > From: Yann E. MORIN
> > > On 2022-03-24 09:15 +0000, David Laight spake thusly:
> > > > From: Yann E. MORIN
> > > > > +Peter: candidate for backporting as a security fix
> > > > Probably not - the security fix is the code that actually
> > > > initialises the RNG.
> > > Not sure I understood... As Jason explained, we have so far been saving
> > > a seed from an RNG that is probably partially deterministic; that is the
> > > security issue.
> > I'm pretty sure the seed is also saved during shutdown.
> > So a normal startup doesn't rely on the seed saved by the startup script.
> > (Although is would be better to use a background script
> > to save after (say) 30 minutes of operation.)
> 
> The problem is that a lot, if not most, embedded devices are never
> properly shut-down; instead, they are, intentionally or not, brutally
> electrically powered off.
> 
> In that case, the seed that is saved at boot is what is going to be
> reused on the next boot. And currently, that seed is weak, because the
> output of the RNG at  boot is mostly predicatble.

Maybe, but what you are saving at boot is also very weak.
Even if the code worked as intended it is almost entirely
completely dependant on the previous value.
So you might as well just count the number of times the
system has been booted.

There really is little point saving a new block of 'entropy'
until the system has had some chance to locate some.

The embedded device's system software also needs to find
somewhere non-volatile to save the entropy.
If the system is likely to just get powered off then none
of the filesystems are actually likely to be mounted rw.
Things like /, /etc, /var and /tmp are very likely to be ramdisk
(or ramdisk overlays) to avoid trashing the actual filesystems [1].

So saving the entropy may not be as simple as just writing to
a file - the filesystem might need (re)mounting first.

[1] Of course, if you are using a flash disk it might decide
to do 'wear levelling' at the time the power is cut.
In that case you can lose 'big time'.
I've seen a completely trashed FAT filesystem.
Nothing was in the right place at all.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Jason A. Donenfeld]

Hi David,

On Thu, Mar 24, 2022 at 3:15 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Yann E. MORIN
> > Sent: 24 March 2022 08:25
> >
> > +Peter: candidate for backporting as a security fix
>
> Probably not - the security fix is the code that actually
> initialises the RNG.

No. The security fix here is that the old and new seeds are hashed
together so that the new one doesn't regress in entropy. Yann is
correct.

Jason
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[David Laight]

From: Jason A. Donenfeld
> Sent: 24 March 2022 13:54
> 
> On Thu, Mar 24, 2022 at 3:15 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Yann E. MORIN
> > > Sent: 24 March 2022 08:25
> > >
> > > +Peter: candidate for backporting as a security fix
> >
> > Probably not - the security fix is the code that actually
> > initialises the RNG.
> 
> No. The security fix here is that the old and new seeds are hashed
> together so that the new one doesn't regress in entropy. Yann is
> correct.

But there is no entropy, you've never let the system collect any.

It is still completely broken.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Jason A. Donenfeld]

Hi David,

On Thu, Mar 24, 2022 at 8:31 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Jason A. Donenfeld
> > Sent: 24 March 2022 13:54
> >
> > On Thu, Mar 24, 2022 at 3:15 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Yann E. MORIN
> > > > Sent: 24 March 2022 08:25
> > > >
> > > > +Peter: candidate for backporting as a security fix
> > >
> > > Probably not - the security fix is the code that actually
> > > initialises the RNG.
> >
> > No. The security fix here is that the old and new seeds are hashed
> > together so that the new one doesn't regress in entropy. Yann is
> > correct.
>
> But there is no entropy, you've never let the system collect any.
>
> It is still completely broken.

You misunderstand the problem. An initial seed might be derived from
some external source and be high quality. The goal is for the next
seed, generated on-device, to be not lower quality. Prior to this
patch, it might become lower quality. With this patch, it now doesn't
regress in quality.

Jason
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Jason, All,
 > +Peter: candidate for backporting as a security fix

 > On 2022-03-23 14:07 -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.
 >> 
 >> As a result of this behavior, which may be understandably surprising,
 >> writing a good seed file into /dev/urandom and then saving a new 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.
 >> 
 >> This has been fixed in systemd with
 >> <https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
 >> and fortunately it's possible to do the same thing in shell script here.
 >> Specifically, instead of just saving new /dev/urandom output straight
 >> up, we hash the new /dev/urandom together with the old seed, in order to
 >> produce the new seed. This way the amount of entropy in the new seed
 >> will stay the same or get better, but not appreciably regress.
 >> 
 >> At the same time, the pool size check in this script is useless. Writing
 >> to /dev/urandom never credits bits anyway, so no matter what, writing
 >> into /dev/urandom is useful and not harmful. There's also not much of a
 >> point in seeding with more than 256 bits, which is what the hashing
 >> operation above produces. So this commit removes the file size check.
 >> 
 >> 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.
 >> 
 >> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

 > Applied to master, thanks.

Committed to 2021.02.x and 2022.02.x, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Eugen.Hristev--- via buildroot]

On 3/28/22 4:17 PM, Peter Korsgaard wrote:
>>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> 
>   > Jason, All,
>   > +Peter: candidate for backporting as a security fix
> 
>   > On 2022-03-23 14:07 -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.
>   >>
>   >> As a result of this behavior, which may be understandably surprising,
>   >> writing a good seed file into /dev/urandom and then saving a new 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.
>   >>
>   >> This has been fixed in systemd with
>   >> <https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
>   >> and fortunately it's possible to do the same thing in shell script here.
>   >> Specifically, instead of just saving new /dev/urandom output straight
>   >> up, we hash the new /dev/urandom together with the old seed, in order to
>   >> produce the new seed. This way the amount of entropy in the new seed
>   >> will stay the same or get better, but not appreciably regress.
>   >>
>   >> At the same time, the pool size check in this script is useless. Writing
>   >> to /dev/urandom never credits bits anyway, so no matter what, writing
>   >> into /dev/urandom is useful and not harmful. There's also not much of a
>   >> point in seeding with more than 256 bits, which is what the hashing
>   >> operation above produces. So this commit removes the file size check.
>   >>
>   >> 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.
>   >>
>   >> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
>   > Applied to master, thanks.
> 
> Committed to 2021.02.x and 2022.02.x, thanks.

Hello Jason,

I noticed that you worked a lot on the S20urandom init script for 
sysvinit in buildroot.

I recently updated from 2022.02 release to 2022.02.1 and now on booting 
all our boards, I noticed this message:

"Initializing random number generator: FAIL"


With the 2022.02 release, with the same kernel, the message stated "OK".

So it might be something changed on this BR revision that causes this fail.

As you worked more on these scripts , do you have maybe an idea or a 
hint about what I can try , to have it working as before ?
Or maybe what change can cause it ?

I tried to test 2022.02 + this patch I am replying to, and things appear 
to work fine. So it must be something else then...

Thank you for your help !

Eugen

> 
> --
> Bye, Peter Korsgaard
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
> 

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Nicolas Cavallari]

On 15/04/2022 12:54, Eugen.Hristev--- via buildroot wrote:
> On 3/28/22 4:17 PM, Peter Korsgaard wrote:
>>>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
>>
>>    > Jason, All,
>>    > +Peter: candidate for backporting as a security fix
>>
>>    > On 2022-03-23 14:07 -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.
>>    >>
>>    >> As a result of this behavior, which may be understandably surprising,
>>    >> writing a good seed file into /dev/urandom and then saving a new 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.
>>    >>
>>    >> This has been fixed in systemd with
>>    >> <https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b>,
>>    >> and fortunately it's possible to do the same thing in shell script here.
>>    >> Specifically, instead of just saving new /dev/urandom output straight
>>    >> up, we hash the new /dev/urandom together with the old seed, in order to
>>    >> produce the new seed. This way the amount of entropy in the new seed
>>    >> will stay the same or get better, but not appreciably regress.
>>    >>
>>    >> At the same time, the pool size check in this script is useless. Writing
>>    >> to /dev/urandom never credits bits anyway, so no matter what, writing
>>    >> into /dev/urandom is useful and not harmful. There's also not much of a
>>    >> point in seeding with more than 256 bits, which is what the hashing
>>    >> operation above produces. So this commit removes the file size check.
>>    >>
>>    >> 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.
>>    >>
>>    >> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>>
>>    > Applied to master, thanks.
>>
>> Committed to 2021.02.x and 2022.02.x, thanks.
> 
> Hello Jason,
> 
> I noticed that you worked a lot on the S20urandom init script for
> sysvinit in buildroot.
> 
> I recently updated from 2022.02 release to 2022.02.1 and now on booting
> all our boards, I noticed this message:
> 
> "Initializing random number generator: FAIL"
> 
> 
> With the 2022.02 release, with the same kernel, the message stated "OK".
> 
> So it might be something changed on this BR revision that causes this fail.
> 
> As you worked more on these scripts , do you have maybe an idea or a
> hint about what I can try , to have it working as before ?
> Or maybe what change can cause it ?
> 
> I tried to test 2022.02 + this patch I am replying to, and things appear
> to work fine. So it must be something else then...
> 
> Thank you for your help !

If /var/lib/random-seed does not already exist during boot then the new version 
of the script will fail and not create it, because start is "init_rng && 
save_random_seed" and init_rng will fail.  If S20urandom stop is never called 
then this may explain the issue.

I pointed this out before but it must have been lost:

https://lore.kernel.org/all/20220324103939.GD3649946@scaer/T/#md433db1bd1e08f3476eefd4e9a83ead5e12b1276

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Peter Korsgaard]

>>>>>   <Eugen.Hristev@microchip.com> writes:

Hi,

 >> Committed to 2021.02.x and 2022.02.x, thanks.

 > Hello Jason,

 > I noticed that you worked a lot on the S20urandom init script for 
 > sysvinit in buildroot.

 > I recently updated from 2022.02 release to 2022.02.1 and now on booting 
 > all our boards, I noticed this message:

 > "Initializing random number generator: FAIL"

 > With the 2022.02 release, with the same kernel, the message stated "OK".

This comes from an error writing the seed value to /dev/urandom:

https://git.buildroot.org/buildroot/tree/package/urandom-scripts/S20urandom#n22

Given that you state that it displayed OK in 2022.02, I take it that you
have a writable and persistent /var/lib, so /var/lib/urandom-seed is
present?

Maybe just try doing that by hand (without the 2>/dev/null) and see what
error message dd displays?

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Peter Korsgaard]

>>>>> "Nicolas" == Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr> writes:

Hi,

 >> to work fine. So it must be something else then...
 >> Thank you for your help !

 > If /var/lib/random-seed does not already exist during boot then the
 > new version of the script will fail and not create it, because start
 > is "init_rng && save_random_seed" and init_rng will fail.  If
 > S20urandom stop is never called then this may explain the issue.

 > I pointed this out before but it must have been lost:

 > https://lore.kernel.org/all/20220324103939.GD3649946@scaer/T/#md433db1bd1e08f3476eefd4e9a83ead5e12b1276

Ahh, indeed. Care to send a patch to fix it?

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH] package/urandom-scripts: do not seed if initial seed doesn't exist

[Jason A. Donenfeld]

By returning a failure in the event that the initial seed doesn't exist,
we'd then skip creating a new seed, which means we'd never in fact have
an initial seed, and this script is therefore useless. Fix this by
checking for the existence of the seed file first, and just returning 0
if it's not there.

Reported-by: Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>
Reported-by: Eugen Hristev <Eugen.Hristev@microchip.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 package/urandom-scripts/S20urandom | 1 +
 1 file changed, 1 insertion(+)

diff --git a/package/urandom-scripts/S20urandom b/package/urandom-scripts/S20urandom
index c6b2ebd48f..6c6aea9eee 100644
--- a/package/urandom-scripts/S20urandom
+++ b/package/urandom-scripts/S20urandom
@@ -18,6 +18,7 @@ else
 fi
 
 init_rng() {
+	[ -f "$URANDOM_SEED" ] || return 0
 	printf 'Initializing random number generator: '
 	dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
 	status=$?
-- 
2.35.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/urandom-scripts: do not seed if initial seed doesn't exist

[Peter Korsgaard]

>>>>> "Jason" == Jason A Donenfeld <Jason@zx2c4.com> writes:

 > By returning a failure in the event that the initial seed doesn't exist,
 > we'd then skip creating a new seed, which means we'd never in fact have
 > an initial seed, and this script is therefore useless. Fix this by
 > checking for the existence of the seed file first, and just returning 0
 > if it's not there.

 > Reported-by: Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>
 > Reported-by: Eugen Hristev <Eugen.Hristev@microchip.com>
 > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/urandom-scripts: do not seed if initial seed doesn't exist

[Eugen.Hristev--- via buildroot]

On 4/16/22 4:47 PM, Peter Korsgaard wrote:

>>>>>> "Jason" == Jason A Donenfeld <Jason@zx2c4.com> writes:
> 
>   > By returning a failure in the event that the initial seed doesn't exist,
>   > we'd then skip creating a new seed, which means we'd never in fact have
>   > an initial seed, and this script is therefore useless. Fix this by
>   > checking for the existence of the seed file first, and just returning 0
>   > if it's not there.
> 
>   > Reported-by: Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>
>   > Reported-by: Eugen Hristev <Eugen.Hristev@microchip.com>
>   > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> Committed, thanks.
> 
> --
> Bye, Peter Korsgaard
> 


Hi Jason,

I cherry-picked this patch to my 2022.02.1 tree, and rebuilt my images.
I still see this message at boot time :

"Initializing random number generator: FAIL"

Any ideas about what I could try ?

Thanks,
Eugen
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/urandom-scripts: do not seed if initial seed doesn't exist

[Jason A. Donenfeld]

Hi Eugen,

On Mon, Apr 18, 2022 at 08:19:13PM +0000, Eugen.Hristev--- via buildroot wrote:
> On 4/16/22 4:47 PM, Peter Korsgaard wrote:
> 
> >>>>>> "Jason" == Jason A Donenfeld <Jason@zx2c4.com> writes:
> > 
> >   > By returning a failure in the event that the initial seed doesn't exist,
> >   > we'd then skip creating a new seed, which means we'd never in fact have
> >   > an initial seed, and this script is therefore useless. Fix this by
> >   > checking for the existence of the seed file first, and just returning 0
> >   > if it's not there.
> > 
> >   > Reported-by: Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>
> >   > Reported-by: Eugen Hristev <Eugen.Hristev@microchip.com>
> >   > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > 
> > Committed, thanks.
> > 
> > --
> > Bye, Peter Korsgaard
> > 
> 
> 
> Hi Jason,
> 
> I cherry-picked this patch to my 2022.02.1 tree, and rebuilt my images.
> I still see this message at boot time :
> 
> "Initializing random number generator: FAIL"
> 
> Any ideas about what I could try ?

Huh, no, not sure. Are you sure your new images are running the new
file? I've manually run that function and I don't see it erroring...

Jason
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/urandom-scripts: do not seed if initial seed doesn't exist

[Peter Korsgaard]

>>>>>   <Eugen.Hristev@microchip.com> writes:

 > On 4/16/22 4:47 PM, Peter Korsgaard wrote:
 >>>>>>> "Jason" == Jason A Donenfeld <Jason@zx2c4.com> writes:
 >> 
 >> > By returning a failure in the event that the initial seed doesn't exist,
 >> > we'd then skip creating a new seed, which means we'd never in fact have
 >> > an initial seed, and this script is therefore useless. Fix this by
 >> > checking for the existence of the seed file first, and just returning 0
 >> > if it's not there.
 >> 
 >> > Reported-by: Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>
 >> > Reported-by: Eugen Hristev <Eugen.Hristev@microchip.com>
 >> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
 >> 
 >> Committed, thanks.
 >> 
 >> --
 >> Bye, Peter Korsgaard
 >> 


 > Hi Jason,

 > I cherry-picked this patch to my 2022.02.1 tree, and rebuilt my images.
 > I still see this message at boot time :

 > "Initializing random number generator: FAIL"

 > Any ideas about what I could try ?

Not really. If you see the message, then that must mean that
/var/lib/random-seed exists in your rootfs. Can you provide some more
details about your setup, E.G. what kind of rootfs file system, is it
mounted R/W, what platform, kernel version?

What error message do you get from dd if you run the dd to /dev/urandom
statement manually?

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/urandom-scripts: do not seed if initial seed doesn't exist

[Eugen.Hristev--- via buildroot]

On 4/18/22 11:36 PM, Jason A. Donenfeld wrote:
> Hi Eugen,
> 
> On Mon, Apr 18, 2022 at 08:19:13PM +0000, Eugen.Hristev--- via buildroot wrote:
>> On 4/16/22 4:47 PM, Peter Korsgaard wrote:
>>
>>>>>>>> "Jason" == Jason A Donenfeld <Jason@zx2c4.com> writes:
>>>
>>>    > By returning a failure in the event that the initial seed doesn't exist,
>>>    > we'd then skip creating a new seed, which means we'd never in fact have
>>>    > an initial seed, and this script is therefore useless. Fix this by
>>>    > checking for the existence of the seed file first, and just returning 0
>>>    > if it's not there.
>>>
>>>    > Reported-by: Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>
>>>    > Reported-by: Eugen Hristev <Eugen.Hristev@microchip.com>
>>>    > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>>>
>>> Committed, thanks.
>>>
>>> --
>>> Bye, Peter Korsgaard
>>>
>>
>>
>> Hi Jason,
>>
>> I cherry-picked this patch to my 2022.02.1 tree, and rebuilt my images.
>> I still see this message at boot time :
>>
>> "Initializing random number generator: FAIL"
>>
>> Any ideas about what I could try ?
> 
> Huh, no, not sure. Are you sure your new images are running the new
> file? I've manually run that function and I don't see it erroring...
> 
> Jason
> 


I apologize, it looks like I committed the patch to the wrong branch.
It's working fine now, reported OK on the message.

Thanks for helping out !

Eugen
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/urandom-scripts: do not seed if initial seed doesn't exist

[Peter Korsgaard]

>>>>> "Jason" == Jason A Donenfeld <Jason@zx2c4.com> writes:

 > By returning a failure in the event that the initial seed doesn't exist,
 > we'd then skip creating a new seed, which means we'd never in fact have
 > an initial seed, and this script is therefore useless. Fix this by
 > checking for the existence of the seed file first, and just returning 0
 > if it's not there.

 > Reported-by: Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr>
 > Reported-by: Eugen Hristev <Eugen.Hristev@microchip.com>
 > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Committed to 2022.02.x, thanks.

> ---
 >  package/urandom-scripts/S20urandom | 1 +
 >  1 file changed, 1 insertion(+)

 > diff --git a/package/urandom-scripts/S20urandom b/package/urandom-scripts/S20urandom
 > index c6b2ebd48f..6c6aea9eee 100644
 > --- a/package/urandom-scripts/S20urandom
 > +++ b/package/urandom-scripts/S20urandom
 > @@ -18,6 +18,7 @@ else
 >  fi
 
 >  init_rng() {
 > +	[ -f "$URANDOM_SEED" ] || return 0
 >  	printf 'Initializing random number generator: '
 >  	dd if="$URANDOM_SEED" bs="$pool_size" of=/dev/urandom count=1 2> /dev/null
 >  	status=$?
 > -- 

 > 2.35.1


-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot