This seems like a Really Bad Idea, and the code is full of serious bugs.
What problem are you trying to solve? And the “antivirus” handwave merely
tries to justify why a bad solution might be desired, but still does not
indicate what problem this is solving. Also, I expect that for Vista and
eyod, integrity levels would be enforced.
joe
more below…
Hi
I am injecting a dll into a process by using APC. I am sending Process ID,
Thread ID, dll path and virtual address of LdrLoadDll in the target
process to my driver.
Below is the code
void APCKernelRoutine(PKAPC pKAPC,
PKNORMAL_ROUTINE pUserAPC,
PVOID pContext,
PVOID pSysArg1,
PVOID pSysArg2)
{
DbgPrint("APCKernelRoutine Entered\n");
ExFreePool(pKAPC);
And this free routine call will work correctly because…?)
}
This is allegedly a __stdcall function. So why does it not have __stdcall
or one of the multitudenous synonyms specified?
NTSTATUS InjectDllByAPC(ULONG TargetPid, ULONG TargetTid, PUNICODE_STRING
usDllPath, ULONG LdrMethodAddress)
{
ULONG size;
PKTHREAD TargetThread;
PEPROCESS TargetProcess;
KAPC_STATE ApcState;
ULONG arg1 = 0;
ULONG arg2 = 0;
ULONG arg3 = 0;
DbgPrint(“Inside InjectDllByAPC…\n”);
size = (unsigned char*)APCMdlCodeEnd - (unsigned char*)APCMdlCode;
DbgPrint(“Allocating MDL (1)…\n”);
pMDLApcCode = IoAllocateMdl(APCMdlCode, size, FALSE, FALSE, NULL);
if (!pMDLApcCode)
{
return(STATUS_UNSUCCESSFUL);
}
MmProbeAndLockPages(pMDLApcCode, KernelMode, IoWriteAccess);
Where do you unlock these pages? And why do you need to lock them?
RtlZeroMemory(pAPCData, sizeof( pAPCData));
Why do you feel a compulsion to zero out memory you are about to overwrite?
memcpy( (char*) pAPCData, usDllPath->Buffer, usDllPath->Length);
and when usDllPath->Length is > size?
unicodeLengthInfo = *(ULONG*) usDllPath;
WHAT??? WHAT WORLD DO YOU LIVE IN WHERE WRITING A LINE LIKE THIS DOES NOT
GET YOU FIRED???
If you want usDllPath->Length, then that is what you write.
pMDLApcData = IoAllocateMdl (pAPCData, sizeof(pAPCData),
FALSE,FALSE,NULL);
if (!pMDLApcData)
{
and what happened to the space allocated by the previous IoAllocateMdl?
return STATUS_UNSUCCESSFUL;
}
MmProbeAndLockPages(pMDLApcData, KernelMode, IoWriteAccess);
Again, why do you feel compelled to lock down the pages?
PsLookupProcessByProcessId((HANDLE)TargetPid, &TargetProcess);
DbgPrint(“Pid: %d, PEPROCESS: 0X%X\n”, TargetPid, TargetProcess);
Why do you know this call can NEVER fail? I would consider its failure to
be a highly-likely outcome. Exercise For The Reader (3 points, since it
should be screamingly obvious): why?
PsLookupThreadByThreadId ((PVOID) TargetTid, &TargetThread);
DbgPrint(“Tid: %d, PKTHREAD: 0X%X\n”, TargetTid, TargetThread);
Ditto.
KeStackAttachProcess((PKPROCESS) TargetProcess, &ApcState);
Given the possibility that one or both of the previous calls failed, why
do you think this one is going to succeed?
pMappedCode = (PVOID*) MmMapLockedPagesSpecifyCache(pMDLApcCode,
UserMode, MmCached, NULL, FALSE, NormalPagePriority);
pMappedData = (PVOID*) MmMapLockedPagesSpecifyCache(pMDLApcData,
UserMode, MmCached, NULL, FALSE, NormalPagePriority);
And why do you assume that either of these mappings coud possibly work?
Why are you not testing for errors?’
KeUnstackDetachProcess (&ApcState);
Given that you have no idea if the attach failed, why do you think a
detatch will succeed?
arg1 = (ULONG) LdrMethodAddress;
arg2 = (ULONG) pMappedData;
arg3 = (ULONG) unicodeLengthInfo;
In what fantssy world do you live in where sizeof(void*) will ALWAYS be
sizeof(ULONG)?
pKAPC = (PKAPC) ExAllocatePool( NonPagedPool, sizeof(KAPC) );
RtlZeroMemory(pKAPC, sizeof(KAPC));
Why do you zero out a structure you are abour to initialize?
KeInitializeApc(pKAPC, TargetThread, OriginalApcEnvironment,
(PKKERNEL_ROUTINE)APCKernelRoutine, NULL,
(PKNORMAL_ROUTINE) pMappedCode,
UserMode, (PVOID)arg1);
Why do you cast a pointer to a ULONG only to cast the ULONG as a pointer?
Why are these variables declared ULONG in the first place?
KeInsertQueueApc(pKAPC, (PVOID)arg2, (PVOID)arg3, 0);
Ditto arg2 and arg3? Special: for two points, explain why code like this
should never exist, and why it is wrong.
//KETHREAD.ApcState.UserApcPending = 1
//*((unsigned char *)TargetThread + 0x4a) = 1; //XP, 2K3 RTM
//*((unsigned char *)TargetThread + 0x3e) = 1; //2K3 SP1, SP2
//*((unsigned char *)TargetThread + 0x4e) = 1; //Vista
*((unsigned char *)TargetThread + 0x56) = 1; //Win 7
Please explain why you should not be fired for writing the above code.
if (pMDLApcCode)
{
MmUnlockPages(pMDLApcCode);
IoFreeMdl(pMDLApcCode);
}
I find it very odd that you would write the above test. Please explain
how execution can reach this point if the pointer *is* NULL.
if (pMDLApcData)
{
MmUnlockPages(pMDLApcData);
IoFreeMdl(pMDLApcData);
}
Ditto
I find it odd that you assume that the mappings are valid in the target
process, even though you are deleting them here ithout the foggiest idea
as to whether or not the APC has executed. There is no way to know if
these pages are still needed.
ObDereferenceObject(TargetProcess);
ObDereferenceObject(TargetThread);
return STATUS_SUCCESS;
}
This code is more than a little scary. It appears to me tat a beginner
has read a few DDI calls and has done a kit-bash of putting them together
with some poor programming practices and a fundamental failure to
understand the basic principes of concurrency, it the vague hope that this
might accomplish something whose design is questionable at best, and upon
deeper examination will probably turn out be completely wrong. The
rampant assumptions that calls must necessarily succeed doom it anyway,
but these superficial errors merely mask the deeper and more fundamental
design flaws.
There are several possible approaches here, and they all start with a
rewrite of this code tat is sufficiently extensive that it could be safely
stated to be “throw this away and start over”. Then look for the places
you make strong sequentiality assumptions where we know these can never be
thought of as even weak guarantees. Learn the basic princinples of modern
Best Practice coding, including why the assumption that sizeof(void*) ==
sizeof(UNLONG) is completely unjustified in modern practice. Learn what
“undocumented field” means. Learn why funky dereferencing instead of
structure member access screams “incompetent programmer” (I kid you not;
looking at this code, I see so many errors that if accompanied by your
resume would be reason enough not to hire you).
I echo Peter’s and (the in-absentia-proxied-by-Peter) Don’s plea: please,
please tell us what the product name is so we can avoid it! If other
parts of the “anti-virus” product resemble this code, it will be
successful because the machine will bluescreen before any virus can get
into it.
void APCMdlCode(PVOID lpLdrLoadDll, PVOID pwsDllPath, PVOID
pwsDllPathLength)
{
UNICODE_STRING usDllName;
ULONG DllCharacteristics = 0;
PVOID DllHandle = 0;
usDllName.Length = (USHORT) pwsDllPathLength;
Has anyone explained to you why storing the low-order 16 bits of the
address of a string length is probably not going to give you the string
length? Or why the somewhat more correct
usDllName.Length = *(USHORT *) pwsDllPathLength
is also semantic nonsense? Why do you think the Length field is a USHORT,
and therefore why iis it that truncating a 32-bit value to 16 bits is
going to make sense?
usDllName.MaximumLength = usDllName.Length + 2;
For one point: why is this statement nonsensical?
usDllName.Buffer = (USHORT*) pwsDllPath;
why is this not (WCHAR*)? And why is it that you know that this buffer is
writeable?
__asm
{
pushad
lea eax, DllHandle
push eax
lea eax, usDllName
push eax
lea eax, DllCharacteristics
push eax
push 0
call [lpLdrLoadDll]
Please, please, PLEASE explain why
lpLdrDll(&DllCharacteristics, &DllName, &DllHandle);
will not accomplish this in a platform-independent fashion, and why,
instead, you wrote that gross piece of assembly code to avvomplish the
same effect. And show me the prototype of lpLdrDll, and te declaration of
the variable lpLdrDll. And why you have chosen the use of Hungarian
Notation in the kernel, and not just using it, but using it incorrectly.
And I did not see where you have enforced the requirement that the called
function has __stdcall linkage.
nop
nop
and the purpose of these teo nops is what, exactly?
popad
}
}
void APCMdlCodeEnd()
{
}
This is working fine in XP and Vista but the target process crashes in
Windows 7. I am not able to figure out why.
Nothing short of a miracle could let this code execute correctly. You
critically depend upon undocumented artifacts of the scheduler, for
example, and “undocumented” means “we are free to change the behavior any
time we feel like it”
Note that you say “it works” without qualifying the statement with any
hardware information, such as number of cores, if hyperthreading is being
used, etc. What I see is a piece of code that is fundamentally incorrect
at very deep levels, but could present with the illusion of working as
long as the unstated assumptions are not vioated.
Also, processes do not “crash”. That is a word which, if unqualified,
marks the poster as a newbie. What actually happened was that the process
terminated prematurely due to a specific kind of error at a specific
address. It is your responsibility to give us that information if you
want an answer that is more helpful than “you have a bug. Fix it.” I’ve
got a pretty good guess where it is failing, and if you look for places
where you depend on nonexistent sequentiality guaranttes, you’ll spot them
as well.
In user mode I am not checking if the thread is alertable or not. Is it
necessary as I am setting KETHREAD.ApcState.UserApcPending to 1 in Kernel
mode? Also in windows 7 the “APCKernelRoutine” is called 10-15 seconds
after DeviceIoControl returns.
Thanks
NTDEV is sponsored by OSR
For our schedule of WDF, WDM, debugging and other seminars visit:
http://www.osr.com/seminars
To unsubscribe, visit the List Server section of OSR Online at
http://www.osronline.com/page.cfm?name=ListServer
