The default bug view has changed. See this FAQ.

Use /dev/urandom if possible

RESOLVED FIXED in 3.3.2

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Andi Kleen, Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

3.3.1
3.3.2
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
unix_rand.c didn't use the urandom device when it was available (e.g. on linux)
This patch fixes it. It should improve the quality of the NSS session keys,
because urandom has much better entropy than anything else unix_rand.c currently
uses.
(Reporter)

Comment 1

16 years ago
Created attachment 46878 [details] [diff] [review]
Add /dev/urandom

Updated

16 years ago
Keywords: patch
Confirming. Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

16 years ago
Ian, could you evaluate this proposal?
Assignee: wtc → ian.mcgreer
Component: Tools → Libraries
Priority: -- → P1
Target Milestone: --- → 3.4
Version: unspecified → 3.3.1
(Assignee)

Comment 4

16 years ago
I've previously looked at this suggestion.  
I wonder how much time /dev/urandom takes to generate 32KB.
That's really my only concern.
(Reporter)

Comment 5

16 years ago
/dev/urandom for 32K costs about 32K/160bytes * cost of an SHA-1 + some
bookkeeping. 

Unlike /dev/random it'll never block waiting for new entropy, but when it
runs out of previously collected "noise" just degenerate to a pseudo random
RND seeded by some old remembered entropy and 64bit timestamps. On 
interactive boxes there should be always enough noise (every mouse move 
and keypress adds to it); on racked servers it is a bit more problematic
as it'll only get input from disk movements. It should be still better than
the method previously used.

If that should be too expensive you can use some cheaper secure RND and just
seed it with urandom.
(Assignee)

Comment 6

16 years ago
If the output of /dev/urandom is the output of a SHA-1 PRNG, there's
no point in taking more than 160 bits of it.  
How about some code that takes whatever data is available from /dev/random
plus 160 bits from /dev/urandom?   Is it feasible to read whatever data
is already available from /dev/random without blocking? 
(Reporter)

Comment 7

16 years ago
SHA-1 is only used to protect the integrity of the internal entropy pool.  
The output is SHA-1, but the input of the hash varies.  Normally (=enough
entropy in pool) for each 160bits output it'll have true random unique 160bits 
of input. Only when it runs out of entropy /dev/urandom will reuse
data in the entropy pool, but still using some feedback (e.g. with 
each /dev/urandom read the 64bit cycle counter is mixed in again) 
So it makes sense to use more data from /dev/urandom.

/dev/random doesn't have this fallback; it'll block for new entropy if it
runs out of it. There is also a cross possible; using non blocking reads
and checking the entropy count using the RNDGETENTCNT ioctl and asking
the user to move the mouse or press random keys to fill it again. This
is unfortunately rather annoying for the user and worse doesn't work
very well for remote X display.

The default size of the entropy pool is 512bytes for the primary pool + 
128 bytes in a secondary pool; so it probably doesn't make much sense 
to read more than 300-600 bytes.

Comment 8

16 years ago
I agree that it doesn't make sense to take more than (say) 1K from /dev/urandom.
 Our own PRNG will take the seeding bytes provided by RNG_FileForRNG and mix
them up with a SHA-1 hash.  Thus, reading in more bytes than the size of the
internal entropy pool from /dev/urandom will just cause double hashing.

This patch, however, still reads 32K.  Would you like to provide a patch that
only reads 1K?  It won't work to limit totalFileBytes to 1*1024, because
RNG_FileForRNG needs to be general enough to read files that are larger than 1K.
 I think the way to do it is to have a separate function for reading in the
device file, in lieu of modifying RNG_FileForRNG.
(Assignee)

Comment 9

16 years ago
Created attachment 57725 [details] [diff] [review]
redeclare RNG functions to treat their inputs as const

I've written an alternative patch for including up to 1KB of data from
/dev/urandom.  It depends on another patch.  I'll attach both patches here.
Ian, if you don't mind, I'll assign this bug to myself.

This first patch changes the declarations of RNG_RandomUpdate and
RNG_FileInfoForRNG to treat their inputs as const, and propagates const
as necessary.
(Assignee)

Comment 10

16 years ago
Created attachment 57726 [details] [diff] [review]
Take 1KB from /dev/urandom, if it exists, prior to other files

This second patch reads in 1KB from /dev/urandom prior to reading any 
data from any of the other files named.
Attachment #46878 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #57725 - Attachment is patch: true
(Assignee)

Comment 11

16 years ago
Comment on attachment 57725 [details] [diff] [review]
redeclare RNG functions to treat their inputs as const

Setting the "patch" flag on attachment 57725 [details] [diff] [review].

Comment 12

16 years ago
Nelson, go ahead and take the bug.  The patch looks good to me.
(Assignee)

Comment 13

16 years ago
Taking bug.
Assignee: ian.mcgreer → nelsonb
(Assignee)

Comment 14

16 years ago
The above two patches were checked in on the NSS_3_3_BRANCH.
I'm working on porting them to the trunk now.  
Status: NEW → ASSIGNED
(Assignee)

Comment 15

16 years ago
Code checked in on both NSS_3_3_BRANCH and on trunk.
If mozilla wants this code, someone in CPD will have to move the 
NSS_CLIENT_TAG on the affected files.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Target Milestone: 3.4 → 3.3.2

Comment 16

16 years ago
Comment on attachment 57726 [details] [diff] [review]
Take 1KB from /dev/urandom, if it exists, prior to other files

This patch looks good to me and I found that Ian
already reviewed the patch.  So Bob, you don't
need to review it again.

Nelson, I have some questions about this patch.

>-    char *files[] = {
>+    static const char * const files[] = {
> 	"/etc/passwd",
> 	"/etc/utmp",
> 	"/tmp",

I have to admit I have trouble deciphering this
double const syntax.  Does the second 'const' mean
the array itself is const, that is, its elements
can't be changed to point to other text strings?

Did you add the second 'const' so that the
compiler can put the array in the text segment?

>+#define TOTAL_FILE_LIMIT 1000000	/* one million */

This constant was 1024*1024 in the original code.
I am just curious as to why you changed it to
1000000.

>-	    if (totalFileBytes > 1024*1024) break;
>+	    /* after TOTAL_FILE_LIMIT has been reached, only read in first
>+	    ** buffer of data from each subsequent file.
>+	    */
>+	    if (totalFileBytes > TOTAL_FILE_LIMIT) 
>+		break;

Do you think that we should still read in at least one
buffer of data from each subsequent file after we have
exceeded the total limit?

Comment 17

16 years ago
Comment on attachment 57726 [details] [diff] [review]
Take 1KB from /dev/urandom, if it exists, prior to other files

The only change I'd suggest is that the new
function RNG_FileUpdate can be made static.
This is not a big deal.
You need to log in before you can comment on or make changes to this bug.