584922 - leak of unicodePw SECITEM in nsPKCS12Blob::ExportToFile and nsPKCS12Blob::ImportFromFileHelper

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Matt Mueller]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.14 Safari/534.3
Build Identifier: mozilla-central trunk as of 20100805

nsPKCS12Blob::ExportToFile and nsPKCS12Blob::ImportFromFileHelper allocate SECITEM data for the unicode password, but don't free it.

Reproducible: Always

Comment 1

[Matt Mueller]

Attachment: add SECITEM_FreeItem calls

Comment 2

[Wan-Teh Chang]

Comment on attachment 463384 [details] [diff] [review]
add SECITEM_FreeItem calls

r=wtc.  We found these two leaks during a code review
(and with the help of valgrind).

The 'data' buffer in the unicodePw SECItem is ultimately
allocated by the SECITEM_AllocItem call in unicodeToItem,
via getPKCS12FilePassword or newPKCS12FilePassword.  It
is correct to free unicodePw using SECITEM_FreeItem,
which can handle a null unicodePw.data (an empty password).

Comment 3

[Wan-Teh Chang]

Comment on attachment 463384 [details] [diff] [review]
add SECITEM_FreeItem calls

Requesting approval2.0.  This fixes memory leaks when importing or
exporting a PKCS #12 file.

Kai, does PSM zero a buffer that holds a password before freeing
it?  If so, this patch should use SECITEM_ZfreeItem instead.

Comment 4

[Kai Engert [:KaiE:]]

(In reply to comment #3)
> 
> Kai, does PSM zero a buffer that holds a password before freeing
> it?

This code simply adds SECITEM_FreeItem, and there seems to be no other code that cleans the variables up. So, answer is: in this function, no


>  If so, this patch should use SECITEM_ZfreeItem instead.

agreed, then the patch should be marked as r-

Comment 5

[Kai Engert [:KaiE:]]

Comment on attachment 463384 [details] [diff] [review]
add SECITEM_FreeItem calls

please update the patch as requested by wtc

Comment 6

[Wan-Teh Chang]

Comment on attachment 463384 [details] [diff] [review]
add SECITEM_FreeItem calls

I will change SECITEM_FreeItem to SECITEM_ZfreeItem
when I push this patch to mozilla-central.

Even SECITEM_FreeItem is better than a memory
leak.

Comment 7

[Wan-Teh Chang]

Attachment: add SECITEM_ZfreeItem calls, by Matt Mueller

Comment 8

[Kai Engert [:KaiE:]]

Comment on attachment 463759 [details] [diff] [review]
add SECITEM_ZfreeItem calls, by Matt Mueller

r=kaie

Comment 9

[(no longer active)]

This requires approval in order to land on mozilla-central.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 463759 [details] [diff] [review]
add SECITEM_ZfreeItem calls, by Matt Mueller

a=beltzner

Comment 11

[Kai Engert [:KaiE:]]

http://hg.mozilla.org/mozilla-central/rev/b4f2c723731d

Comment 12

[u279076]

Is there a clear way to reproduce this from a user perspective? What's the easiest way to verify this bug is fixed?

Comment 13

[Wan-Teh Chang]

Anthony: this fix can only be verified by code review or a
memory leak tool such as valgrind or Purify.  Sorry.

Comment 14

[u279076]

Ok, marking verified based on code review.  If someone wants to go to the trouble of verifying fixed with Valgrind or Purify, by all means.