341590 - In FIPS mode, softoken should report the fatalError condition before the isLoggedIn != PR_TRUE condition

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

In lib/softoken/fipstokn.c, we have:

  static PRBool isLoggedIn = PR_FALSE;
  static PRBool fatalError = PR_FALSE;
  ...
  /* FIPS required checks before any useful cryptographic services */
  static CK_RV sftk_fipsCheck(void) {
      if (isLoggedIn != PR_TRUE) 
          return CKR_USER_NOT_LOGGED_IN;
      if (fatalError) 
          return CKR_DEVICE_ERROR;
      return CKR_OK;
  }

Note the order we check these two error conditions.

I propose that we reverse the order of those two checks.
The order matters when both error conditions are true.
When both error conditions are true, we should report the
more serious error condition (fatalError).  This change
will also simplify our FIPS documentation.  I will be
able to simply say "when the module is in the Error state,
all security functions fail with the error code
CKR_DEVICE_ERROR."

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

Reverse the two checks and use !isLoggedIn instead of
isLoggedIn != PR_TRUE.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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

r=nelson

Comment 3

[Wan-Teh Chang]

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

Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.15; previous revision: 1.14
done

Checking in fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.11.2.4; previous revision: 1.11.2.3
done

Comment 4

[Robert Relyea]

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

Sounds like reasonable logic to me.

bob