Closed Bug 497217 Opened 11 years ago Closed 11 years ago

The first random value ever generated by the RNG should be discarded

Categories

(NSS :: Libraries, defect, P1, critical)

3.12.4
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: rrelyea, Assigned: rrelyea)

Details

(Whiteboard: FIPS [Awaiting Softoken's Thaw])

Attachments

(2 files, 1 obsolete file)

FIPs requires that first output of the rng be discarded and kept for the continuous prng test.
Whiteboard: FIPS [Awaiting Softoken's Thaw]
Attachment #382406 - Flags: review?(glen.beasley)
Severity: normal → critical
Priority: -- → P1
Target Milestone: --- → 3.12.4
Comment on attachment 382406 [details] [diff] [review]
make one call the rng and discard the output.

Bob, PR_STATIC_ASSERT can only be used in two places:
1. Outside of a function (it's an extern declaration), or 
2. At the very beginning of a basic block, that is, after the {
and before the first executable statement in that block.
The place where you inserted this call is neither of those.

>+ 	/* fetch one random value so that we can populate rng->oldV for our
>+ 	 * continous random number test. */
>+ 	PR_STATIC_ASSERT(PRNG_SEEDLEN*2 >= SHA256_LENGTH);
>+         prng_generateNewBytes(globalrng, bytes, SHA256_LENGTH, NULL, 0);
>+
I didn't have a problem on Linux, so I suspect those restrictions are because of other platforms.

new patch with static assert moved.
Attachment #382406 - Attachment is obsolete: true
Attachment #382406 - Flags: review?(glen.beasley)
Attachment #382416 - Flags: review?(nelson)
Comment on attachment 382419 [details] [diff] [review]
Friendlier version of this patch.

Bob, prng_generateNewBytes potentially can return an error code. I think you should not ignore it. Also, error code from prng_reseed call is ignored as well(same function line 361).
Actually I am intentionally ignoring it..;).

I'm keeping the same sematics we had in the previous rng to reduce complexity in the evaluation.

bob
Comment on attachment 382419 [details] [diff] [review]
Friendlier version of this patch.

r+.  I have two more thoughts that you may ignore. :)

1. I'd prefer to see the PR_STATIC_ASSERT call external to any function.
That emphasizes the fact that it's a compile time check, not a run-time 
check that happens at a certain point in the execution of the code.

2. Should the new call to prng_generateNewBytes come _after_ the call to 
RNG_SystemInfoForRNG rather than before it?
Attachment #382419 - Flags: review+
Comment on attachment 382416 [details] [diff] [review]
move static assert.

I reviewed the other copy of this patch.
Attachment #382416 - Flags: review?(nelson)
OS: Linux → All
Hardware: x86 → All
RE: alexi

A less flippant response.

prng_generateNewBytes can fail for exactly 2 reasons....

1) isValid isn't set. 

We set isValid one the line before our call.

2) V matches Vold. 

This is extremely unlikely, but harmless in this case. The purpose of the call is to make one run through the random number generator (which will have occurred by the time we do the check), and make sure V = Vold for the next comparison (which it is at this point or the compare would not have succeeded).

There is a bug here if we trip over this case, isValid would be set to 0. If we care I can change the code here (the chances of tripping over this is roughly 1 in 2^440 (or 10^132, so if every person on earth had 1 million computers and they initialized the rng once every second for their entire 80 year lives there would be a 1 in 6.7 x 10 ^104 chance one of those people would trip over the case).

That can easily be solved by always setting isValid before and after the call. I'm not sure it's worth it, but I would be willing to add that to the patch.

bob
re: nelson. and RNG_SystemInfoForRNG.

calling prng_generateNewBytes means it doesn't really matter. We have most of the input from RNG_SystemInfoForRNG going into the additional input buffer, so in either case it won't get mixed in until just before the application calls the random number generator. As it is, I just wanted to get it initialized as soon as possible. The reason for doing so is to make sure we don't ever return data back to the user that hasn't been check by the continuous random number check.


bob
Checking in drbg.c;
/cvsroot/mozilla/security/nss/lib/freebl/drbg.c,v  <--  drbg.c
new revision: 1.9; previous revision: 1.8
done
patch revied and checked in.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.