Last Comment Bug 387621 - certutil's random noise generator isn't very efficient
: certutil's random noise generator isn't very efficient
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: trunk
: All All
: P3 normal (vote)
: 3.12
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-10 17:27 PDT by Dave Townsend [:mossop]
Modified: 2007-07-23 21:48 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (2.75 KB, patch)
2007-07-11 23:09 PDT, Nelson Bolyard (seldom reads bugmail)
dtownsend: review+
julien.pierre: superreview+
Details | Diff | Splinter Review
alternative patch, v2 (checked in on trunk) (2.74 KB, patch)
2007-07-12 13:40 PDT, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
Details | Diff | Splinter Review

Description Dave Townsend [:mossop] 2007-07-10 17:27:28 PDT
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) 2007-07-11 22:19:17 PDT
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) 2007-07-11 23:09:36 PDT
Created attachment 271968 [details] [diff] [review]
patch v1

Dave, is this patch satisfactory?  (feel free to give it r+  :)
Comment 3 Dave Townsend [:mossop] 2007-07-12 01:22:43 PDT
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) 2007-07-12 13:40:57 PDT
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.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2007-07-12 13:46:17 PDT
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) 2007-07-13 23:09:50 PDT
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 2007-07-17 16:50:17 PDT
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) 2007-07-23 21:46:27 PDT
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) 2007-07-23 21:48:06 PDT
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.  

Note You need to log in before you can comment on or make changes to this bug.