202270 - potential for infinite loop in prng_GenerateGlobalRandomBytes

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Ian McGreer]

Comment 1

[Ian McGreer]

Oops, hit enter and the bug gets submitted.

Anyway, the following code:

381     while (len > 0) {
382         if (rng->avail == 0)
383             /* All available bytes are used, so generate more. */
384             rv = alg_fips186_1_x3_1(rng, NULL, q);
385         /* number of bytes to obtain on this iteration (max of 20) */
386         num = PR_MIN(rng->avail, len);
387         /* if avail < BSIZE, the first avail bytes have already been used. */
388         memcpy(output, rng->Xj + (BSIZE - rng->avail), num);
389         rng->avail -= num;
390         len -= num;
391         output += num;
392     }

has the potential for an infinite loop when alg_fips186_1_x3_1 returns
SECFailure, if rng->avail is 0.  Now, that function can only fail when the PRNG
violates the FIPS requirement that the output of the PRNG does not equal the
previous output.  It seems highly unlikely that this would happen.

The person who reported this (I'm trying to get more info) claims it is somewhat
repeatable.  I suspect that some part of some code is writing into memory it
shouldn't, and causing rng->isValid != 0.  I highly doubt the FIPS requirement
is repeatedly not being met.

Nonetheless, we should break out of the loop above when rv == SECFailure.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Ian, this bugs claims to be about NSS 3.0.  What is the correct version number?

If this is repeatable, seems like it's at least a P2.  And it's pretty 
trivial to fix.  I'll attach a patch.

Comment 3

[Wan-Teh Chang]

Attachment: patch

Nelson created this patch, and said:
  Ian, please review this patch.  Is it what you want?

Comment 4

[Ian McGreer]

Comment on attachment 122923 [details] [diff] [review]
patch


Yes, this looks good.  I set the version as 3.1 because this bug has existed as
long as the PRNG has existed, but perhaps that should be 3.1 (3.0 was
source-only, IIRC).

Comment 5

[Ian McGreer]

Just to note: I never heard back from the reporter, which is why I forgot about
this bug.  I still don't think this can happen without some really strange
things going on, but no reason not to remove the potential for an infinite loop.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Taking bug.

Wan-Teh, thanks for catching my mistake of posting the patch to the wrong bug!

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Patch checked in on trunk