446592 - Changing a token password with a PIN reader

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nicolas JUSTIN]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.5; Linux) KHTML/3.5.7 (like Gecko) SUSE
Build Identifier: 

In my application I want to change a password token through a PIN reader, but there is a small problem in underlying functions:

 - PK11_ChangePW should accept NULL pointers as oldpw and newpw values since C_SetPIN requires its arguments to be set to NULL if a protected authentication path is in place and wanted to be use.

 - When using PK11_ChangePW through the nsPK11Token::ChangePassword method with both arguments set to 'null', the string convert functions return an empty string instead of a NULL pointer.

(I call the NSS method's directly, I do not use the PSM to change this password; I suppose that the PSM does not support this feature.)

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Nicolas JUSTIN]

Attachment: Proposed patch

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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

Thanks for writing the patch and creating it with cvs diff -u.
This patch changes files in two separate modules that have separate 
groups of reviewers and separate review requirements. So, please 
split it into two patches, one for NSS and one for PSM.  Then we 
can request each patch to be reviewed separately.  Please ask 
rrelyea to review the NSS patch, and kengert@redhat to review the 
PSM patch.

Regarding the nss patch, please change one line:
>+	if (slot->protectedAuthPath == PR_TRUE) {
Change it to just
>+	if (slot->protectedAuthPath) {
or if you prefer
>+	if (slot->protectedAuthPath != PR_FALSE) {

Regarding the PSM patch, the patch adds a couple new lines that
exceed 80 columns in width.  This would not be acceptable in NSS
but might be OK for PSM.  Probably easiest to solve by reducing
the level of indentation.

>-               const_cast<char *>(aUtf8NewPassword.get()));
>+                     (oldPassword != NULL ? const_cast<char *>(aUtf8OldPassword.get()) : NULL), 
>+                     (newPassword != NULL ? const_cast<char *>(aUtf8NewPassword.get()) : NULL));

Comment 3

[Nicolas JUSTIN]

Attachment: NSS only patch, modified as requested

Comment 4

[Robert Relyea]

Comment on attachment 330900 [details] [diff] [review]
NSS only patch, modified as requested

r+

I'm going to r+ this because it does what the bug is asking, whoever I'm wondering how many applications use NULL instead of "" to set the password to empty (softoken treats the password of "" as having no password set).

bob

Comment 5

[Robert Relyea]

bobs-laptop(106) cvs commit pk11auth.c
Checking in pk11auth.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v  <--  pk11auth.c
new revision: 1.11; previous revision: 1.10
done