Uninits in crypto-related code

[derekbruening]

From timurrrr@google.com on May 24, 2011 06:18:59

Repro:

include

include

include

pragma comment(lib, "crypt32.lib")

pragma comment(lib, "advapi32.lib")

define ABORT(msg) do { fprintf(stderr, "Error: %s\n", msg); return 1; } while (0)

int main() { HCRYPTPROV provider; if (!CryptAcquireContext(&provider, NULL, NULL, PROV_RSA_AES, 0)) ABORT("CryptAcquireContext failed");

HCRYPTKEY key; if (!CryptGenKey(provider, CALG_AES_256, CRYPT_EXPORTABLE, &key)) ABORT("CryptGenKey failed");

DWORD buffer_size = 0; if (!CryptExportKey(key, 0, PLAINTEXTKEYBLOB, 0, NULL, &buffer_size)) ABORT("CryptExportKey-1 failed");

BYTE *buffer = new BYTE[buffer_size]; if (!buffer) ABORT("Memory allocation failed");

if (!CryptExportKey(key, 0, PLAINTEXTKEYBLOB, 0, buffer, &buffer_size)) ABORT("CryptExportKey-2 failed");

// 12 = sizeof(BLOBheader) + 4 bytes for the length field std::string str; str.assign((char*)buffer+12, buffer_size-12); printf("BLOB: %s\n", str.c_str());

delete [] buffer; CryptDestroyKey(key); CryptReleaseContext(provider, 0);

printf("PASS\n"); return 0; }

WARNING: unhandled system call #0x1019 WARNING: unhandled system call #0x101e WARNING: unhandled system call #0x102c WARNING: unhandled system call #0x10c8 WARNING: unhandled system call #0x10dc WARNING: unhandled system call #0x10f4 WARNING: unhandled system call #0x1101 WARNING: unhandled system call #0x1142 WARNING: unhandled system call #0x1143 WARNING: unhandled system call #0x1179 WARNING: unhandled system call #0x11b2 WARNING: unhandled system call #0x11e8

result in UNINIT reports on the printf line.

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

[derekbruening]

From timurrrr@google.com on May 24, 2011 03:24:36

Hm, looks like all those win2k3 syscalls are GDI-related

Summary: Uninits in crypto-related code

[derekbruening]

From timurrrr@google.com on May 24, 2011 03:38:51

Also, two uninit reports: system call NtDeviceIoControlFile AFD_LISTEN_DATA.UseSAN system call NtDeviceIoControlFile AFD_LISTEN_DATA.Backlog system call NtDeviceIoControlFile AFD_LISTEN_DATA.UseDelayedAcceptance

Interestingly, the printf report goes away if we only print the first 4 bytes of the payload: str.assign((char*)buffer+12, 4); so at least 4 bytes are initialized (should be: 32).

Maybe there's a "bit count vs byte count" mistake somewhere?

[derekbruening]

From timurrrr@google.com on May 24, 2011 03:49:48

This suppression file makes the test clean except for the printf report: ############################ UNINITIALIZED READ ... ADVAPI32.dll!RegValue

UNINITIALIZED READ ... ADVAPI32.dll!Crypt*

UNINITIALIZED READ ... *!NdrClientCall2

############################

issue #65 UNINITIALIZED READ

... ADVAPI32.dll!SystemFunction036

issue #68 UNINITIALIZED READ

... ntdll.dll!RtlValidateUnicodeString

issue #407 UNINITIALIZED READ

IMM32.dll!* ntdll.dll!LdrInitializeThunk ntdll.dll!LdrShutdownThread

[derekbruening]

From bruen...@google.com on May 24, 2011 06:16:13

In general for unknown syscalls, run with -verbose 2 and extract the nearest symbol name (w/ private pdbs for ntdll, user32, gdi32, and kernel32). Xref the anon script I was using which I pasted into issue #384 (not perfect: would need -thread_logs to remove nondet, but easy to ignore the known syscalls).

[derekbruening]

From timurrrr@google.com on May 24, 2011 06:32:27

(after spending some time reading the sources) and this should be done using a Debug build

[derekbruening]

From timurrrr@google.com on June 02, 2011 05:37:17

One more related code snippet: http://msdn.microsoft.com/en-us/library/aa382042(v=vs.85).aspx and a slightly modified version (attached)

