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

Status: RESOLVED FIXED

(NSS :: Libraries, 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: 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.

Comment 1

[Matt Mueller]

Attachment: track whether we need to close the digest, and do so in SEC_PKCS12DecoderFinish

Comment 2

[Kai Engert [:KaiE:]]

over to NSS

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 4

[Matt Mueller]

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.

Comment 5

[Matt Mueller]

The problem still exists in NSS 3.12.7, and the patch still applies.

Comment 6

[Kai Engert [:KaiE:]]

(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.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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.

Comment 10

[Matt Mueller]

Attachment: nit fixes

Comment 11

[Kai Engert [:KaiE:]]

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!

Comment 12

[Wan-Teh Chang]

Attachment: 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