550435 - PK11_ChangePW passes newLen/oldLen uninitialized to C_SetPIN if slot->protectedAuthPath and newpw/oldpw != NULL

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[timeless]

478 PK11_ChangePW(PK11SlotInfo *slot, const char *oldpw, const char *newpw)
482     int newLen;
483     int oldLen;

487     if (slot->protectedAuthPath) {
488         if (newpw == NULL) newLen = 0;
489         if (oldpw == NULL) oldLen = 0;

505     crv = PK11_GETTAB(slot)->C_SetPIN(rwsession,
506                 (unsigned char *)oldpw,oldLen,(unsigned char *)newpw,newLen);

Comment 1

[timeless]

Attachment: proposal

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 430586 [details] [diff] [review]
proposal

I'm not 100% sure, but I think that when slot->protectedAuthPath is true, 
then the function C_SetPin is supposed to completely ignore the pw arguments.
We expect both oldPW and newPW to be NULL in that case.  Maybe we should 
force them both to be NULL in that case.  

I think this patch is at least as correct as the code is replaces, so
I'm giving it r+, but I'm asking Bob to SR, to answer the question of
whether we should force the PW strings to be NULL in that case.

Comment 3

[Robert Relyea]

Comment on attachment 430586 [details] [diff] [review]
proposal

The new code is correct (and cleaner).

bob

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Checking in pk11auth.c; new revision: 1.12; previous revision: 1.11

Thanks, Timeless

Comment 5

[Wan-Teh Chang]

timeless: does the summary of this bug have a typo?

It seems that it should say:
    PK11_ChangePW passes newLen/oldLen uninitialized
    to C_SetPIN if slot->protectedAuthPath and
    newpw/oldpw != NULL

because if newpw/oldpw == NULL, the original code
will set newLen/oldLen to 0.  See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11auth.c&rev=1.11&mark=478,482-483,487-489#477

Comment 6

[timeless]

looks like it did :(, *sigh*