-> gives one leak we haven't seen earlier: 0x68021ed7 <RSAENH.dll+0x21ed7> RSAENH.dll!DllUnregisterServer 0x68011361 <RSAENH.dll+0x11361> RSAENH.dll!CPAcquireContext 0x6800f988 <RSAENH.dll+0xf988> RSAENH.dll!CPReleaseContext 0x6800fb7a <RSAENH.dll+0xfb7a> RSAENH.dll!CPAcquireContext 0x77de7caf <ADVAPI32.dll+0x17caf> ADVAPI32.dll!CryptAcquireContextA 0x77a8aa36 <CRYPT32.dll+0xaa36> CRYPT32.dll!CryptEnumOIDFunction 0x77a98d9e <CRYPT32.dll+0x18d9e> CRYPT32.dll!CryptVerifyCertificateSignatureEx 0x77aa0dfb <CRYPT32.dll+0x20dfb> CRYPT32.dll!I_CryptSetTls 0x77a97796 <CRYPT32.dll+0x17796> CRYPT32.dll!I_CryptInsertLruEntry 0x77a96ff1 <CRYPT32.dll+0x16ff1> CRYPT32.dll!CertFindCertificateInStore 0x77a97540 <CRYPT32.dll+0x17540> CRYPT32.dll!I_CryptInsertLruEntry 0x77a97a33 <CRYPT32.dll+0x17a33> CRYPT32.dll!I_CryptInsertLruEntry 0x77aa0a81 <CRYPT32.dll+0x20a81> CRYPT32.dll!CryptSIPVerifyIndirectData 0x77a98114 <CRYPT32.dll+0x18114> CRYPT32.dll!CertVerifyRevocation 0x77a9801b <CRYPT32.dll+0x1801b> CRYPT32.dll!CertVerifyRevocation 0x77a9795c <CRYPT32.dll+0x1795c> CRYPT32.dll!I_CryptInsertLruEntry 0x77a9755c <CRYPT32.dll+0x1755c> CRYPT32.dll!I_CryptInsertLruEntry 0x77a97a33 <CRYPT32.dll+0x17a33> CRYPT32.dll!I_CryptInsertLruEntry 0x77aa0a81 <CRYPT32.dll+0x20a81> CRYPT32.dll!CryptSIPVerifyIndirectData 0x77a98114 <CRYPT32.dll+0x18114> CRYPT32.dll!CertVerifyRevocation 0x77a9801b <CRYPT32.dll+0x1801b> CRYPT32.dll!CertVerifyRevocation 0x77a98510 <CRYPT32.dll+0x18510> CRYPT32.dll!I_CryptGetLruEntryData 0x77a9831a <CRYPT32.dll+0x1831a> CRYPT32.dll!I_CryptGetLruEntryData 0x77a92fc8 <CRYPT32.dll+0x12fc8> CRYPT32.dll!CertGetCertificateChain 0x0040117f <test.exe+0x117f> test.exe!main test.cpp:113

Attachment: test.cpp

[derekbruening]

From bruen...@google.com on July 15, 2011 12:37:54

after issue #96 and issue #484 , the attached test.cpp on win7 still has a bunch of uninits but I think they're from ntoskrnl. on my xp VM that app doesn't do as much (natively) and has no uninits.

[derekbruening]

From bruen...@google.com on December 02, 2011 07:13:16

xref https://code.google.com/p/dynamorio/issues/detail?id=624

[derekbruening]

From rnk@google.com on August 30, 2012 07:25:12

This is pretty high up on the number of suppressed uninits in full mode, so I took a look at CryptGenKey yesterday, trying to pin down the source of uninit usage.

For Timur's code, I get a bunch of reports through CryptAcquireContext and CryptGenKey: CryptAcquireContext: Dr.M Error #4: UNINITIALIZED READ: reading 0x00cdf294-0x00cdf2a4 16 byte(s) within 0x00cdf294-0x00cdf2a4 Dr.M # 0 RSAENH.dll!AesEncrypt Dr.M # 1 RSAENH.dll!AesCtrRng_Generate Dr.M # 2 RSAENH.dll!AesCtrWithFipsChecks Dr.M # 3 RSAENH.dll!FIPS186GenRandom Dr.M # 4 RSAENH.dll!NTagLogonUser Dr.M # 5 RSAENH.dll!CPAcquireContext Dr.M # 6 CRYPTSP.dll!CryptAcquireContextA Dr.M # 7 CryptoTests_CryptoBasic_Test::TestBody

CryptGenKey: Dr.M Error #5: UNINITIALIZED READ: reading register eax Dr.M # 0 RSAENH.dll!Aes4SboxXmmAsm Dr.M # 1 RSAENH.dll!AesExpandKey Dr.M # 2 RSAENH.dll!AesCtrRng_Update Dr.M # 3 RSAENH.dll!AesCtrRng_Generate Dr.M # 4 RSAENH.dll!AesCtrWithFipsChecks Dr.M # 5 RSAENH.dll!FIPS186GenRandom Dr.M # 6 RSAENH.dll!CPGenKey Dr.M # 7 CRYPTSP.dll!CryptGenKey Dr.M # 8 CryptoTests_CryptoBasic_Test::TestBody

