calling SEC_PKCS12DecoderStart with NULL dOpen, dClose, dRead, dWrite, dArg leads to leaks

RESOLVED FIXED in 3.12.8

Status

NSS
Libraries
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Matt Mueller, Assigned: Matt Mueller)

Tracking

({memory-leak})

unspecified
3.12.8
memory-leak

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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: both NSS 3.12.3.1 & hg mozilla-central trunk (as of 20100804)

When PKCS #12 decoder is used with the internal memory-backed file implementation, the buffer is not freed in all cases.

Reproducible: Always

Steps to Reproduce:
Initialize the decoder by passing NULL for all the dOpen, dClose, dRead, dWrite, dArg params.

Run under valgrind.
Actual Results:  
Leak_DefinitelyLost
4,096 bytes in 1 blocks are definitely lost in loss record 777 of 782
  malloc (algrind-for-chromium-client/valgrind/scripts/valgrind-memcheck/coregrind/m_replacemalloc/vg_replace_malloc.c:236)
  malloc (base/process_util_linux.cc:607)
  PR_Malloc (uild/buildd/nspr-4.7.5/mozilla/nsprpub/pr/src/malloc/prmem.c:467)
  PORT_Alloc_Util (mp/nss.y17059/nss-3.12.3.1/mozilla/security/nss/lib/util/secport.c:113)
  p12u_DigestOpen (mp/nss.y17059/nss-3.12.3.1/mozilla/security/nss/lib/pkcs12/p12d.c:1056)
  sec_pkcs12_decoder_pfx_notify_proc (mp/nss.y17059/nss-3.12.3.1/mozilla/security/nss/lib/pkcs12/p12d.c:911)
  sec_asn1d_notify_before (mp/nss.y17059/nss-3.12.3.1/mozilla/security/nss/lib/util/secasn1d.c:451)
  SEC_ASN1DecoderUpdate_Util (mp/nss.y17059/nss-3.12.3.1/mozilla/security/nss/lib/util/secasn1d.c:2016)
  SEC_PKCS12DecoderUpdate (mp/nss.y17059/nss-3.12.3.1/mozilla/security/nss/lib/pkcs12/p12d.c:1283)
  mozilla_security_manager::(anonymous namespace)::nsPKCS12Blob_ImportHelper(char const*, unsigned int, std::basic_string<unsigned short, base::string16_char_traits, std:
:allocator<unsigned short> > const&, bool, PK11SlotInfoStr*) (net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:206)
  mozilla_security_manager::nsPKCS12Blob_Import(char const*, unsigned int, std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> > 
const&) (net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:298)
  net::CertDatabase::AddUserCertFromPKCS12(char const*, unsigned int, std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> > const
&) (net/base/cert_database_nss.cc:100)
  net::CertDatabaseNSSTest_ImportFromPKCS12WrongPassword_Test::TestBody() (net/base/cert_database_nss_unittest.cc:65)
  testing::Test::Run() (testing/gtest/src/gtest.cc:2095)
  testing::internal::TestInfoImpl::Run() (testing/gtest/src/gtest.cc:2314)
  testing::TestCase::Run() (testing/gtest/src/gtest.cc:2420)
  testing::internal::UnitTestImpl::RunAllTests() (testing/gtest/src/gtest.cc:4024)
  testing::UnitTest::Run() (testing/gtest/src/gtest.cc:3687)
  TestSuite::Run() (./base/test/test_suite.h:152)
  main (net/base/run_all_unittests.cc:27)


Expected Results:  
No leaks.

The buffer is only freed when the close function is called with the second arg TRUE, but that only happens from from sec_pkcs12_decoder_verify_mac, but that won't be called in some error cases, or when the pkcs12 blob uses a signature instead of mac.
(This may also affect caller implemented file functions, depending how they are handling the close operation.)

Attaching a patch to call the close function in SEC_PKCS12DecoderFinish if necessary.
(Assignee)

Comment 1

8 years ago
Created attachment 463312 [details] [diff] [review]
track whether we need to close the digest, and do so in SEC_PKCS12DecoderFinish

Comment 2

8 years ago
over to NSS
Assignee: nobody → nobody
Status: UNCONFIRMED → NEW
Component: Security: PSM → Libraries
Ever confirmed: true
Product: Core → NSS
QA Contact: psm → libraries
Did you test with current NSS code from NSS 3.12.7?
Or were you testing with old code in Mozilla's downstream Hg repository?
(Assignee)

Comment 4

8 years ago
I was testing with the code in hg.mozilla.org/mozilla-central.  The 3.12.7 tarball was not released at the time.   I'll have a look at that now.

