347024 - Move the software integrity test into sftk_fipsPowerUpSelfTest

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Derived Test Requirements for FIPS PUB 140-2 has this
requirement:

VE09.12.01: The vendor shall describe the procedure by
which an operator can initiate the power-up self-tests
on demand.  All of the power-up self-tests must be
included.

Our current procedure to initiate the power-up self-tests
on demand is to call FC_Login when the user is already
logged into the FIPS token.  We call the function
sftk_fipsPowerUpSelfTest in that case.

sftk_fipsPowerUpSelfTest performs the cryptographic
algorithm tests, but doesn't perform the software integrity
test, which is also a power-up self-test.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

The best way to ensure that we meet the requirement VE09.12.01
is to move the software integrity test into the sftk_fipsPowerUpSelfTest
function.  Then, whenever we call sftk_fipsPowerUpSelfTest, we can
rest assured that *all* of the power-up self-tests have been included.

But I believe the reason Bob added the software integrity test to
the very beginning of the nsc_CommonInitialize function is that he
wants to perform the software integrity test as early as possible.
Although this is not required by FIPS (FIPS just says the software
integrity test should be performed "when the module is powered up"),
I agree this is a good thing.  So it's a shame that the patch will
lose this.  If you feel strongly about this, please r- the patch.

Summary of the patch:
1. Move the software integrity test from nsc_CommonInitialize in
lib/softoken/pkcs11.c to the new function sftk_fipsSoftwareIntegrityTest
in lib/softoken/fipstest.c, and have sftk_fipsPowerUpSelfTest call
sftk_fipsSoftwareIntegrityTest.  Note that in the software integrity
test we need to pass the address of a static function to BLAPI_SHVerify.
Since the code is moved to a new file, we have to use a different
static function.

2. Update the audit logging code.  Remove an extraneous comma in
lib/softoken/fipstokn.c between the two string literals in the second
audit logging message.

Comment 2

[Wan-Teh Chang]

Attachment: Alternative patch

If we want to continue to perform the software/firmware
integrity test the first thing in FC_Initialize, we can
use this patch.  This patch refactors the software/firmware
integrity test into a new function sftk_fipsSoftwareIntegrityTest
and has FC_Login call it (before calling sftk_fipsPowerUpSelfTests,
which performs only the cryptographic algorithm tests).  The comment
for sftk_fipsPowerUpSelfTests is updated to reflect what it does.

Comment 3

[Robert Relyea]

Comment on attachment 231774 [details] [diff] [review]
Proposed patch

r= rrelyea I don't have a problem moving the test.

It simplifies logging.

bob

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 231774 [details] [diff] [review]
Proposed patch

It seems odd to me to test at Login time, rather than at Initialize time.
But assuming that is kosher, then the rest of this patch seems OK.

I did catch one very minor thing:


>@@ -1923,12 +1936,18 @@ sftk_fipsPowerUpSelfTest( void )
> #ifdef NSS_ENABLE_ECC
>     /* ECDSA Power-Up SelfTest(s). */
>     rv = sftk_fips_ECDSA_PowerUpSelfTest();
> 
>     if( rv != CKR_OK )
>         return rv;
> #endif
> 
>+    /* Software/Firmware Integrity Test. */
>+    rv = sftk_fipsSoftwareIntegrityTest();
>+
>+    if( rv != CKR_OK )
>+        return rv;
>     /* Passed Power-Up SelfTest(s). */
>     return( CKR_OK );

These last 3 executable lines, taken together, are exactly equivalent
to the single line:
      return rv;  

I would prefer the single line, for its simplicity.  But this is a nit.

> }

Comment 5

[Wan-Teh Chang]

Nelson,

The softoken does perform the power-up self-tests during
FC_Initialize.  But FIPS also requires that the operator be
able to initiate the power-up self-tests on demand.  The
softoken meets this requirement by overloading FC_Login --
if the operator is already logged into the token and calls
FC_Login again, it means he wants to initiate the power-up
self-tests.

You suggested that the three executable lines at the end of
sftk_fipsPowerUpSelfTest can be replaced by the single line
    return rv;
This is correct.  But we perform every self-test in the same way:

    rv = sftk_XxxTest();

    if( rv != CKR_OK )
        return rv;

Therefore, keeping the if statement for the last self-test
will avoid a possible mistake when we add a new self-test in
the future.

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

Checking in fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.18; previous revision: 1.17
done
Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.17; previous revision: 1.16
done
Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.128; previous revision: 1.127
done

Checking in fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.13.2.5; previous revision: 1.13.2.4
done
Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.11.2.6; previous revision: 1.11.2.5
done
Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.112.2.13; previous revision: 1.112.2.12
done