There are some ksec ioctls that look interesting, but they are just 0x390008, which is for generating random numbers, and it has no other pointers.

I turned on -check_uninits_all and got this report inside CryptGenKey: Dr.M Error #36: UNINITIALIZED READ: reading 0x00c8f338-0x00c8f33c 4 byte(s) Dr.M # 0 replace_memcpy [d:\src\drmemory\drmemory\replace.c:205] Dr.M # 1 RSAENH.dll!AesCtrWithFipsChecks Dr.M # 2 RSAENH.dll!FIPS186GenRandom Dr.M # 3 RSAENH.dll!CPGenKey Dr.M # 4 CRYPTSP.dll!CryptGenKey Dr.M # 5 CryptoTests_CryptoBasic_Test::TestBody [d:\src\drmemory\tests\app_suite\crypto_tests_win.cpp:64]

OK, so someone passes in an uninitialized buffer and it gets copied elsewhere. This confirms my assumptions and previous findings.

I have the process that produced the last report in windbg, and looked for where the incriminating buffer (0x00c8f338) came from. The stack with frame pointers is: 00c8f2dc 732d46af rsaenh!AesCtrWithFipsChecks+0x75 00c8f2f4 732db5ea rsaenh!FIPS186GenRandom+0x14 00c8f36c 733149d6 rsaenh!CPGenKey+0x142 00c8f3bc 00037a6f CRYPTSP!CryptGenKey+0x5b

So it's inside the rsaenh!CPGenKey frame. Looking at the parameters on the stack for FIPS186GenRandom, I see this: 0:000> dds 00c8f2f4 00c8f2f4 00c8f36c 00c8f2f8 732db5ea rsaenh!CPGenKey+0x142 00c8f2fc 01114d5c 00c8f300 01114c60 00c8f304 01114c40 00c8f308 00c8f338 # BOOM this is the uninit buffer, arg 4 00c8f30c 00000020 00c8f310 00c8f56c 00c8f314 0110e838 00c8f318 00000000

Here is the code in CPGenKey that does the call to FIPS186GenRandom: rsaenh!CPGenKey+0x1eb: 732ef49a ff7748 push dword ptr [edi+0x48] 732ef49d 8d474c lea eax,[edi+0x4c] # This computes an address on the stack 732ef4a0 50 push eax 732ef4a1 ff75b0 push dword ptr [ebp-0x50] 732ef4a4 ff75b4 push dword ptr [ebp-0x4c] 732ef4a7 ff75c0 push dword ptr [ebp-0x40] 732ef4aa e8ec51feff call rsaenh!FIPS186GenRandom (732d469b) 732ef4af 85c0 test eax,eax 732ef4b1 0f853937ffff jne rsaenh!CPGenKey+0x252 (732e2bf0)

Tracing back, edi comes from:

732ef48e 8b45c8 mov eax,[ebp-0x38] 732ef491 c740480b000000 mov dword ptr [eax+0x48],0xb 732ef498 8bf8 mov edi,eax

Tracing [ebp-0x38]:

732db637 8d45c8 lea eax,[ebp-0x38] # Stack address passed to MakeNewKey 732db63a 50 push eax 732db63b 6a00 push 0x0 732db63d 6a00 push 0x0 732db63f 8d45cc lea eax,[ebp-0x34] 732db642 50 push eax 732db643 ff7508 push dword ptr [ebp+0x8] 732db646 ff75c4 push dword ptr [ebp-0x3c] # Other fields in the same key struct? 732db649 ff75bc push dword ptr [ebp-0x44] 732db64c 53 push ebx 732db64d e869c5ffff call rsaenh!MakeNewKey (732d7bbb)

Not sure exactly what's going on here, but this may be enough info to write a wrapper to silence these for good without suppressions. Unfortunately, I don't think CPGenKey is exported. I'll have to check. It's called from cryptsp.dll, but it looks like a virtual style call from the CryptGenKey code:

CRYPTSP!CryptGenKey+0x4b: 733149c6 8b5d14 mov ebx,[ebp+0x14] 733149c9 53 push ebx 733149ca ff7510 push dword ptr [ebp+0x10] 733149cd ff750c push dword ptr [ebp+0xc] 733149d0 ff7770 push dword ptr [edi+0x70] 733149d3 ff5708 call dword ptr [edi+0x8] # Call to rsaenh!CPGenKey

edi looks like some kind of object.

So, we may be able to identify CPGenKey by wrapping CryptGenKey and waiting for it to make an indirect call to rsaenh. Then we could mark its frame defined from inside a wrapper or something.

[derekbruening]

From bruen...@google.com on August 30, 2012 08:22:56

would be nice to start handling these internally w/o suppression.

xref issue #878 where these show up in mspaint

[derekbruening]

From rnk@google.com on August 30, 2012 09:57:35

