556505 - SECU_GetPasswordString, SECU_ChangePW2 and secu_InitSlotPassword are strange

Status: NEW

(NSS :: Tools, defect, P3)

Description

[timeless]

340 secu_InitSlotPassword(PK11SlotInfo *slot, PRBool retry, void *arg)

342     char *p0 = NULL;
344     FILE *input, *output;
356 #ifdef _WINDOWS
357     input = stdin;
358 #else
359     input = fopen(consoleName, "r");
360 #endif

373     if (output == NULL) {
375         return NULL;

this leaks input on ! _WINDOWS

398     fclose(input);

this closes stdin on _WINDOWS

399     fclose(output);
401     return p0;

411 SECU_ChangePW2(PK11SlotInfo *slot, char *oldPass, char *newPass,
416     char *oldpw = NULL, *newpw = NULL;

440     if (PK11_NeedUserInit(slot)) {
441         newpw = secu_InitSlotPassword(slot, PR_FALSE, &pwdata);

this can fail (See 375), but it's passed to PK11_InitPin which treats 0 as "\0"

442         rv = PK11_InitPin(slot, (char*)NULL, newpw);

Comment 1

[timeless]

161 SECU_GetPasswordString(void *arg, char *prompt)
163 #ifndef _WINDOWS
165     FILE *input, *output;
168     input = fopen(consoleName, "r");

175     if (output == NULL) {
177         return NULL;

leaks input

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Assigning to Shailendra, who's looking for bugs to fix, if I'm not mistaken.

Comment 3

[Shailen]

Attachment: Patch V 1

Hi Nelson,

  Please review the patch.

  I guess I wasnt sure about the below change.

+       if (newpw == NULL) {
+           rv = PK11_InitPin(slot, (char*)NULL, (char*)NULL);
+       } else {
+           rv = PK11_InitPin(slot, (char*)NULL, newpw);
+       }

Regards,
Shailendra

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 436659 [details] [diff] [review]
Patch V 1

Shailendra, your patch was all fine except for this part.

> 	newpw = secu_InitSlotPassword(slot, PR_FALSE, &pwdata);
>-	rv = PK11_InitPin(slot, (char*)NULL, newpw);
>+	if (newpw == NULL) {
>+	    rv = PK11_InitPin(slot, (char*)NULL, (char*)NULL);
>+	} else {
>+	    rv = PK11_InitPin(slot, (char*)NULL, newpw);
>+	}

That patch makes the code behave no differently than the old code did when secu_InitSlotPassword returns NULL.  The crucial point is to NOT call 
PK11_InitPin with a NULL last argument, but to instead report an error 
and return SECFailure, as done elsewhere in that same function.

Comment 5

[Shailen]

Attachment: Patch Version 2

Hi Nelson,

  Please review the newly attached patch for error message.

  Also please let me if we need to have similar check for the below piece of code which comes after the place where the above patch is applied

       newpw = secu_InitSlotPassword(slot, PR_FALSE, &newpwdata);

    if (PK11_ChangePW(slot, oldpw, newpw) != SECSuccess) {
        PR_fprintf(PR_STDERR, "Failed to change password.\n");
        return SECFailure;
    }

Regards,
Shailendra

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 436910 [details] [diff] [review]
Patch Version 2

This patch is VERY close to being right.

First, in answer to your question, YES, both places where 
secu_InitSlotPassword is called need to be fixed.  

Second, We need to have a different message for the case 
where secu_InitSlotPassword failed to get a password (not
even an empty password) and returned NULL) than for the 
case where the function that followed it (e.g. PK11_InitPin 
or PK11_ChangePW) failed.  If secu_InitSlotPassword fails,
we should say something like "failed to get new password"
and if the other function fails, we should say "failed to 
set password" or "failed to change password" (as we do now).

Comment 7

[Shailen]

Attachment: Patch Version 3

Hi Nelson,

     I modified the code and can you please review this patch.

Thanks,
Shailendra

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 439873 [details] [diff] [review]
Patch Version 3


>     if (PK11_NeedUserInit(slot)) {
> 	newpw = secu_InitSlotPassword(slot, PR_FALSE, &pwdata);
>+	if (newpw == NULL) {
>+	    PR_fprintf(PR_STDERR, "Failed to get new password.\n");
>+	    return SECFailure;
>+	}
> 	rv = PK11_InitPin(slot, (char*)NULL, newpw);

What if this fails?

> 	goto done;
>     }

Comment 9

[Shailen]

Attachment: Patch Version 4

Hi nelson,

      I modified the patch to take care of the error condition you mentioned.

      Please review and let me know if any further changes required.

Thanks and Regards,
Shailendra

Comment 10

[timeless]

Comment on attachment 440159 [details] [diff] [review]
Patch Version 4

>     if (PK11_NeedUserInit(slot)) {
> 	newpw = secu_InitSlotPassword(slot, PR_FALSE, &pwdata);
...
> 	rv = PK11_InitPin(slot, (char*)NULL, newpw);
>+	if (rv == SECFailure) {
this leaks newpw:
>+	    PR_fprintf(PR_STDERR, "Failed to set new password.\n");
>+	    return SECFailure;

>     newpw = secu_InitSlotPassword(slot, PR_FALSE, &newpwdata);
...
>     if (PK11_ChangePW(slot, oldpw, newpw) != SECSuccess) {
this leaks newpw:
> 	PR_fprintf(PR_STDERR, "Failed to change password.\n");
> 	return SECFailure;

Please consider fixing these too.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.