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?
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.
Created attachment 271968 [details] [diff] [review] patch v1 Dave, is this patch satisfactory? (feel free to give it r+ :)
Comment on attachment 271968 [details] [diff] [review] patch v1 Looks far more sensible to me. Gets my r+ for whatever that's worth ;)
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.
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 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. :)
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 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
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.