search

DynamoRIO / drmemory

Memory Debugger for Windows, Linux, Mac, and Android
Other
2.41k stars 256 forks source link

Memory leaks below CryptQueryObject on Chromium net_unittests #475

Open derekbruening opened 9 years ago

derekbruening commented 9 years ago

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

$ tools\valgrind\chrome_tests.bat -t net --tool drmemory --gtest_filter="X509CanParse" (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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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