1453408 - modutil -changepw fails in FIPS mode if password is an empty string

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Hubert Kario]

As mentioned in bug 1415847, prior to bug 1395495 being fixed, simple creation of database would leave it in a semi-initialised state. To fully initialise it, setting a password on it was necessary.

Additionally, NSS supports FIPS mode operation in two modes, Level 1 and Level 2.
Level 1 is identified by setting the database password to an empty string.

So, with old NSS (tested 3.28) it was necessary to issue two commands to initialise database to Level 1:

  modutil -dbdir ./nssdb -create -force
  echo > no-pass.txt
  modutil -dbdir nssdb -changepw 'NSS FIPS 140-2 Certificate DB' -newpwfile no-pass.txt -force

With NSS 3.36 second operation fails:

ERROR: Unable to change password on token "NSS FIPS 140-2 Certificate DB".

Comment 1

[Daiki Ueno [:ueno]]

Attachment: nss-modutil-skip-initpw-fips.patch

This patch reverts to the original behavior of modutil -create, that is not to initialize the database password, if the system is in FIPS mode.

Note that this cannot be tested in the upstream test suite, because the automatic enablement of FIPS in a new database is a downstream (RHEL-7) feature:
https://git.centos.org/blob/rpms!nss.git/c7/SOURCES!enable-fips-when-system-is-in-fips-mode.patch

Comment 2

[Daiki Ueno [:ueno]]

Attachment: nss-modutil-skip-changepw-fips.patch

This implements the workaround Kai suggested:
Check if the new requested password is the empty password. Check if FIPS mode is active. If both are true, try to authenticate to the database with the empty password. If that works, do nothing, report success

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 8967401 [details] [diff] [review]
nss-modutil-skip-changepw-fips.patch

Thanks, r=kaie

Comment 4

[Hubert Kario]

Comment on attachment 8967401 [details] [diff] [review]
nss-modutil-skip-changepw-fips.patch

r+

Comment 5

[Daiki Ueno [:ueno]]

Attachment: nss-fipstokn-level-transition.patch

Although the previous patch might be worth for RHEL, I would rather take a different approach for upstream, that is to fix the root cause in softoken.

Currently, C_SetPIN() calls sftk_newPinCheck() regardless of the FIPS level and always rejects empty password.  This patch adds an extra check whether the new password is empty when the FIPS mode is level 1.

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 8967401 [details] [diff] [review]
nss-modutil-skip-changepw-fips.patch

Bob Relyea told us, this isn't ideal. Changing the password "from empty to empty" can be an intended change to rekey the internal keys. This patch would make that impossible.

Comment 7

[Robert Relyea]

Comment on attachment 8968462 [details] [diff] [review]
nss-fipstokn-level-transition.patch

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

Tests looks good.

In the patch we aren't actually calling setpin in the case where we are in level1 and setting the password to "". That's necessary to do the database rekey.

::: lib/softoken/fipstokn.c
@@ +646,4 @@
>      CHECK_FORK();
>  
>      if ((rv = sftk_fipsCheck()) == CKR_OK &&
> +        (isLevel2 || usNewLen > 0) &&

This is the right idea, but it is missing something. We need to call NSC_SetPIN in the case where is where we are in level 1 mode and the new pin is length == 0. This allows a rekey of the database.

The new patch will be a bit more complicated. I think we will need to unwrap this complicated if and break it up into separate calls.

Comment 8

[Daiki Ueno [:ueno]]

Attachment: nss-fipstokn-level-transition-v2.patch

I realized that it is a bit tricky because NSC_SetPIN() also checks the length of new password against slot->minimumPinLen, which is FIPS_MIN_PIN(7) in FIPS mode (either in level1 and level2).

The new patch adds an internal function that takes a flag to skip the length check, but it looks ugly.  Do you have any better suggestions?

Comment 9

[Robert Relyea]

Comment on attachment 8968593 [details] [diff] [review]
nss-fipstokn-level-transition-v2.patch

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

I have some suggestions, though this does work.

::: lib/softoken/fipstokn.c
@@ +664,2 @@
>          }
>      }

I would have been tempted to unroll this if and add a goto error label (would simplify things), but functions correctly and is still under standable.

::: lib/softoken/pkcs11.c
@@ +3779,3 @@
>          crv = CKR_PIN_LEN_RANGE;
>          goto loser;
>      }

Maybe just check to see if ulNewLen ==0 and ulOldLen ==0 and then skip the min check. Then you don't need the ignoreMinimumPinLen. 

If the existing pw is already NULL then it should be OK to set it to NULL (basically rekey). if the existing pw is not NULL and ulOldLen == 0, then we will fail to authenticate with the old pin.

You could also check !(slot->needLogin && (ulNewLen == 0))

Comment 10

[Hubert Kario]

(In reply to Robert Relyea from comment #9)
> Comment on attachment 8968593 [details] [diff] [review]
> nss-fipstokn-level-transition-v2.patch
> 
> Review of attachment 8968593 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: lib/softoken/pkcs11.c
> @@ +3779,3 @@
> >          crv = CKR_PIN_LEN_RANGE;
> >          goto loser;
> >      }
> 
> Maybe just check to see if ulNewLen ==0 and ulOldLen ==0 and then skip the
> min check. Then you don't need the ignoreMinimumPinLen. 

what if the slot->minimumPinLen != 0 and the DB is not in FIPS mode?

wouldn't that allow the user to ignore the policy by setting the password to NULL?

Comment 11

[Robert Relyea]

> what if the slot->minimumPinLen != 0 and the DB is not in FIPS mode?
> wouldn't that allow the user to ignore the policy by setting the password to NULL?

You should only bypass the check if the existing password is NULL and the new password is NULL. In that case it's just a rekey (you aren't actually changing the password). You can check the existing password is NULL either by slot->needLogin == 0 or ulOldLen=0. The former is set to 1 if the current password isn't NULL. The latter indirectly checks if the password is NULL. When we change the password, we'll check that the old password is correct. If it isn't we'll fail. If we supply the old password as NULL, then the password change will only succeed if the actual db password is NULL. 

All other cases we do need to enforce the minimumPinLen.

That was my suggestion in comment 9 (in case it wasn't clear).

Comment 12

[Daiki Ueno [:ueno]]

Attachment: Bug 1453408, fix FIPS level1 -> level2 transition in C_SetPIN

Comment 13

[Daiki Ueno [:ueno]]

Sorry Bob, I completely forgot about this for some reason.

(In reply to Robert Relyea from comment #9)

Maybe just check to see if ulNewLen ==0 and ulOldLen ==0 and then skip the
min check. Then you don't need the ignoreMinimumPinLen.

If the existing pw is already NULL then it should be OK to set it to NULL
(basically rekey). if the existing pw is not NULL and ulOldLen == 0, then we
will fail to authenticate with the old pin.

Yes, this makes sense; I've submitted a new patch on phabricator. Would you mind taking a look at it?

Comment 14

[Daiki Ueno [:ueno]]

Thank you for the review! Pushed as:
https://hg.mozilla.org/projects/nss/rev/221c90384c9a