336509 - Continuous RNG test failure does not immediately put the FIPS module in the error state

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

FIPS 140-2 Sec. 4.9 "Self-Tests" on p. 33 says:

  If a cryptographic module fails a self-test, the module shall
  enter an error state [...].  The cryptographic module shall not
  perform any cryptographic operations while in an error state.
  [...]

One of the self-tests is the continuous random number generation
(RNG) test.  If our RNG fails this test, we put the RNG itself in
the invalid state (see the statement "rng->isValid = PR_FALSE;" in
prng_fips1861.c:alg_fips186_2_cn_1), and the RNG can't be used
while it is in the invalid state (see the test for rng->isValid at
the beginning of the alg_fips186_2_cn_1 function).  But we do not
put the FIPS module in the error state.  We made an effort to do
that in fipstokn.c:FC_GenerateRandom, however, the C_GenerateRandom
function is not the only PKCS #11 function that uses the RNG.
Many other PKCS #11 functions, such the key generation functions,
also use the RNG, but we do not check for continuous RNG test failure
in those functions.  So, if the continuous RNG test fails during
key generation, the FIPS module can still be used to perform many
cryptographic operations until its C_GenerateRandom function is
invoked.

Although this is not a strict conformance to the FIPS 140-2
requirement, I think we meet the spirit of the requirement because
we do put the RNG in the error (invalid) state.

Comment 1

[Wan-Teh Chang]

Attachment: Solution 1: softoken registers a callback with freebl that is called on continuous RNG test failure

One way to fix this is to add a callback function to freebl
that is called when the continuous RNG test fails.  Then
softoken registers a callback with freebl that puts the
softoken in the error state and logs an audit message.

Note that this patch contains some audit-related changes
to lib/softoken/fipstokn.c that should be ignored.  This
patch should not be applied as is.  This patch is intended
to illustrate how this solution works.  This patch unfortunately
introduces a circular dependency between freebl and softoken
(the callback), but it requires fewer changes to the softoken
and is more robust with respect to future changes to the softoken.

Another solution is to check for continuous RNG test failure
after every call to RNG_RandomUpdate or
RNG_GenerateGlobalRandomBytes in lib/softoken.

Comment 2

[Wan-Teh Chang]

Attachment: Solution 2: check for continuous RNG test failure after every call to RNG_GenerateGlobalRandomBytes call

This patch implements the solution I outlined at the end of comment 1,
except that it doesn't check for continuous RNG test failure after calls
to RNG_RandomUpdate.  The rationale is:
  There are 106 RNG_RandomUpdate calls in lib/freebl and we don't check
  any of their return values.  Although RNG_RandomUpdate does perform
  the continuous RNG test, it doesn't generate any output, and a
  continuous RNG test failure during RNG_RandomUpdate will be detected
  the next time we call RNG_GenerateGlobalRandomBytes.  So it is safe
  to not check for continuous RNG test failure after calls to
  RNG_RandomUpdate (including calls to RNG_RNGInit and
  RNG_SystemInfoForRNG, which call RNG_RandomUpdate).

I check for continuous RNG test failure after calls to the following
functions:
- RNG_GenerateGlobalRandomBytes
- DSA_GenerateGlobalRandomBytes (very similar to the above function)

and calls to freebl functions that call the above two functions:
- RSA_NewKey
- RSA_PrivateKeyOp (RNG used for RSA bliding)
- RSA_PrivateKeyOpDoubleChecked (RNG used for RSA bliding)
- DSA_NewKey
- DSA_SignDigest
- DH_GenParam
- DH_NewKey
- EC_NewKey
- ECDSA_SignDigest
- PQG_ParamGen
- PQG_ParamGenSeedLen

This list was generated using LXR.  Code reviewers don't need to verify
this list is correct.

When the continuous RNG test fails, the error code is SEC_ERROR_LIBRARY_FAILURE.

To make it easier to put the FIPS token in the Error state from any source
file in lib/softoken, I made the variable 'fatalError' extern and renamed it
nsf_fatalError.

Comment 3

[Wan-Teh Chang]

Comment on attachment 230789 [details] [diff] [review]
Solution 2: check for continuous RNG test failure after every call to RNG_GenerateGlobalRandomBytes call

I just realized that RNG_GenerateGlobalRandomBytes may
also fail with the error codes SEC_ERROR_INVALID_ARGS and
SEC_ERROR_NEED_RANDOM.  I'm not checking the error code on
RNG_GenerateGlobalRandomBytes failure because existing code
in NSC_GenerateRandom isn't checking the error code.  If
you think we should check for the error code SEC_ERROR_LIBRARY_FAILURE
on RNG_GenerateGlobalRandomBytes failure, please note it in
your code review comments.

Note that not checking for the error code still meets the
FIPS requirement.  It just means we're putting the FIPS
token in the Error state under more conditions than required
by FIPS.

Comment 4

[Wan-Teh Chang]

Attachment: Make prng_GenerateGlobalRandomBytes static (for the NSS trunk only)

Using LXR, you can verify that prng_GenerateGlobalRandomBytes is
only used inside the file lib/freebl/prng_fips1861.c.  Therefore
it should be made static.  This patch is intended for the NSS
trunk only.

Comment 5

[Wan-Teh Chang]

Attachment: Correction: Make prng_GenerateGlobalRandomBytes static (for the NSS trunk only)

I attached the wrong patch. Sorry. This is the correct patch.

Comment 6

[Robert Relyea]

