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

[Lukasz Majewski <lukma@denx.de>]

On Thu, 2 May 2019 14:27:06 +0200
Eugeniu Rosca <erosca@de.adit-jv.com> 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@globallogic.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> --
> 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());

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190507/5d233fd7/attachment.sig>