Memory leaks below CryptQueryObject on Chromium net_unittests

[derekbruening]

From timurrrr@google.com on June 24, 2011 09:46:01

$ tools\valgrind\chrome_tests.bat -t net --tool drmemory --gtest_filter="X509_CanParse_"
(Chromium r90363 )

w/o symbols
LEAK 8 direct bytes 0x00175748-0x00175750 + 488 indirect bytes
#1 DllUnregisterServer RSAENH.dll+0x21ed7
#2 CPAcquireContext RSAENH.dll+0x11361
#3 CPReleaseContext RSAENH.dll+0xf988
#4 CPAcquireContext RSAENH.dll+0xfb7a
#5 CryptAcquireContextA ADVAPI32.dll+0x17caf
#6 CryptEnumOIDFunction CRYPT32.dll+0xaa36
#7 CryptMsgOpenToDecode CRYPT32.dll+0x1fb97
#8 CertAddEncodedCRLToStore CRYPT32.dll+0x2d3fe
#9 CryptQueryObject CRYPT32.dll+0x2541c
#10 net::anonymous namespace'::ParsePKCS7 c:\chromium\src\net\base\x509_certificate_win.cc:458 \#11 net::X509Certificate::CreateOSCertHandlesFromBytes c:\chromium\src\net\base\x509_certificate_win.cc:968 \#12 net::X509Certificate::CreateCertificateListFromBytes c:\chromium\src\net\base\x509_certificate.cc:329 \#13 net::CreateCertificateListFromFile c:\chromium\src\net\base\x509_certificate_unittest.cc:182 \#14 net::X509CertificateParseTest_CanParseFormat_Test::TestBody c:\chromium\src\net\base\x509_certificate_unittest.cc:1048 \---- with symbols LEAK 8 direct bytes 0x00175748-0x00175750 + 488 indirect bytes \# 1 ContAlloc RSAENH.dll+0x21ed7 \# 2 NTLMakeItem RSAENH.dll+0x11361 \# 3 NTagLogonUser RSAENH.dll+0xf988 \# 4 CPAcquireContext RSAENH.dll+0xfb7a \# 5 CryptAcquireContextA ADVAPI32.dll+0x17caf \# 6 I_CryptGetDefaultCryptProv CRYPT32.dll+0xaa36 \# 7 CryptMsgOpenToDecode CRYPT32.dll+0x1fb97 \# 8 I_CryptQueryObject CRYPT32.dll+0x2d3fe \# 9 CryptQueryObject CRYPT32.dll+0x2541c \#10 net::anonymous namespace'::ParsePKCS7 c:\chromium\src\net\base\x509_certificate_win.cc:458
#11 net::X509Certificate::CreateOSCertHandlesFromBytes c:\chromium\src\net\base\x509_certificate_win.cc:968
#12 net::X509Certificate::CreateCertificateListFromBytes c:\chromium\src\net\base\x509_certificate.cc:329
#13 net::CreateCertificateListFromFile c:\chromium\src\net\base\x509_certificate_unittest.cc:182
#14 net::X509CertificateParseTest_CanParseFormat_Test::TestBody c:\chromium\src\net\base\x509_certificate_unittest.cc:1048

Original issue: http://code.google.com/p/drmemory/issues/detail?id=475

[derekbruening]

From timurrrr@google.com on June 24, 2011 07:33:35

Reproducible on the attached repro (extracted from Chromium sources).

Ryan, does this leak report look like a false positive to you?

Cc: rsleevi@chromium.org
Labels: Bug-FalsePositive

Attachment: test.cpp

[derekbruening]

From rsleevi@chromium.org on June 24, 2011 20:59:45

timur: Is there a getting started page for Dr. Memory + Chrome to run these tests?

With the repro you provided, there is a leak. If you were to look at the results of CryptCloseStore (a BOOL), you should find it's failing. The reason is that the OSCertHandles in |results| are not being freed.

Lines 128-135 allocate the new handle (by inserting into the new store), which is stored in |results| at lines 141 / 163.

Update lines 163+164 to read

AddCertsFromStore(out_store, &results);
for (size_t i = 0; i < results.size(); ++i) {
CertFreeCertificateContext(results[i]);
}
CertCloseStore(out_store, CERT_CLOSE_STORE_CHECK_FLAG);

With that updated code, does Dr. Memory still report a leak?

[derekbruening]

From timurrrr@google.com on June 27, 2011 10:15:01

timur: Is there a getting started page for Dr. Memory + Chrome to run these tests?
Yes, http://dev.chromium.org/developers/how-tos/using-valgrind/dr-memory but it works well only on Windows XP at the moment.

