174135 - certutil -N doesn't check passwords for validity

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

certutil -N tells the user:

> In order to finish creating your database, you
> must enter a password which will be used to
> encrypt this key and any future keys.
>
> The password must be at least 8 characters long,
> and must contain at least one non-alphabetic 
> character.

but then certutil accepts any password, even an empty string.

The responsible function for checking passwords is SEC_BlindCheckPassword.
All it does is check that the input pointer argument is not null.
It does not check even a single character in the password string.

at the very least, I think it should check that the first character is 
not null.  

 PRBool SEC_BlindCheckPassword(char *cp)
 {
-    if (cp != NULL) {
+    if (cp != NULL && cp[0] != 0) {
         return PR_TRUE;
     }
     return PR_FALSE;
 }

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Any objection to requiring passwords contain at least one non-null character?

Comment 2

[Wan-Teh Chang]

I suggest that we implement what the certutil message
says:

  The password must be at least 8 characters long,
  and must contain at least one non-alphabetic 
  character.

#include <ctype.h>

PRBool SEC_BlindCheckPassword(char *cp)
{
    PRBool nonalpha = PR_FALSE;
    int i;

    if (cp != NULL) {
        for (i = 0; cp[i] != '\0'; i++) {
            if (!isalpha(cp[i])) {
                nonalpha = PR_TRUE;
            }
        }
        if ((i >= 8) && nonalpha) {
            return PR_TRUE;
        }
    }
    return PR_FALSE;
}

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

If we actually enforce the password constraints, our QA scripts will stop
working, since they use the word "test" (if I recall correctly) as the 
password.  Of course, the scripts can be changed.  

Many NSS users already have key DBs with passwords that do not meet these
criteria.  I'd want to be certain that this change would not suddenly
cause those legacy DBs to become inaccessible with NSS test programs like
certutil.  

As soon as we reach consensus about the right thing to do here, I will 
work on implementing it.

Comment 4

[Wan-Teh Chang]

Just to clarify: I do not object to requiring passwords
contain at least one non-null character.

Comment 5

[Robert Relyea]

For the internal database, nelson's check should not be a problem. If someone
set their password to the empty string, then the database would interpret this
as having no password, so we shouldn't have a problem rejecting the NULL string
in SEC_BlindCheckPassword for the internal database. I don't know about ""
passwords and tokens. My guess is they would be extremely rare, so I would be in
favor of this change.

Wan-Teh's type of check should be in the code that sets the password, not the
code that gets called to create the database. PKCS #11 already has ways of
specifying password quality on a per-token basis. Some tokens would not work
with a long password. That check is made when we try to set the password on the
database. There are configuration calls the application can make which will tell
the softoken to specify more restrictive password checks -- though the clients
that use NSS have chosen to use a visual password quality meter rather than
strict enforcement of password quality.

bob

Comment 6

[Julien Pierre]

We do have many applications and users with key dbs that have passwords which
don't meet our recommended criteria.

On the other hand, I really like to be able to type "ENTER / ENTER" when
prompted for the password of the new key db in certutil, when creating a DB for
testing purposes.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Bob, I'm trying to understand comment #5.  

Are you saying that you think that certutil -N should NOT do the more 
restrictive check?

SEC_BlindCheckPassword is part of libsecutil.  
That is, it's only part of our commands, not part of the NSS shared libraries.
The change I'm proposing affects only programs that link with libsecutil,
and I don't think that includes any of the servers or browsers.  

I think the question we really need to answer is: 
how much password pain do we want to inflict on ourselves?  :)

Comment 8

[Robert Relyea]

I'm in favor of the "check for empty" case because I don't think we are likely
to run into the case where we want to deal with a token that has an empty
password with our tools (that's the only case I see a drawback on).

If we want to do more strict password checks on what we set the password to, we
should use the mechanism already built into NSS to inforce the stricter checks
(that way we are testing that code as well). We should always be able to open
databases that had less strict checks on it because the NSS tools are often used
to manipulate, diagnose, and/or fix databases created by other applications. The
"check for empty" shouldn't cause a problem here because a database with an
empty password will look to NSS as a database without any password (keys in a
'no-password' database are encrypted with "" as the password passed to the PBE).

One question is SEC_BlindPasswordCheck() used to verify passwords before we
authenticate to tokens, or is it just used to verify passwords we are setting or
changing to? I've been assuming the former.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

We do it in both.  

SEC_BlindCheckPassword is called from exactly 2 other functions, 
SECU_GetPasswordString and secu_InitSlotPassword, both in cmd/lib/secutil.c.  
SECU_GetPasswordString gets called from all over.   It gets called from
code that wants the user to enter a new password (where it is requested 
twice) and from code that wants the user to enter an old password (requested
once).   

Clearly, we don't want to be strict when asking for an old password.
If we want to be strict at all, it is when asking for a new password.

To really fix this right, I guess we need two versions of 
SECU_GetPasswordString and two of SEC_BlindCheckPassword, one pair for
use in new password situations, and the other pair for use in old password
situations.  I'll code up a patch that does that.

Or do we just want to drop the requirement, and remove the false message
about the password content requirements?

Comment 10

[Wan-Teh Chang]

How about replacing "must" by "should" in the message?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

I changed the message displayed by certutil -N to

Enter a password which will be used to encrypt your keys.
The password should be at least 8 characters long,
and should contain at least one non-alphabetic character.