Re: [ros-dev] [ros-diffs] [tkreuzer] 64591: [NTOSKRNL] - Improve the random address base code in MiCreatePebOrTeb to actually make sense and not rely on retarded hacks implicitly hardcoding the PEB size in pages into the ra...
Where't the unit test proving your 'improved' algorithm matches XP SP2/SRV03 SP1?
Best regards, Alex Ionescu
On Tue, Oct 7, 2014 at 5:31 PM, tkreuzer@svn.reactos.org wrote:
Author: tkreuzer Date: Wed Oct 8 00:31:35 2014 New Revision: 64591
URL: http://svn.reactos.org/svn/reactos?rev=64591&view=rev Log: [NTOSKRNL]
- Improve the random address base code in MiCreatePebOrTeb to actually
make sense and not rely on retarded hacks implicitly hardcoding the PEB size in pages into the random value generation.
Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c
Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/procsup.c?...
============================================================================== --- trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] Wed Oct 8 00:31:35 2014 @@ -48,13 +48,13 @@ NTAPI MiCreatePebOrTeb(IN PEPROCESS Process, IN ULONG Size,
OUT PULONG_PTR Base)
OUT PULONG_PTR BaseAddress){ PETHREAD Thread = PsGetCurrentThread(); PMMVAD_LONG Vad; NTSTATUS Status;
- ULONG RandomCoeff;
- ULONG_PTR StartAddress, EndAddress;
- ULONG_PTR HighestAddress, RandomBase;
- ULONG AlignedSize; LARGE_INTEGER CurrentTime; TABLE_SEARCH_RESULT Result = TableFoundNode; PMMADDRESS_NODE Parent;
@@ -83,25 +83,30 @@ /* Check if this is a PEB creation */ if (Size == sizeof(PEB)) {
/* Start at the highest valid address */StartAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1;/* Select the random coefficient */
/* Create a random value to select one page in a 64k region */ KeQueryTickCount(&CurrentTime);
CurrentTime.LowPart &= ((64 * _1KB) >> PAGE_SHIFT) - 1;if (CurrentTime.LowPart <= 1) CurrentTime.LowPart = 2;RandomCoeff = CurrentTime.LowPart << PAGE_SHIFT;/* Select the highest valid address minus the random coefficient*/
StartAddress -= RandomCoeff;EndAddress = StartAddress + ROUND_TO_PAGES(Size) - 1;
CurrentTime.LowPart &= (_64K / PAGE_SIZE) - 1;/* Calculate a random base address */RandomBase = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1;RandomBase -= CurrentTime.LowPart << PAGE_SHIFT;/* Make sure the base address is not too high */AlignedSize = ROUND_TO_PAGES(Size);if ((RandomBase + AlignedSize) >(ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1)
{RandomBase = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1 -AlignedSize;
}/* Calculate the highest allowed address */HighestAddress = RandomBase + AlignedSize - 1; /* Try to find something below the random upper margin */ Result = MiFindEmptyAddressRangeDownTree(ROUND_TO_PAGES(Size),
EndAddress,
HighestAddress, PAGE_SIZE, &Process->VadRoot,
Base,
BaseAddress, &Parent);@@ -113,7 +118,7 @@
(ULONG_PTR)MM_HIGHEST_VAD_ADDRESS, PAGE_SIZE, &Process->VadRoot,
Base,
BaseAddress, &Parent); /* Bail out, if still nothing free was found */ if (Result == TableFoundNode)@@ -125,12 +130,12 @@ }
/* Validate that it came from the VAD ranges */
- ASSERT(*Base >= (ULONG_PTR)MI_LOWEST_VAD_ADDRESS);
ASSERT(*BaseAddress >= (ULONG_PTR)MI_LOWEST_VAD_ADDRESS);
/* Build the rest of the VAD now */
- Vad->StartingVpn = (*Base) >> PAGE_SHIFT;
- Vad->EndingVpn = ((*Base) + Size - 1) >> PAGE_SHIFT;
- Vad->u3.Secured.StartVpn = *Base;
- Vad->StartingVpn = (*BaseAddress) >> PAGE_SHIFT;
- Vad->EndingVpn = ((*BaseAddress) + Size - 1) >> PAGE_SHIFT;
- Vad->u3.Secured.StartVpn = *BaseAddress; Vad->u3.Secured.EndVpn = (Vad->EndingVpn << PAGE_SHIFT) | (PAGE_SIZE
- 1); Vad->u1.Parent = NULL;
@@ -146,7 +151,7 @@ Vad->ControlArea = NULL; // For Memory-Area hack Vad->FirstPrototypePte = NULL; DPRINT("VAD: %p\n", Vad);
- DPRINT("Allocated PEB/TEB at: 0x%p for %16s\n", *Base,
Process->ImageFileName);
- DPRINT("Allocated PEB/TEB at: 0x%p for %16s\n", *BaseAddress,
Process->ImageFileName); MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
/* Release the working set */
The improvement is based on readability / coding style. I think the comment explained the reason to improve it. But I can explain again in more detail: The old code created a random value for a page offset inside a 64k region. The PEB was supposed to get into that region, but preferably not below. Obviously, we don't have ALL of these 16 possible 4k page locations available, if we neither want to go above the upper or below the lower margin, but still fit n pages between these 2. The old code "fixed" the random value to account for that limitation by hardcoding a 2 (for 2 pages), making sure that there are 2 pages available to put the PEB in. The new code will check the size of the PEB instead of relying on the hardcoded magic value of 2, wich wasn't even explained anywhere. So all I did here was remove a hack.
The rest of the change affects the path that we follow on a failure. Instead of trying to allocate the PEB at a constant given address and if that fails, try again from top down, we use the upper margin and try to allocate at the highest address from there. So it cannot fail, unless the whole address space is blocked. The commit message might not have been describing this properly, but the claim that there is no change in Windows behaviour still stays, since there is no way to predict the address of the PEB anyway. It's random, from top down. And it stays that way. Under normal circumstances only the NLS section would block the address range and in that case the allocation will go below, so no change at all. When it comes to cloned processes, things might be more complicated, but then there is no chance to predict the location of the PEB anyway, it could go anywhere, even below the 64k range. Again no change.
If you still have doubts, please let me know what exactly you think could be wrong here, so I can address that accordingly.
Timo
PS: we are talking about randomized behavior here, that is done for security reasons, so doing it differently without breaking assumptions that user mode applications can make would most likely be beneficial. When you write a security software that has prevention capabilities, you also also change Windows behaviour. If that had a negative effect on *legitimate* software running on the system, it would be bad. If it doesn't have any negative effect, or only on "bad" software, it's good. If you can reasonably argue, why this change could possibly affect legitimate software in a negative way, then I can write a test based on that.
Am 11.10.2014 18:37, schrieb Alex Ionescu:
Where't the unit test proving your 'improved' algorithm matches XP SP2/SRV03 SP1?
Best regards, Alex Ionescu
On Tue, Oct 7, 2014 at 5:31 PM, <tkreuzer@svn.reactos.org mailto:tkreuzer@svn.reactos.org> wrote:
Author: tkreuzer Date: Wed Oct 8 00:31:35 2014 New Revision: 64591 URL: http://svn.reactos.org/svn/reactos?rev=64591&view=rev Log: [NTOSKRNL] - Improve the random address base code in MiCreatePebOrTeb to actually make sense and not rely on retarded hacks implicitly hardcoding the PEB size in pages into the random value generation. Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/procsup.c?rev=64591&r1=64590&r2=64591&view=diff ============================================================================== --- trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] Wed Oct 8 00:31:35 2014 @@ -48,13 +48,13 @@ NTAPI MiCreatePebOrTeb(IN PEPROCESS Process, IN ULONG Size, - OUT PULONG_PTR Base) + OUT PULONG_PTR BaseAddress) { PETHREAD Thread = PsGetCurrentThread(); PMMVAD_LONG Vad; NTSTATUS Status; - ULONG RandomCoeff; - ULONG_PTR StartAddress, EndAddress; + ULONG_PTR HighestAddress, RandomBase; + ULONG AlignedSize; LARGE_INTEGER CurrentTime; TABLE_SEARCH_RESULT Result = TableFoundNode; PMMADDRESS_NODE Parent; @@ -83,25 +83,30 @@ /* Check if this is a PEB creation */ if (Size == sizeof(PEB)) { - /* Start at the highest valid address */ - StartAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1; - - /* Select the random coefficient */ + /* Create a random value to select one page in a 64k region */ KeQueryTickCount(&CurrentTime); - CurrentTime.LowPart &= ((64 * _1KB) >> PAGE_SHIFT) - 1; - if (CurrentTime.LowPart <= 1) CurrentTime.LowPart = 2; - RandomCoeff = CurrentTime.LowPart << PAGE_SHIFT; - - /* Select the highest valid address minus the random coefficient */ - StartAddress -= RandomCoeff; - EndAddress = StartAddress + ROUND_TO_PAGES(Size) - 1; + CurrentTime.LowPart &= (_64K / PAGE_SIZE) - 1; + + /* Calculate a random base address */ + RandomBase = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1; + RandomBase -= CurrentTime.LowPart << PAGE_SHIFT; + + /* Make sure the base address is not too high */ + AlignedSize = ROUND_TO_PAGES(Size); + if ((RandomBase + AlignedSize) > (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1) + { + RandomBase = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1 - AlignedSize; + } + + /* Calculate the highest allowed address */ + HighestAddress = RandomBase + AlignedSize - 1; /* Try to find something below the random upper margin */ Result = MiFindEmptyAddressRangeDownTree(ROUND_TO_PAGES(Size), - EndAddress, + HighestAddress, PAGE_SIZE, &Process->VadRoot, - Base, + BaseAddress, &Parent); } @@ -113,7 +118,7 @@ (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS, PAGE_SIZE, &Process->VadRoot, - Base, + BaseAddress, &Parent); /* Bail out, if still nothing free was found */ if (Result == TableFoundNode) @@ -125,12 +130,12 @@ } /* Validate that it came from the VAD ranges */ - ASSERT(*Base >= (ULONG_PTR)MI_LOWEST_VAD_ADDRESS); + ASSERT(*BaseAddress >= (ULONG_PTR)MI_LOWEST_VAD_ADDRESS); /* Build the rest of the VAD now */ - Vad->StartingVpn = (*Base) >> PAGE_SHIFT; - Vad->EndingVpn = ((*Base) + Size - 1) >> PAGE_SHIFT; - Vad->u3.Secured.StartVpn = *Base; + Vad->StartingVpn = (*BaseAddress) >> PAGE_SHIFT; + Vad->EndingVpn = ((*BaseAddress) + Size - 1) >> PAGE_SHIFT; + Vad->u3.Secured.StartVpn = *BaseAddress; Vad->u3.Secured.EndVpn = (Vad->EndingVpn << PAGE_SHIFT) | (PAGE_SIZE - 1); Vad->u1.Parent = NULL; @@ -146,7 +151,7 @@ Vad->ControlArea = NULL; // For Memory-Area hack Vad->FirstPrototypePte = NULL; DPRINT("VAD: %p\n", Vad); - DPRINT("Allocated PEB/TEB at: 0x%p for %16s\n", *Base, Process->ImageFileName); + DPRINT("Allocated PEB/TEB at: 0x%p for %16s\n", *BaseAddress, Process->ImageFileName); MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result); /* Release the working set */
Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
The magic of 2 is to account for the Wow64 PEB, on 64-bit systems, FYI.
I agree on your other points -- the 'test' was not "prove me this generates the same random numbers as windows" but rather "make sure this actually generates enough randomness"
Best regards, Alex Ionescu
On Sat, Oct 11, 2014 at 4:59 PM, Timo Kreuzer timo.kreuzer@web.de wrote:
The improvement is based on readability / coding style. I think the comment explained the reason to improve it. But I can explain again in more detail: The old code created a random value for a page offset inside a 64k region. The PEB was supposed to get into that region, but preferably not below. Obviously, we don't have ALL of these 16 possible 4k page locations available, if we neither want to go above the upper or below the lower margin, but still fit n pages between these 2. The old code "fixed" the random value to account for that limitation by hardcoding a 2 (for 2 pages), making sure that there are 2 pages available to put the PEB in. The new code will check the size of the PEB instead of relying on the hardcoded magic value of 2, wich wasn't even explained anywhere. So all I did here was remove a hack.
The rest of the change affects the path that we follow on a failure. Instead of trying to allocate the PEB at a constant given address and if that fails, try again from top down, we use the upper margin and try to allocate at the highest address from there. So it cannot fail, unless the whole address space is blocked. The commit message might not have been describing this properly, but the claim that there is no change in Windows behaviour still stays, since there is no way to predict the address of the PEB anyway. It's random, from top down. And it stays that way. Under normal circumstances only the NLS section would block the address range and in that case the allocation will go below, so no change at all. When it comes to cloned processes, things might be more complicated, but then there is no chance to predict the location of the PEB anyway, it could go anywhere, even below the 64k range. Again no change.
If you still have doubts, please let me know what exactly you think could be wrong here, so I can address that accordingly.
Timo
PS: we are talking about randomized behavior here, that is done for security reasons, so doing it differently without breaking assumptions that user mode applications can make would most likely be beneficial. When you write a security software that has prevention capabilities, you also also change Windows behaviour. If that had a negative effect on *legitimate* software running on the system, it would be bad. If it doesn't have any negative effect, or only on "bad" software, it's good. If you can reasonably argue, why this change could possibly affect legitimate software in a negative way, then I can write a test based on that.
Am 11.10.2014 18:37, schrieb Alex Ionescu:
Where't the unit test proving your 'improved' algorithm matches XP SP2/SRV03 SP1?
Best regards, Alex Ionescu
On Tue, Oct 7, 2014 at 5:31 PM, tkreuzer@svn.reactos.org wrote:
Author: tkreuzer Date: Wed Oct 8 00:31:35 2014 New Revision: 64591
URL: http://svn.reactos.org/svn/reactos?rev=64591&view=rev Log: [NTOSKRNL]
- Improve the random address base code in MiCreatePebOrTeb to actually
make sense and not rely on retarded hacks implicitly hardcoding the PEB size in pages into the random value generation.
Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c
Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/procsup.c?...
============================================================================== --- trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] Wed Oct 8 00:31:35 2014 @@ -48,13 +48,13 @@ NTAPI MiCreatePebOrTeb(IN PEPROCESS Process, IN ULONG Size,
OUT PULONG_PTR Base)
OUT PULONG_PTR BaseAddress){ PETHREAD Thread = PsGetCurrentThread(); PMMVAD_LONG Vad; NTSTATUS Status;
- ULONG RandomCoeff;
- ULONG_PTR StartAddress, EndAddress;
- ULONG_PTR HighestAddress, RandomBase;
- ULONG AlignedSize; LARGE_INTEGER CurrentTime; TABLE_SEARCH_RESULT Result = TableFoundNode; PMMADDRESS_NODE Parent;
@@ -83,25 +83,30 @@ /* Check if this is a PEB creation */ if (Size == sizeof(PEB)) {
/* Start at the highest valid address */StartAddress = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1;/* Select the random coefficient */
/* Create a random value to select one page in a 64k region */ KeQueryTickCount(&CurrentTime);
CurrentTime.LowPart &= ((64 * _1KB) >> PAGE_SHIFT) - 1;if (CurrentTime.LowPart <= 1) CurrentTime.LowPart = 2;RandomCoeff = CurrentTime.LowPart << PAGE_SHIFT;/* Select the highest valid address minus the random coefficient*/
StartAddress -= RandomCoeff;EndAddress = StartAddress + ROUND_TO_PAGES(Size) - 1;
CurrentTime.LowPart &= (_64K / PAGE_SIZE) - 1;/* Calculate a random base address */RandomBase = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1;RandomBase -= CurrentTime.LowPart << PAGE_SHIFT;/* Make sure the base address is not too high */AlignedSize = ROUND_TO_PAGES(Size);if ((RandomBase + AlignedSize) >(ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1)
{RandomBase = (ULONG_PTR)MM_HIGHEST_VAD_ADDRESS + 1 -AlignedSize;
}/* Calculate the highest allowed address */HighestAddress = RandomBase + AlignedSize - 1; /* Try to find something below the random upper margin */ Result = MiFindEmptyAddressRangeDownTree(ROUND_TO_PAGES(Size),
EndAddress,
HighestAddress, PAGE_SIZE, &Process->VadRoot,
Base,
BaseAddress, &Parent);@@ -113,7 +118,7 @@
(ULONG_PTR)MM_HIGHEST_VAD_ADDRESS, PAGE_SIZE, &Process->VadRoot,
Base,
BaseAddress, &Parent); /* Bail out, if still nothing free was found */ if (Result == TableFoundNode)@@ -125,12 +130,12 @@ }
/* Validate that it came from the VAD ranges */
- ASSERT(*Base >= (ULONG_PTR)MI_LOWEST_VAD_ADDRESS);
ASSERT(*BaseAddress >= (ULONG_PTR)MI_LOWEST_VAD_ADDRESS);
/* Build the rest of the VAD now */
- Vad->StartingVpn = (*Base) >> PAGE_SHIFT;
- Vad->EndingVpn = ((*Base) + Size - 1) >> PAGE_SHIFT;
- Vad->u3.Secured.StartVpn = *Base;
- Vad->StartingVpn = (*BaseAddress) >> PAGE_SHIFT;
- Vad->EndingVpn = ((*BaseAddress) + Size - 1) >> PAGE_SHIFT;
- Vad->u3.Secured.StartVpn = *BaseAddress; Vad->u3.Secured.EndVpn = (Vad->EndingVpn << PAGE_SHIFT) | (PAGE_SIZE
- 1); Vad->u1.Parent = NULL;
@@ -146,7 +151,7 @@ Vad->ControlArea = NULL; // For Memory-Area hack Vad->FirstPrototypePte = NULL; DPRINT("VAD: %p\n", Vad);
- DPRINT("Allocated PEB/TEB at: 0x%p for %16s\n", *Base,
Process->ImageFileName);
- DPRINT("Allocated PEB/TEB at: 0x%p for %16s\n", *BaseAddress,
Process->ImageFileName); MiInsertNode(&Process->VadRoot, (PVOID)Vad, Parent, Result);
/* Release the working set */
Ros-dev mailing listRos-dev@reactos.orghttp://www.reactos.org/mailman/listinfo/ros-dev
Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
-
Alex Ionescu -
Timo Kreuzer