Problems with memory reading inside injected dll (VirtualProtect)

[Jon Willis]

Hi all,

(deep breath)

So, I have an injected dll that is constantly looping the following code:

long startAddress = atol(szRecvBufferSplit);
szRecvBufferSplit = strtok(NULL, "|");
int numberOfBytes = atoi(szRecvBufferSplit);

MEMORY_BASIC_INFORMATION mbi;

if(VirtualQuery((LPCVOID)startAddress, &mbi, sizeof(mbi)))
{
if (&mbi && mbi.State == MEM_COMMIT && mbi.Protect != PAGE_NOACCESS && mbi.Protect != PAGE_GUARD)
{
if(VirtualProtect((LPVOID)startAddress, numberOfBytes, PAGE_EXECUTE_READWRITE, &dwOldProtect))
{
//if(safeMethod)
//memcpy(szSendBuffer, (void*)startAddress, numberOfBytes);
for(int i = 0; i < numberOfBytes; i++)
{
szSendBuffer[i] = *(unsigned char*)(startAddress + i);
}
VirtualProtect((LPVOID)startAddress, numberOfBytes, dwOldProtect, &dwOldProtect);
}
}
}

The intent of the following code is to read memory without calling ReadProcessMemory and is part of a memory scanning application that I've written. The application scans all of the process' private memory once, and then when it begins a second iteration, it will crash.

The crash occurs when memory is read, after VirtualProtect reports a successful operation. As you can determine from the code, the memory region I am unprotecting and reading from is committed and accessible.

I'm completely baffled as to why reading memory a second time is causing a crash.

[code.kliu.org]

First, what is the error/exception code that the kernel raises when your program dies?

Second, why are you using numberOfBytes instead of mbi.RegionSize? And if you insist on using numberOfBytes, you need to check it against mbi.RegionSize because if the former exceeds the latter (perhaps if a page was decommitted in the interim), then this code will blow up.

BTW, there is no need to check to make sure that &mbi is non-NULL since you're allocating that on the stack.

PS: Oh, and I assume that your code has already ensured that the receiving buffer is always large enough, even if numberOfBytes changes between reads? (BTW, sz denotes a string with a zero termination, so unless you are grabbing C strings, you should not use the sz prefix in the variable name, since that's potentially confusing.)

Edited June 16, 2009 by kliu0x52

[Jon Willis]

if(VirtualQuery((LPCVOID)startAddress, &mbi, sizeof(mbi)))

{

if (mbi.State == MEM_COMMIT && mbi.Protect != PAGE_NOACCESS && mbi.Protect != PAGE_GUARD)

{

if(VirtualProtect((LPVOID)startAddress, numberOfBytes, PAGE_EXECUTE_READWRITE, &dwOldProtect))

{

if(!IsBadReadPtr((LPCVOID)startAddress, numberOfBytes))

{

//if(safeMethod)

//memcpy(szSendBuffer, (void*)startAddress, numberOfBytes);

for(int i = 0; i < numberOfBytes; i++)

{

sendBuffer = *(unsigned char*)(startAddress + i);

}

}

VirtualProtect((LPVOID)startAddress, numberOfBytes, dwOldProtect, &dwOldProtect);

}

}

}

Causes either:

Problem Event Name: APPCRASH

Application Name: hl.exe

Application Version: 1.1.1.1

Application Timestamp: 3fd11900

Fault Module Name: ntdll.dll

Fault Module Version: 6.1.7127.0

Fault Module Timestamp: 4a03d5a1

Exception Code: c0000005

Exception Offset: 0001fd37

Or, sometimes (I cannot reproduce it at the moment), the Fault module name is StackHash_XXXX (where XXXX is a hex number)

StackHash is not a module, and googling the error provides little useful information.

I tried de-protecting each page that my memory region to scan resides on (startAddress to startAddress+numberOfBytes), and it still crashed. I may have screwed up the code, however.

[code.kliu.org]

Well, the nature of the crash report seems to suggest that you are doing something to corrupt memory. It could be some error elsewhere in your program that is being stepped on by this piece of code.

And my second concern? That you should be using mbi.RegionSize instead? Note that these operations that you are using work on entire pages, so you want to be sure that your start address and sizes are correct (both of which should be in increments of 0x1000). And is it really necessary to change the protection mode of pages? Of the three types that you shouldn't read from, you're already exempting no-access and guard pages, leaving you with execute-only pages, which are usually just the contents of the executable part of the PE image.

In any case, I've never tried to do what you're doing, so beyond some generalities, I can't offer any more advice.

Edited June 17, 2009 by kliu0x52

[Jon Willis]

Thanks for your help :)

I tweaked two things. I reduced the scan "chunk" size (numberOfBytes) from 0x5000 to 0x1000, and I changed the code that loops to:

if(!IsBadReadPtr((LPCVOID)startAddress, numberOfBytes))
						{
							//if(safeMethod)
							//memcpy(sendBuffer, (void*)startAddress, numberOfBytes);
							for(int i = 0; i < numberOfBytes; i++)
							{
								sendBuffer[i] = *(unsigned char*)(startAddress + i);
							}
						}

And it still crashes... The crazy thing is that this is the only part of the program that directly deals with memory. The rest is in C#, for crying out loud.

Oh, and I've been leaving that commented out memcpy in there because it is causing the crash as well.

[Jon Willis]

And here's the strange StackHash error:

Problem Event Name: APPCRASH

Application Name: hl.exe

Application Version: 1.1.1.1

Application Timestamp: 3fd11900

Fault Module Name: StackHash_e98d

Fault Module Version: 0.0.0.0

Fault Module Timestamp: 00000000

Exception Code: c0000005

Exception Offset: 004d004f

[code.kliu.org]

A StackHash crash just means that the location of the crash does not correspond to any module; it's just a hash of the stack trace so that a unique-ish name can be used in place of a module name. Normally, this would suggest a memory corruption, but you just mentioned that you are using C#, and that explains a lot. To put it simply, you should not be doing what you are doing using managed code. Injecting code is messy, especially if .NET is in the picture, and there is a good reason why ReadProcessMemory is preferred. I have no idea what you are trying to do, but whatever it is, I think you should rethink your approach.

[Jon Willis]

There seems to be a general issue with checking if memory is readable/writable. IsBadReadPtr has been dubbed "CrashMyProgram" by some. It appears that I may need to do a 'smarter' iteration through memory, rather than just doing continuous scans of 4 kb of memory.