certutil's random noise generator isn't very efficient

RESOLVED FIXED in 3.12

Status

NSS
Tools
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mossop, Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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?
(Assignee)

Comment 1

10 years ago
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.
Assignee: nobody → nelson
(Assignee)

Updated

10 years ago
Summary: certutil's random noise generator isn't very random → certutil's random noise generator isn't very efficient
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Comment 2

10 years ago
Created attachment 271968 [details] [diff] [review]
patch v1

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

Comment 3

10 years ago
Comment on attachment 271968 [details] [diff] [review]
patch v1

Looks far more sensible to me. Gets my r+ for whatever that's worth ;)
Attachment #271968 - Flags: review+
(Assignee)

Comment 4

10 years ago
Created attachment 272060 [details] [diff] [review]
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.
Attachment #272060 - Flags: review?(dtownsend)
(Assignee)

Comment 5

10 years ago
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.
(Assignee)

Comment 6

10 years ago
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. :)
Attachment #272060 - Flags: review?(julien.pierre.boogz)

Comment 7

10 years ago
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.

Updated

10 years ago
Attachment #271968 - Flags: superreview+

Updated

10 years ago
Attachment #272060 - Flags: review?(julien.pierre.boogz) → review+
(Assignee)

Comment 8

10 years ago
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
Attachment #272060 - Attachment description: alternative patch, v2 → alternative patch, v2 (checked in on trunk)
Attachment #272060 - Flags: review?(dtownsend)
(Assignee)

Comment 9

10 years ago
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.  
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
You need to log in before you can comment on or make changes to this bug.