310518 - SEC_ERROR_INVALID_PASSWORD is defined but not used

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

We have two error codes with the same meaning:
SEC_ERROR_BAD_PASSWORD and SEC_ERROR_INVALID_PASSWORD.

Fortunately, SEC_ERROR_INVALID_PASSWORD is defined but
not used.  See
http://lxr.mozilla.org/mozilla/search?string=SEC_ERROR_INVALID_PASSWORD

Should we update our documentation and source code
comments to say SEC_ERROR_INVALID_PASSWORD is deprecated
and you should use SEC_ERROR_BAD_PASSWORD instead?
Anyone remember if SEC_ERROR_INVALID_PASSWORD was
ever used?

Comment 1

[Wan-Teh Chang]

Hmm... perhaps they are different.  One can interpret
SEC_ERROR_INVALID_PASSWORD as a password that does not
meet PKCS #11 or FIPS 140-2 requirements such as it is
not UTF-8, it is too short, or it doesn't contain three
or more classes of characters.

What do you think?

Comment 2

[Wan-Teh Chang]

Two other password related error codes that are
defined but not used:

SEC_ERROR_RETRY_PASSWORD
http://lxr.mozilla.org/mozilla/search?string=SEC_ERROR_RETRY_PASSWORD

SEC_ERROR_RETRY_OLD_PASSWORD
http://lxr.mozilla.org/mozilla/search?string=SEC_ERROR_RETRY_OLD_PASSWORD

Comment 3

[Robert Relyea]

I looks like the 'retry' portions were part of the old libsec ui code, since
it's really an application policy whether it is going to retry the password.

SEC_ERROR_BAD_PASSWORD and SEC_ERROR_INVALID_PASSWORD looks like the former is a
password that was incorrect, and the latter is a password that doesn't meet the
requirements of a password. They map to CKR_PIN_INCORRECT and CKR_PIN_INVALID
respectively. Currently pk11err.c maps both to SEC_ERROR_BAD_PASSWORD.

I think we should change that mapping for the general case (that is
PK11_MapError() should return SEC_ERROR_INVALID_PASSWORD for CKR_PIN_INVALID).

I was initially worried about applications checking the SEC_ERROR_BAD_PASSWORD
and doing something different (like not retrying), but I see NSS does has the
same logic in not retrying CKR_PIN_INCORRECT in it's loop. I verified the
C_Login can only return CKR_BAD_PASSWORD.

PKCS #11 has another define CKR_PIN_LEN_RANGE, which seems to be a special case
of CKR_PIN_INVALID (CKR_PIN_INVALID means you used invalid characters in your
password, CKR_PIN_LEN_RANGE means your password was too short or too long. ).

These definitiona are all suppositions based on which functions return which
error codes and the error code name, none of these are define explicitly in the
PKCS #11 spec AFAIK.

Comment 4

[Wan-Teh Chang]

Attachment: Improve CKR_PIN_XXX error mapping

Should we add a new SEC_ERROR_PASSWORD_LEN_RANGE
error code?

Comment 5

[Wan-Teh Chang]

Bob, PKCS #11 v2.20 says CKR_PIN_INVALID and CKR_PIN_LEN_RANGE
can only be returned by C_InitPIN and C_SetPIN.  I think
C_InitToken can also return these two error codes
if the pPin parameter is the initial SO PIN and
pPin has invalid characters in it or is too long
or too short, but these two error codes are not
documented as possible return values of C_InitToken.

Comment 6

[Robert Relyea]

Comment on attachment 197951 [details] [diff] [review]
Improve CKR_PIN_XXX error mapping

r=relyea.

Comment 7

[Wan-Teh Chang]

Patch checked in.

I don't know what to do about SEC_ERROR_RETRY_PASSWORD
and SEC_ERROR_RETRY_OLD_PASSWORD.  Should we update our
documentation to deprecate them?