1303785 - NSS changes for web replay

Status: RESOLVED INVALID

(NSS :: Libraries, defect, P2)

Description

[Brian Hackett [Laid off!]]

This bug has one change to NSS which is needed by Web Replay (bug 1207696), in addition to the changes to NSPR (bug 1303779).

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

The random number seed generation incorporates the environment and its pointer address.  Since this can address vary between recording and replay, don't use the environment in such cases.

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 8792573 [details] [diff] [review]
patch

This is going to need review from Wan-Teh

Comment 3

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8792573 [details] [diff] [review]
patch

Review of attachment 8792573 [details] [diff] [review]:
-----------------------------------------------------------------

If the NSPR changes go through, this is fine with me.
This code has to die anyway.

Comment 4

[Wan-Teh Chang]

Comment on attachment 8792573 [details] [diff] [review]
patch

Review of attachment 8792573 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Brian: thank you for the patch, and sorry about the late review.

I marked this patch review- for two reasons.

1. I hope you can find a solution that does not require modifying
NSPR and NSS. Although many Mozilla developers contribute to NSPR
and NSS, we should treat NSPR and NSS as third-party code that
Mozilla doesn't have full control of. I hope web replay can be
implemented without modifying all third-party code used in Mozilla.

Note: I will add this same comment to NSPR bug 1303779, so if you
want to respond to this point, please follow up in that bug.

2. If you can't avoid modifying this NSS file, then I would prefer
either removing the code in question, or moving it after check that
we've read from /dev/urandom successfully:

#if defined(BSDI) || defined(FREEBSD) || defined(NETBSD) || defined(OPENBSD) || defined(DARWIN) || defined(LINUX) || defined(HPUX)
    if (bytes)
        return;
#endif

The reason is that we will be in big trouble if /dev/urandom does not
give us enough entropy to seed our PRNG.

Comment 5

[Wan-Teh Chang]

Attachment: Change RNG_SystemInfoForRNG to only read /dev/urandom and NSRANDFILE on major platforms

This patch changes RNG_SystemInfoForRNG to only read /dev/urandom
and the file specified in the NSRANDFILE environment variable on
platforms that have /dev/urandom.

I noticed two calls to GiveSystemInfo(). The second call seems
redundant and probably should be removed.

Comment 6

[Wan-Teh Chang]

Comment on attachment 8797297 [details] [diff] [review]
Change RNG_SystemInfoForRNG to only read /dev/urandom and NSRANDFILE on major platforms

Review of attachment 8797297 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/freebl/unix_rand.c
@@ +887,5 @@
> + * BSD/OS we do not call safe_popen when we succeeded in getting data
> + * from /dev/urandom.
> + *
> + * Bug 174993: On platforms providing /dev/urandom, don't fork netstat
> + * either, if data has been gathered successfully.

This entire comment block probably should also be updated.

Comment 7

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8797297 [details] [diff] [review]
Change RNG_SystemInfoForRNG to only read /dev/urandom and NSRANDFILE on major platforms

Review of attachment 8797297 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch Wan-Teh. We should do more clean up here.

::: lib/freebl/unix_rand.c
@@ +887,5 @@
> + * BSD/OS we do not call safe_popen when we succeeded in getting data
> + * from /dev/urandom.
> + *
> + * Bug 174993: On platforms providing /dev/urandom, don't fork netstat
> + * either, if data has been gathered successfully.

Not only the comment. We should take on bug 889116 and bug 1287231 and fix this entire file. This is already a step in the right direction for that but not sufficient (see bug 889116).

Comment 8

[Wan-Teh Chang]

Comment on attachment 8797297 [details] [diff] [review]
Change RNG_SystemInfoForRNG to only read /dev/urandom and NSRANDFILE on major platforms

Review of attachment 8797297 [details] [diff] [review]:
-----------------------------------------------------------------

Bob: since we need to document how we gather the entropy used to seed
the PRNG, I need your approval of this patch. Please review. Thanks.

Comment 9

[Wan-Teh Chang]

I'm sorry my previous comment is incomplete. I meant to say:

Bob: since we need to document how we gather the entropy used to seed
the PRNG for the FIPS validation, I need your approval of this patch.
Please review. Thanks.

Comment 10

[Robert Relyea]

wtc: RNG_SystemInfoForRNG isn't part of the FIPS seeding. From the FIPS point of view it's generating additional data used in subsequent calls to get random. This outside the FIPS seeding, so it's unnecessary to complete this patch for your FIPS validation.

RNG_SystemRNG() is the FIPS seeding (and grabs exactly the number of bytes FIPS needs to seed the drbg).

That being said, there's nothing wrong with cleaning this up, though I personally prefer to get as much system dependent information as possible to skew the results of our drbg as much as possible. When it comes to the drbg I find a healthy paranoia is good, so as far as spewing files into the drbg, I'd rather keep that. Doing forks and pipes causes more problems then their worth, so I'm OK with getting rid of those. Reducing the number of unnecessary ifdefs is a good thing as well.


bob

Comment 11

[Brian Hackett [Laid off!]]

All changes made to NSS by Web Replay have been removed.  The RNG initialization issues fixed by the patch here seem to no longer be a problem, and if they crop up again we'll deal with it in a new bug.