I'll have a look at the proposed fix tomorrow (had to stay at home for a few days)

[derekbruening]

From rsleevi@chromium.org on July 01, 2011 18:06:35

timurrrr: I still see the errors with my suggested fix applied, so I don't think it was related to the cert leaking (although it doesn't hurt)

I'm wondering now if it's related somehow to the function CryptInstallDefaultContext - http://msdn.microsoft.com/en-us/library/aa380213(VS.85).aspx . Based on the symbolicated callstacks I see on Win XP and Win 7, it appears as if the leak is related to the acquisition of the default HCRYPTPROV used to perform cryptographic verification.

This would suggest a false positive, because what I suspect is happening is CryptoAPI is installing itself (or more aptly, the Microsoft CSP) without setting CRYPT_DEFAULT_CONTEXT_AUTO_RELEASE_FLAG.

Plausible?

[derekbruening]

From bruen...@google.com on July 07, 2011 07:40:52

can you track down where it's being stored and why the leak scan is missing it: perhaps tweak the repro to pass the *_RELEASE_FLAG and watch where it pulls the pointer from.

[derekbruening]

From timurrrr@google.com on August 09, 2011 09:08:50

The same leak is reported on a smaller repro which does just CryptQueryObject and then CertCloseStore. [attached]

Interestingly, there is just one leak if I loop through the main() contents 5 times

Status: Started
Owner: timurrrr@google.com

Attachment: 475.cpp

[derekbruening]

From timurrrr@google.com on August 09, 2011 09:13:41

Derek, can you please check my reasoning? http://read.pudn.com/downloads3/sourcecode/crypt/11366/ntcrypto/inc/manage.h__.htm BOOL NTLMakeItem(HCRYPTKEY *phKey, BYTE bTypeValue, void *NewData);

windbg:
rsaenh!NTLMakeItem:
68011355 8bff mov edi,edi
68011357 55 push ebp
68011358 8bec mov ebp,esp
6801135a 6a08 push 0x8
6801135c e85f0b0100 call rsaenh!ContAlloc (68021ec0) # the allocated address i
68011361 85c0 test eax,eax
68011363 7505 jnz rsaenh!NTLMakeItem+0x15 (6801136a) # eax is non-zero, skipping 3 instrs
#68011365 6a08 push 0x8
#68011367 58 pop eax
#68011368 eb18 jmp rsaenh!NTLMakeItem+0x2d (68011382)
6801136a 8b4d10 mov ecx,[ebp+0x10] # take NewData pointer ...
6801136d 8908 mov [eax],ecx # and put it as the 1st field
6801136f 0fb64d0c movzx ecx,byte ptr [ebp+0xc] # take bTypeValue ...
68011373 894804 mov [eax+0x4],ecx # and write it as the 2nd field
68011376 8b4d08 mov ecx,[ebp+0x8] # now take phKey
68011379 352c175ae3 xor eax,0xe35a172c # mask the ContAlloc return value (why?!?)
6801137e 8901 mov [ecx],eax # and write it to *phKey
68011380 33c0 xor eax,eax
68011382 5d pop ebp
68011383 c20c00 ret 0xc

=> so even if the chunk is still addressable [e.g. it's a singleton analog], we can't find it during our leak scan

[derekbruening]

From bruen...@google.com on August 09, 2011 10:14:22

probably it's being encoded to add some simple security.
this is a common mechanism: since Vista, part of the heap headers are xor-ed with a cookie to make it that little bit harder to exploit.
more elaborately encrypted pointers also exist: xref issue #153 .
I long ago listed encrypted pointers as a source of possible false positives in the drmem documentation.

[derekbruening]

From timurrrr@google.com on August 10, 2011 03:27:00

I think we should just suppress them then...

btw, on Win7 the code is basically the same:

rsaenh!NTLMakeItem:
...
74ed1510 e84f030000 call rsaenh!ContAlloc (74ed1864)
74ed1515 85c0 test eax,eax
74ed1517 0f847efb0100 je rsaenh!NTLMakeItem+0x10 (74ef109b)
74ed151d 8b4d10 mov ecx,[ebp+0x10]
74ed1520 8908 mov [eax],ecx
74ed1522 0fb64d0c movzx ecx,byte ptr [ebp+0xc]
74ed1526 894804 mov [eax+0x4],ecx
74ed1529 8b4d08 mov ecx,[ebp+0x8]
74ed152c 352c175ae3 xor eax,0xe35a172c
74ed1531 8901 mov [ecx],eax