695571 - NSS needs more DRBG test to keep NIST happy in FIPS validations.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

According to NIST SP 800-90, there a several 'Health checks' we are missing in our POST, namely checking that we reject certain invalid input, and that automatic reseed works.

We also need to run these tests whenever we reseed.
This patch is for 3.13, it's not critical to get into the next release of 3.13, but upstream approval is critical.

Comment 1

[Robert Relyea]

Red hat also needs a version of the patch that applies to NSS 3.12.9, but does not interfere with the binary compatibility of freebl and softoken.

Comment 2

[Robert Relyea]

Elio, it may be good to get test builds with this patch ready...

bob

Comment 3

[Robert Relyea]

Attachment: Patch for 3-12.9

Comment 4

[Robert Relyea]

Attachment: Patch for 3.13.2

Comment 5

[Elio Maldonado]

(In reply to Robert Relyea from comment #2)
> Elio, it may be good to get test builds with this patch ready...

Scratch builds downstream for Fedora and RHEL are possible but how do I do a test build here? Mozilla has such a thing but not us with CVS. Any test build would have to be local unless we are free to checkin this patch in the NSS_3_12_BRANCH?

Comment 6

[Wan-Teh Chang]

Comment on attachment 567948 [details] [diff] [review]
Patch for 3.13.2

Review of attachment 567948 [details] [diff] [review]:
-----------------------------------------------------------------

Generally the patch is good.  Most of my comments below are about nits.

I suggest that we remove isFIPS and prng_reseed_test, and always do the
health tests in prng_reseed.  It should not affect performance that much.

::: lib/freebl/blapi.h
@@ -1257,3 +1257,5 @@
> >  extern SECStatus
> >  PRNGTEST_Uninstantiate(void);
> >  
> > +extern SECStatus
> > +PRNGTEST_HealthTests(void);

Nit: I suggest naming this function with a verb, such as
PRNGTEST_DoHealthTests or PRNGTEST_RunHealthTests.

::: lib/freebl/drbg.c
@@ -66,3 +66,4 @@
> >  #define PRNG_ADDITONAL_DATA_CACHE_SIZE (8*1024) /* must be less than
> >  						 *  PRNG_MAX_ADDITIONAL_BYTES
> >  						 */
> > +static PRBool isFIPS=PR_FALSE;

Nit: add spaces around the equal sign '='.

But, I recommend that we remove isFIPS and always operate the DRBG in FIPS
mode.

@@ -194,2 +195,2 @@
> >  static SECStatus
> >  prng_instantiate(RNGContext *rng, PRUint8 *bytes, unsigned int len)

Please add 'const' to 'bytes' to make it clear it's an input argument.

@@ -194,3 +195,5 @@
> >  static SECStatus
> >  prng_instantiate(RNGContext *rng, PRUint8 *bytes, unsigned int len)
> >  {
> > +    if (len < PRNG_SEEDLEN) {
> > +	PORT_SetError(SEC_ERROR_INVALID_ARGS);

Use SEC_ERROR_INPUT_LEN instead.  It's more specific than SEC_ERROR_INVALID_ARGS.

@@ -233,3 +238,5 @@
> >  	PORT_Memcpy(&noise[sizeof rng->V_Data],entropy, entropy_len);
> >      }
> >  
> > +    if (entropy_len < 256/PR_BITS_PER_BYTE) {
> > +	/* noise != &noiseData[0] at this point, so nothing to free */

I think the comment is wrong.  noise should be equal to &noiseData[0]
in this case.

@@ -234,2 +239,5 @@
> >      }
> >  
> > +    if (entropy_len < 256/PR_BITS_PER_BYTE) {
> > +	/* noise != &noiseData[0] at this point, so nothing to free */
> > +	return SECFailure;

We should set an error code.  Perhaps SEC_ERROR_NEED_RANDOM?

@@ -250,3 +260,4 @@
> >  }
> >  
> >  /*
> > + * SP 800-90 requires we rurun our health tests on reseed

Typo: rurun => rerun

@@ -252,1 +262,5 @@
> >  /*
> > + * SP 800-90 requires we rurun our health tests on reseed
> > + */
> > +static
> > +SECStatus

Nit: put static SECStatus on the same line.

@@ -253,0 +263,16 @@
> > + * SP 800-90 requires we rurun our health tests on reseed
> > + */
> > +static
> > +SECStatus
NaN more ...

Set SEC_ERROR_LIBRARY_FAILURE.

@@ -417,1 +453,1 @@
> >  	/* the RNG is in a valid state */

This comment must go after the if (rv != SECSuccess) test.  But it seems
better to move the if (rv != SECSuccess) test to line 445 above, after
            memset(bytes, 0, numBytes);

@@ -672,1 +712,5 @@
> > -   PRUint8 *bytes = PORT_Alloc(bytes_len);
> > +   PRUint8 *bytes = NULL;
> > +   SECStatus rv;
> > +
> > +   /* if we are calling PRNGTEST_Instantiate, we are running in FIPS mode,
> > +    * turn the isFIPS flag on so we do heal checks on reseed operations */

Typo: heal => health

@@ -672,1 +712,9 @@
> > -   PRUint8 *bytes = PORT_Alloc(bytes_len);
> > +   PRUint8 *bytes = NULL;
> > +   SECStatus rv;
> > +
> > +   /* if we are calling PRNGTEST_Instantiate, we are running in FIPS mode,
NaN more ...

Use SEC_ERROR_INPUT_LEN or SEC_ERROR_NEED_RANDOM instead.

@@ -674,1 +725,2 @@
> >     if (bytes == NULL) {
> > +	PORT_SetError(SEC_ERROR_INVALID_ARGS);

This should be SEC_ERROR_NO_MEMORY.

@@ -701,3 +756,4 @@
> >  	PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
> >  	return SECFailure;
> >      }
> > +   /* This magic tells us to set the reseed count to it's max count, 

Add "input" after "magic".  Change it's to its.

@@ -713,4 +776,5 @@
> >      if (!testContext.isValid) {
> >  	PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
> >  	return SECFailure;
> >      }
> > +    /* replicate reseed test from prng_GenerateGlobalRandomBytes */

This comment doesn't seem useful to me because the reseed test is obvious.

@@ -715,2 +778,5 @@
> >  	return SECFailure;
> >      }
> > +    /* replicate reseed test from prng_GenerateGlobalRandomBytes */
> > +    if (testContext.reseed_counter[0] >= RESEED_VALUE) {
> > +	rv = prng_reseed(&testContext, NULL, 0, NULL, 0);

This should be prng_reseed_test, right?  Otherwise you won't be running
the health tests.

@@ -729,0 +803,51 @@
> > +SECStatus
> > +PRNGTEST_HealthTests()
> > +{
> > +   static const PRUint8 entropy[] = {
NaN more ...

Remove two blank lines.

@@ -730,0 +876,8 @@
> > +   /********************************************/
> > +   /* Generate random bytes with a known seed. */
> > +   /********************************************/
> > +   rng_status = PRNGTEST_Instantiate(entropy, sizeof entropy, 
NaN more ...

Remove one of the return statements.

(Would be a good idea to check compiler warnings.)

::: lib/freebl/ldvector.c
@@ -290,2 +290,3 @@
> >  
> >      /* End of Version 3.013 */
> > +    PRNGTEST_HealthTests

Add the following comment after this line:
    /* End of Version 3.014 */

::: lib/freebl/loader.h
@@ -595,3 +595,4 @@
> >   PRBool (*p_BLAPI_SHVerifyFile)(const char *name);
> >  
> >    /* Version 3.013 came to here */
> > + SECStatus (*p_PRNGTEST_HealthTests)(void);

Add the following comment after this line:
 /* Version 3.014 came to here */

::: lib/softoken/fipstest.c
@@ +1991,5 @@
> +   /*******************************************/
> +   /*   Run the SP 800-90 Health tests        */
> +   /*******************************************/
> +   rng_status = PRNGTEST_HealthTests();
> +   if (rng_status == SECSuccess) {

BUG: this should be rng_status != SECSuccess

Comment 7

[Robert Relyea]

> I suggest that we remove isFIPS and prng_reseed_test, and always do the
> health tests in prng_reseed.  It should not affect performance that much.

I can remove isFIPS without a problem, but I still need 2 prng_reseed functions so I don't loop in when my tests runs (prng_reseed is called from several places inside the health tests themselves).

> > > +    /* replicate reseed test from prng_GenerateGlobalRandomBytes */
> This comment doesn't seem useful to me because the reseed test is obvious.

yeah, I added it when the Health tests fails because the health tests don't actually call prng_GenerateRandomBytes, but the Test interface. It's more a gripe against the way the SP 800-90 is specified. I'll remove the comment.


> This should be prng_reseed_test, right?  Otherwise you won't be running
> the health tests.

No. this function is called directly from the health tests, so calling prng_reseed_test would create an infinite loop. (See comment above about SP 800-90.




> Add the following comment after this line:
> /* Version 3.014 came to here */

yeah, I left it off until we actually freeze 3.014, which would be when we release 3.13.2 after this gets checked in, but I think you are right, I add it now, other functions added in 3.13.2 can still fall in 3.014.

> BUG: this should be rng_status != SECSuccess
Yikes! Right.


I'll also implement the rest of your comments.

Comment 8

[Robert Relyea]

Attachment: 3-13-2 incorporate wtc's comments [checked in]

Comment 9

[Robert Relyea]

/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.48; previous revision: 1.47
done
Checking in freebl/drbg.c;
/cvsroot/mozilla/security/nss/lib/freebl/drbg.c,v  <--  drbg.c
new revision: 1.11; previous revision: 1.10
done
Checking in freebl/ldvector.c;
/cvsroot/mozilla/security/nss/lib/freebl/ldvector.c,v  <--  ldvector.c
new revision: 1.32; previous revision: 1.31
done
Checking in freebl/loader.c;
/cvsroot/mozilla/security/nss/lib/freebl/loader.c,v  <--  loader.c
new revision: 1.57; previous revision: 1.56
done
Checking in freebl/loader.h;
/cvsroot/mozilla/security/nss/lib/freebl/loader.h,v  <--  loader.h
new revision: 1.38; previous revision: 1.37
done
Checking in softoken/fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.31; previous revision: 1.30
done

Comment 10

[Robert Relyea]

Comment on attachment 567946 [details] [diff] [review]
Patch for 3-12.9

A version of this is already in the 3.12.9 branch.

Comment 11

[Robert Relyea]

Everything in this bug has been checked in now.