Closed Bug 550435 Opened 11 years ago Closed 11 years ago

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

Categories

(NSS :: Libraries, defect)

x86
All
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
3.12.7

People

(Reporter: timeless, Assigned: timeless)

References

()

Details

(Keywords: coverity)

Attachments

(1 file)

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);
Attached patch proposalSplinter Review
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #430586 - Flags: review?(nelson)
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.
Attachment #430586 - Flags: superreview?(rrelyea)
Attachment #430586 - Flags: review?(nelson)
Attachment #430586 - Flags: review+
Comment on attachment 430586 [details] [diff] [review]
proposal

The new code is correct (and cleaner).

bob
Attachment #430586 - Flags: superreview?(rrelyea) → superreview+
Checking in pk11auth.c; new revision: 1.12; previous revision: 1.11

Thanks, Timeless
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.7
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
looks like it did :(, *sigh*
Summary: PK11_ChangePW passes newLen/oldLen uninitialized to C_SetPIN if slot->protectedAuthPath and newpw/oldpw == NULL → PK11_ChangePW passes newLen/oldLen uninitialized to C_SetPIN if slot->protectedAuthPath and newpw/oldpw != NULL
Duplicate of this bug: 581582
You need to log in before you can comment on or make changes to this bug.