[U-Boot] [PATCH v2 4/4] lib: uuid: Fix unseeded PRNG on RANDOM_UUID=y

[Heinrich Schuchardt]

On 5/2/19 2:27 PM, Eugeniu Rosca wrote:
> The random uuid values (enabled via CONFIG_RANDOM_UUID=y) on our
> platform are always the same. Below is consistent on each cold boot:
>
>   => ### interrupt autoboot
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=d117f98e-6f2c-d04b-a5b2-331a19f91cb2
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=ad5ec4b6-2d9f-8544-9417-fe3bd1c9b1b3
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=cceb0b18-39cb-d547-9db7-03b405fa77d4
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=d4981a2b-0478-544e-9607-7fd3c651068d
>   => env default -a; gpt write mmc 1 $partitions; print uuid_gpt_misc
>   ...
>   uuid_gpt_misc=6d6c9a36-e919-264d-a9ee-bd00379686c7
>
> While the uuids do change on every 'gpt write' command, the values
> appear to be taken from the same pool, in the same order.
>
> Assuming U-Boot with RANDOM_UUID=y is deployed on a large number of
> devices, all those devices would essentially expose the same UUID,
> breaking the assumption of system/RFS/application designers who rely
> on UUID as being globally unique (e.g. a database using UUID as key
> would alias/mix up entries/records due to duplicated UUID).
>
> The root cause seems to be simply _not_ seeding PRNG before generating
> a random value. It turns out this belongs to an established class of
> PRNG-specific problems, commonly known as "unseeded randomness", for
> which I am able to find below bugs/CVE/CWE:
>   - https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2015-0285
>     ("CVE-2015-0285 openssl: handshake with unseeded PRNG")
>   - https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2015-9019
>     ("CVE-2015-9019 libxslt: math.random() in xslt uses unseeded
>     randomness")
>   - https://cwe.mitre.org/data/definitions/336.html
>     ("CWE-336: Same Seed in Pseudo-Random Number Generator (PRNG)")
>
> The first revision [1] of this patch updated the seed based on the
> output of get_timer(), similar to [4].
>
> There are two problems with this approach:
>   - get_timer() has a poor _ms_ resolution
>   - when gen_rand_uuid() is called in a loop, get_timer() returns the
>     same result, leading to the same seed being passed to srand(),
>     leading to the same uuid being generated for several partitions
>     with different names
>
> The above drawbacks have been addressed in the second version [2].
> In its third revision (current), the patch reworded the description
> and summary line to emphasize it is a *fix* rather than an improvement.
>
> Testing [3] consisted of running 'gpt write mmc 1 $partitions' in a
> loop on R-Car3 for several minutes, collecting 8844 randomly generated
> UUIDS. Two consecutive cold boots are concatenated in the log.
> As a result, all uuid values are unique (scripted check).
>
> Thanks to Roman, who reported the issue and provided support in fixing.
>
> [1] https://patchwork.ozlabs.org/patch/1091802/
> [2] https://patchwork.ozlabs.org/patch/1092945/
> [3] https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
> [4] commit da384a9d7628 ("net: rename and refactor eth_rand_ethaddr() function")
>
> Reported-by: Roman Stratiienko <roman.stratiienko at globallogic.com>
> Signed-off-by: Eugeniu Rosca <erosca at de.adit-jv.com>
Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

> --
> v3:
>   - Reworked the patch summary line and description to emphasize this is
>     a fix rather than an improvement by precisely pointing out the root
>     cause and mentioning related CVE/CWE.
> v2:
>   - https://patchwork.ozlabs.org/patch/1092945/
>   - Replaced get_timer(0) with get_ticks() and added rand() to seed value
>   - Performed extensive testing on R-Car3 (ARMv8). See:
>     https://gist.github.com/erosca/2820be9d554f76b982edd48474d0e7ca
> v1:
>   - https://patchwork.ozlabs.org/patch/1092944/
> ---
>   lib/uuid.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/lib/uuid.c b/lib/uuid.c
> index fa20ee39fc32..2d4d6ef7e461 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -238,6 +238,8 @@ void gen_rand_uuid(unsigned char *uuid_bin)
>   	unsigned int *ptr = (unsigned int *)&uuid;
>   	int i;
>
> +	srand(get_ticks() + rand());
> +
>   	/* Set all fields randomly */
>   	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
>   		*(ptr + i) = cpu_to_be32(rand());
>