Comment on attachment 230789 [details] [diff] [review]
Solution 2: check for continuous RNG test failure after every call to RNG_GenerateGlobalRandomBytes call

r+
I'd also like to see, however, RNG_GenerateGlobalRandomBytes  return a new error code for failure of the random number generator and use that error code to determin when to set nsf_fatalerror.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 230789 [details] [diff] [review]
Solution 2: check for continuous RNG test failure after every call to RNG_GenerateGlobalRandomBytes call

Questions and observation about this patch (not the final review):

1. what does nsf mean?  National Science Foundation is the only expansion 
of that abbreviation that I know.  

2. IINM, By NSS convention, external names of the form 
  "prefix underscore suffix" 
are reserved for function names.  In particular, the underscore after the 
prefix signifies being a function, so I'd rather not see this global variable 
named with an underscore.

3. This is a global variable.  Now, imagine that we have multiple virtual 
softoken tokens, running concurrently in separate virtual slots, perhaps
some in FIPS mode and some not.  An error in any one of them would disable
them all, all slots, FIPS or not.  yes?  
Do we need an array of these variables, one per slot?

Comment 8

[Wan-Teh Chang]

The "f" in "nsf" means "FIPS".  I believe "ns" means "Netscape".
The "nsf" prefix comes from the existing extern variable "nsf_init".
Note that I modeled the variable name "nsf_fatalError" after "nsf_init".
(There is a static variable named "nsc_init" for the non-FIPS token.)
I'll be happy to rename nsf_fatalError.  Could you suggest a name?
fipsFatalError?

The FIPS token uses two global variables, fatalError and isLoggedIn,
that I agree should be per-slot.  In fact, the SFTKSlot structure also
has an isLoggedIn field.  I guess these are global variables
so that the FC_xxx functions can test them on entry to the functions.
If they are per-slot, the FC_xxx functions would need to decode the
slot ID or get the SFTKSlot structure from the session handle, which
would be redundant with what the NSC_xxx functions do.  Here is an
example that illustrates this point:

 CK_RV FC_EncryptInit(CK_SESSION_HANDLE hSession,
                 CK_MECHANISM_PTR pMechanism, CK_OBJECT_HANDLE hKey) {
    SFTK_FIPSCHECK();
    return NSC_EncryptInit(hSession,pMechanism,hKey);
}

Ideally, SFTK_FIPSCHECK() should not need to decode hSession.

In the current design, we put all the FIPS tokens in the Error
state.  Worse, the isLoggedIn variable applies to all the FIPS
token.  I hope that we don't need to support more than one FIPS
token in NSS 3.11.x.

Comment 9

[Neil Williams]

Comment on attachment 230794 [details] [diff] [review]
Correction: Make prng_GenerateGlobalRandomBytes static (for the NSS trunk only)

Review for this patch only. I read, but did not study,  the previous patch.

Comment 10

[Wan-Teh Chang]

Comment on attachment 230794 [details] [diff] [review]
Correction: Make prng_GenerateGlobalRandomBytes static (for the NSS trunk only)

I checked in the patch to make prng_GenerateGlobalRandomBytes
static on the NSS trunk (NSS 3.12).

Checking in prng_fips1861.c;
/cvsroot/mozilla/security/nss/lib/freebl/prng_fips1861.c,v  <--  prng_fips1861.c

new revision: 1.24; previous revision: 1.23
done

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 230789 [details] [diff] [review]
Solution 2: check for continuous RNG test failure after every call to RNG_GenerateGlobalRandomBytes call

Regarding comment 3: 
I think we don't care why the PRNG failed.  If the PRNG failed,
then the FIPS token is dead, regardless of the reason.

I think we should use as few prefixes as possible in each module of
NSS (where softoken is a module).  So, rather than inventing YA new
one for two global variables, let's use sftk (with no underscore).
So, nsf_init -> sftkInit   and 
nsf_fataError-> sftkFatalError and
  isLoggedIn -> sftkIsLoggedIn.

I think we can live with the idea that a PRNG failure will disable 
all our slots.  

IIRC, presently, when we are in "FIPS mode" we only have one slot,
not two (not a DB slot and a "generic crypto" slot).  
AS LONG AS THAT REMAINS TRUE, I don't have a problem with a single
global variable sftkIsLoggedIn, PROVIDED that it is only used in FIPS
mode.  But if we're using a single global variable that confuses the
logged in state of multiple slots, when we are operating with multiple
slots, THAT needs to be fixed ASAP, IMO.

So, r+, conditional upon the name changes shown above.

Comment 12

[Wan-Teh Chang]

Attachment: check for continuous RNG test failure after every call to RNG_GenerateGlobalRandomBytes call (as checked in)

In this patch I use the variable name sftk_fatalError and declare
it in a different header file, softoken.h.  I didn't find any variable
name in lib/softoken of the form sftkFooBar.  The command
"grep sftk * | grep -v _" showed that sftkFooBar is used for type names.

I checked in this patch on the NSS trunk (3.12) and NSS_3_11_BRANCH (3.11.3).

Comment 13

[Wan-Teh Chang]

Attachment: Minor change

This patch makes a minor adjustment to the patch I just checked in.
In lib/softoken/pkcs11.c, we should set sftk_fatalError to PR_TRUE
only if RNG_GenerateGlobalRandomBytes fails.

Comment 14

[Wan-Teh Chang]

Comment on attachment 231444 [details] [diff] [review]
Minor change

I forgot to say that I've checked in this patch on the NSS trunk (3.12)
and NSS_3_11_BRANCH (3.11.3).