For a complete solution, we have a couple of alternatives:

1. Carefully disect rsaenh.dll and wrap the functions that use uninit stack buffers to mark the individual stack frame as wholly defined. This is the most precise, but it takes a lot of effort and will probably be fragile w.r.t. different versions of Windows and updates.
2. Blanket rsaenh.dll suppression. This is the least fragile, but it could have false negatives on data passed in by the user. It's reasonably efficient since we won't take callstacks or symbolize anything.
3. Mark all new memory allocated by stack adjustments in rsaenh.dll as defined instead of undefined. I like this one the most because we'll catch uninits on passed in user data. However, it adds yet another axis of indirection to stack.c, which has many approaches to stack adjustment shadowing that would all have to be updated.

I think 3 is probably the best approach so I'm working on that. As a proof of concept, changing SHADOW_*WORD_UNDEFINED to defined in stack.c silences all uninits in my crypto test case, so this should work.

[derekbruening]

From bruen...@google.com on August 30, 2012 18:00:49

One concern with 3 is leak false positives from pointer-looking data or stale pointers on the stack. This doesn't have to involve a leak scan with rsaenh on the current stack: w/ stack reuse its stack slots can easily end up in other frames below TOS at the time of a later scan.

[derekbruening]

From bruen...@google.com on August 30, 2012 18:30:20

Strike that: the only concern is a leak scan w/ rsaenh on the current stack

[derekbruening]

From rnk@google.com on August 30, 2012 18:30:29

But after returning from rsaenh.dll the frames are marked as unaddressable, so those stale pointers should be considered undefined. Does that mitigate your concerns?

[derekbruening]

From timurrrr@google.com on August 31, 2012 01:26:54

leak false positives from pointer-looking data or stale pointers on the stack you've meant false negatives, I suppose

[derekbruening]

From rnk@google.com on September 04, 2012 10:33:37

https://code.google.com/p/drmemory/source/detail?r=1009 fixes some basic usage, but there are yet more uninits. See this report with -check_uninit_all:

RtlAllocateHeap-post changing from 0x01050d70 to 0x01050d80 MALLOC 0x01050d80-0x01051028 set range 0x01050d80-0x01051028 => 0x3 in event_basic_block(tag=0x732df82a) in event_basic_block(tag=0x732df837) in event_basic_block(tag=0x732df844) in event_basic_block(tag=0x732df870) in event_basic_block(tag=0x732e09b5)

Error #16: UNINITIALIZED READ: reading 0x01050ea8-0x01050eac 4 byte(s)

0 replace_memcpy [d:\src\drmemory\drmemory\replace.c:203](0x73a93130 <drmemorylib.dll+0x293130) modid:10

1 RSAENH.dll!AesCtrWithFipsChecks (0x732d2d78 <RSAENH.dll+0x2d78>) modid:30

2 RSAENH.dll!FIPS186GenRandom (0x732d46af <RSAENH.dll+0x46af>) modid:30

3 RSAENH.dll!random_bytes (0x732e09cb <RSAENH.dll+0x109cb>) modid:30

4 RSAENH.dll!rsa_construction_X931 (0x732df880 <RSAENH.dll+0xf880>) modid:30

5 RSAENH.dll!GenRSAx931KeyPair (0x732df63f <RSAENH.dll+0xf63f>) modid:30

6 RSAENH.dll!ReGenKey (0x732df3f8 <RSAENH.dll+0xf3f8>) modid:30

7 RSAENH.dll!CPGenKey (0x732df35d <RSAENH.dll+0xf35d>) modid:30

8 CRYPTSP.dll!CryptGenKey (0x733149d6 <CRYPTSP.dll+0x49d6>) modid:29

9 CryptoTests_CryptoRsaFull_Test::TestBody [d:\src\drmemory\tests\app_suite\crypto_tests_win.cpp:95](0x0032c4bb <app_suite_tests.exe+0x3c4bb) modid:1

The initial uninit read comes from memcpying data out of this heap allocated buffer.

This is the test case I am using:

TEST(CryptoTests, CryptoRsaFull) { BOOL success; HCRYPTPROV provider; success = CryptAcquireContext(&provider, NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT); ASSERT_TRUE(success) << "CryptAcquireContext failed: " << GetLastError();

HCRYPTKEY key;
DWORD flags = CRYPT_EXPORTABLE;
flags |= (1024 \<< 16);  // Size in bits goes in top 16 bits of flags.
success = CryptGenKey(provider, CALG_RSA_SIGN, flags, &key);
ASSERT_TRUE(success) \<< "CryptGenKey failed: " \<< GetLastError();

CryptDestroyKey(key);
CryptReleaseContext(provider, 0);

}

Really annoying.

Status: Started
Owner: rnk@google.com