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...
11 Oct
2014
11 Oct
'14
4:37 p.m.
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(a)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 */
11 Oct
11 Oct
11:59 p.m.
New subject: [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...
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(a)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…
==============================================================================
--- 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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
12 Oct
12 Oct
4:16 a.m.
New subject: [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...
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(a)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(a)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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
3727
days inactive
3728
days old
2 comments
2 participants
participants (2)
-
Alex Ionescu -
Timo Kreuzer