Where is the current NSS repository? http://www.mozilla.org/projects/security/pki/nss/index.html#mozillacvs mentions CVS but doesn't actually point to any repo, so I assumed it was just out of date docs and the current code is in mercurial now.
(Assignee)

Comment 5

8 years ago
The problem still exists in NSS 3.12.7, and the patch still applies.
OS: Linux → Windows CE

Updated

8 years ago
OS: Windows CE → All
Hardware: x86 → All

Comment 6

8 years ago
(In reply to comment #4)
> 
> Where is the current NSS repository?

Matt, upstream NSS still uses the old CVS infrastructure of mozilla.
https://developer.mozilla.org/en/Mozilla_Source_Code_Via_CVS

The code in Mozilla's mercurial are snapshot copies.
I'm glad to learn that the patch still applies. 
A bunch of work was done to pkcs#12 code to plug lots of leaks.
That work was done only in the CVS upstream repository (at the time).
Now that 3.12.7 is released, does the leak you observed still occur in it?
I may have confused this bug with bug 584875, but the same questions really
apply to both.  Do these bugs still occur with 3.12.7?  You've answered that
for bug 584875, so it remains for this bug.

Comment 9

8 years ago
Comment on attachment 463312 [details] [diff] [review]
track whether we need to close the digest, and do so in SEC_PKCS12DecoderFinish

This leak is in the NSS CVS trunk.

We should take the opportunity to document how p12d.c
calls these d* callback functions.  For example, based
on about half an hour of browsing the code, I think this
is how they're called:

Stage 1: decode the aSafes cinfo into a buffer in dArg,
which p12d.c sometimes refers to as the "temp file".
I think this occurs during SEC_PKCS12DecoderUpdate calls.

dOpen(dArg, PR_FALSE)
dWrite(dArg, buf, len)
...
dWrite(dArg, buf, len)
dClose(dArg, PR_FALSE)

I think this occurs during SEC_PKCS12DecoderUpdate calls.
So whether dOpen(dArg, PR_FALSE) has been called seems
unobservable by the caller externally.  This means adding
a boolean flag as mattm suggested to track this internally
seems necessary.

Stage 2: verify MAC
dOpen(dArg, PR_TRUE)
dRead(dArg, buf, IN_BUF_LEN)
...
dRead(dArg, buf, IN_BUF_LEN)
dClose(dArg, PR_TRUE)

This occurs SEC_PKCS12DecoderVerify.

It seems that we can pass PR_FALSE to the second
dClose call, and then add a dClose(dArg, PR_TRUE)
call to SEC_PKCS12DecoderFinish as mattm suggested.

I have two nit-picking comments about mattm's patch.

>     digestCloseFn 		dClose;
>     digestIOFn 			dRead, dWrite;
>     void 			*dArg;
>+    PRBool                      digestIsOpen;

What's open is not the digest, but rather a buffer
(the "temp file") holding decoded data.  I suggest
naming this flag "dIsOpen" to dodge the issue of
naming the thing that's open, and to match the d*
style of the related fields, or "dFileIsOpen"
to match the "removeFile" parameter of dClose.

>@@ -915,6 +916,9 @@
> 	p12dcx->errorValue = PORT_GetError();
> 	goto loser;
>     }
>+    else {
>+      p12dcx->digestIsOpen = PR_TRUE;
>+    }

Don't use "else" after a goto statement.
(Assignee)

Comment 10

8 years ago
Created attachment 464211 [details] [diff] [review]
nit fixes

Comment 11

8 years ago
Comment on attachment 464211 [details] [diff] [review]
nit fixes

Matt, you should set the "review ?" flag each time you add a new patch, to ensure it gets the attention. Also please enter the email address of the requested reviewer, like this. Thanks!
Attachment #464211 - Flags: review?(wtc)

Comment 12

8 years ago
Created attachment 471296 [details] [diff] [review]
track whether we need to close the digest, and do so in SEC_PKCS12DecoderFinish, v3, by Matt Mueller

I checked in mattm's patch, with additional comments, on
the NSS trunk (NSS 3.13) and NSS_3_12_BRANCH (NSS 3.12.8).

Checking in p12d.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v  <--  p12d.c
new revision: 1.49; previous revision: 1.48
done

Checking in p12d.c;
/cvsroot/mozilla/security/nss/lib/pkcs12/p12d.c,v  <--  p12d.c
new revision: 1.48.2.1; previous revision: 1.48
done
Attachment #463312 - Attachment is obsolete: true
Attachment #464211 - Attachment is obsolete: true
Attachment #464211 - Flags: review?(wtc)

Updated

8 years ago
Assignee: nobody → mattm
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12.8

Updated

8 years ago
Keywords: mlk
You need to log in before you can comment on or make changes to this bug.