387621 - certutil's random noise generator isn't very efficient

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Dave Townsend [:mossop]]

When generating a new private key in certutil it asks the user to enter characters on the keyboard to initialise the random seed. It looks to me like there is an error in how this is coded however.

Look at the code the buffer is initialised to 60 characters. However only the first character is ever written to so the randomness is continually initialised by just one random character. Also rather than initialising with the full length of the buffer sizeof is used which merely returns the length of the buffer's pointer.

I'm a little confused as to why PK11_RandomUpdate is called inside the while loop too. Is there some significance to this or would it just be more sensible to fill the buffer with characters once and then call it with them all?

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

That code is indeed silly.  But it's not as bad as you might think.
It does manage to input into the PRNG all but the last of the 
characters entered.   The good news is that this "random" data is 
not the only source of seeding for NSS's PRNG.  It's just MORE
entropy, in addition to that which has previously been (or 
subsequently will be) entered.  It never hurts to feed lots of 
non-entropy into a PRNG.  You can't reduce the entropy that is 
already seeded into it (unless it's a REALLY LOUSY PRNG).  
So, yes, we can make this code more efficient, but it won't have 
a large impact on the actual entropy of the PRNG used by certutil.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Dave, is this patch satisfactory?  (feel free to give it r+  :)

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 271968 [details] [diff] [review]
patch v1

Looks far more sensible to me. Gets my r+ for whatever that's worth ;)

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative patch, v2 (checked in on trunk)

This patch is an alternative to the previous one.  
It differs only in the way that the progress meter is displayed.
Both of these seem to work equally well on Windows. 
I haven't tested on Unix/Linux.  I'm not sure which one will work 
better in general.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

The first patch outputs 63 characters for each non-repeated character typed.
I can just imagine how well that would work on a KSR33, but I 
guess nobody uses them anymore.  :-)

The second patch outputs 1 character for each non-repeated character typed.
But one some platforms, I think the \r would cause the rest of the line to
be blanked, meaning that the '|' character that marks the right end of the
progress meter would disappear, and would not be visible during the operation.

I guess it would be necessary to test on Unix/Linux with various terminal 
emulations to see if the missing-right-hand-delimiter problem actually 
occurs or not.  I consider the second patch to be preferable, IFF there is
no missing-right-hand-delimiter problem.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272060 [details] [diff] [review]
alternative patch, v2 (checked in on trunk)

Julien, please review these two alternative patches and cast your vote for one. :)

Comment 7

[Julien Pierre]

Nelson,

Either the patch is OK. But I would feel better if meter wasn't a hardcoded string but an array with a defined size readable in the code, and with boundary checking based on the string size during the assignments.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272060 [details] [diff] [review]
alternative patch, v2 (checked in on trunk)

Checking in keystuff.c; new revision: 1.17; previous revision: 1.16

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Julien, thanks for your reviews.  
The reason not to use a hard-coded "meter" would be to implement a 
variable-width meter, I think.  (Is there any other reason?) 
and we've lived with this fixed length meter for 11+ years now,
so I didn't feel a great need to change it.