351767 - pk12util cmd with -o and -W options crashes if exportfile is a directory

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

I was trying to create a .p12 file from a DBdir containing some certs and 
a private key.  The pk12util program I was using was a debug build, built 
with Windows' debug RTL.  

I intended to create the p12 file with the name outdir/outfile.p12 but I 
mistyped the output filename, omitting the "/outfile.p12", leaving just 
the directory name.  I typed the command like this:

pk12util -n nickname -d DBdir -o outdir -W password -K password

where nickname, DBdir, and the two passwords were the correct/expected 
values, but (again) outdir should have been outdir/outfile.p12.

pk12util output a single line error message, then crashed.  
The error message was:

   pk12util: Initialization failed: outdir: Access Denied.

The code crashed in free().  
The stack was:

free(void * 0x00a61e60) line 926 + 11 bytes
PR_Free(void * 0x00a61e60) line 536 + 10 bytes
P12U_ExportPKCS12Object() line 712 + 13 bytes
main(int 0x0000000b, char * * 0x00a61cd0) line 957 + 28 bytes

The lines of code where the crash occurred were:

    if (slotPw)
        PR_Free(slotPw->data);

    if (p12FilePw)
        PR_Free(p12FilePw->data);   <- crashed here

By trial and error, I found that there are two crucial ingredients
to this crash, which are:

a) the output filename must be a name that will result in a failure
b) the -W option and its argument must be present on the command line

The -K option and its argument are not required.  Without the -K option, 
the user is prompted for the cert DB password before the crash.

The problem is a double-free.

In function P12U_GetP12FilePassword, the value p12FilePw->data is assigned
to a variable named p0, which is then freed.  Then later, the value of 
p12FilePw->data is freed again (at the line shown above) resulting in a
double-free.  

One possible (untested) solution might be this change in P12U_GetP12FilePassword:

    } else { /* Plaintext */
-	p0 = p12FilePw->data;
+	p0 = PORT_Strdup(p12FilePw->data);
    }

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

More observations about this code.

In function P12U_ExportPKCS12Object, at label "loser", the function frees
the password strings 

    if (slotPw)
        PR_Free(slotPw->data);

    if (p12FilePw)
        PR_Free(p12FilePw->data);

This is only done in the error path, not in the success path.  
Notice that the tests are wrong.  They should be
    if (slowPw && slotPw->data)
and
    if (p12FirePw && p12FilePw->data)

But further, since the caller allocates those strings, the caller 
should free them, not this function.  The caller should erase the
strings and free them regardless of whether this function returns
success or failure.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

please try to complete this for 3.11.7

Comment 3

[Neil Williams]

Attachment: fix to tidy up and free calls

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 263488 [details] [diff] [review]
fix to tidy up and free calls

This patch did not address the issues from comment 1 above.

Comment 5

[Neil Williams]

Attachment: errors in previous patched fixed

I have not been able to get the debugger to catch the double free on Solaris--and it doesn't crash. I have verified that without this patch p12FilePw->data gets freed twice.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 263685 [details] [diff] [review]
errors in previous patched fixed


>+	    if (p1 == NULL) {    
>+		PR_Free(p0);    /* doesn't this need to be zero'ed first? */
>+		p0 = NULL;
> 		break;
>+	    }
>+	    if (PL_strcmp(p0, p1) == 0) {
>+		PORT_Memset(p1, 0, PL_strlen(p1));
>+		PORT_Free(p1);
>+		break;
>+	    }
>+	    PR_Free(p0);       /* see comment below */
>+	    PR_Free(p1);
> 	}

If strings p0 and p1 don't match, odds are good that ONE of them is the password
and the other one is CLOSE to the password.  So, both need to be zeroed before
freeing.  PORT_ZFree is probably more concise.

There's probably a way to structure this code so that you only need 
one place where p0 is conditionally zeroed and freed, and likewise only 
one place where p1 is conditionally zeroed and freed.

+    if (slotPw.data != NULL)
+	PORT_Free(slotPw.data);          need to be zeroed first?
+    if (p12FilePw.data != NULL)
+	PORT_Free(p12FilePw.data);       need to be zeroed first?

Perhaps these should be zeroed as soon as we've used them and no longer need them?

Comment 7

[Neil Williams]

Attachment: zero all password memory before freeing

Comment 8

[Neil Williams]

(In reply to comment #6)
> (From update of attachment 263685 [details] [diff] [review])
...
> 
> There's probably a way to structure this code so that you only need 
> one place where p0 is conditionally zeroed and freed, and likewise only 
> one place where p1 is conditionally zeroed and freed.

I couldn't find one. I restructured it slightly so p1 is zeroed and freed once.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 263788 [details] [diff] [review]
zero all password memory before freeing

r=nelson
Thanks.
Requesting second review from Alexei

Comment 10

[Alexei Volkov]

Comment on attachment 263788 [details] [diff] [review]
zero all password memory before freeing

r=alexei.volkov

Any idea why it is needed to have line termination data of secitem pwItem?

>     pwItem = SECITEM_AllocItem(NULL, NULL, PL_strlen(p0) + 1);
                                                          ^^^^^
Looks to me like the line used for password encryption will be a password 
and '\n'.

Comment 11

[Neil Williams]

Checking in cmd/pk12util/pk12util.c;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v  <--  pk12util.c
new revision: 1.32.2.2; previous revision: 1.32.2.1
done

Comment 12

[Neil Williams]

Checking in cmd/pk12util/pk12util.c;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v  <--  pk12util.c
new revision: 1.36; previous revision: 1.35
done

on trunk.