Re: [ros-dev] [ros-diffs] [tkreuzer] 67140: [RTL] Improve RtlImageNtHeaderEx: - Fix signed/unsigned mismatch when comparing NT header offset - Simplify overflow checks - Add missing overflow-into-systemspace check CR-77 / C...

[Alex Ionescu]

The Windows implementation of the API is different in user vs kernel-mode.
We should probably try to remain compatible.

Best regards,
Alex Ionescu

On Fri, Apr 10, 2015 at 12:21 PM, <tkreuzer(a)svn.reactos.org> wrote:

>   Author: tkreuzer
>  Date: Fri Apr 10 19:21:22 2015
>  New Revision: 67140
>
>  URL: http://svn.reactos.org/svn/reactos?rev=67140&view=rev
>  Log:
>  [RTL]
>  Improve RtlImageNtHeaderEx:
>  - Fix signed/unsigned mismatch when comparing NT header offset
>  - Simplify overflow checks
>  - Add missing overflow-into-systemspace check
>  CR-77 / CORE-8091 #resolve
>
>  Modified:
>      trunk/reactos/boot/freeldr/freeldr/lib/rtl/libsupp.c
>      trunk/reactos/dll/ntdll/rtl/libsupp.c
>      trunk/reactos/include/ndk/amd64/mmtypes.h
>      trunk/reactos/include/ndk/i386/mmtypes.h
>      trunk/reactos/lib/rtl/image.c
>      trunk/reactos/lib/rtl/rtlp.h
>      trunk/reactos/ntoskrnl/include/internal/amd64/mm.h
>      trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
>
>  Modified: trunk/reactos/boot/freeldr/freeldr/lib/rtl/libsupp.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/boot/freeldr/freeldr/lib/r…
>
>  ==============================================================================
>  --- trunk/reactos/boot/freeldr/freeldr/lib/rtl/libsupp.c
>  [iso-8859-1] (original)
>  +++ trunk/reactos/boot/freeldr/freeldr/lib/rtl/libsupp.c
>  [iso-8859-1] Fri Apr 10 19:21:22 2015
>  @@ -19,6 +19,8 @@
>    */
>
>   #include <freeldr.h>
>  +
>  +PVOID MmHighestUserAddress = (PVOID)MI_HIGHEST_USER_ADDRESS;
>
>   #if DBG
>   VOID FASTCALL
>
>  Modified: trunk/reactos/dll/ntdll/rtl/libsupp.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/ntdll/rtl/libsupp.c?re…
>
>  ==============================================================================
>  --- trunk/reactos/dll/ntdll/rtl/libsupp.c       [iso-8859-1] (original)
>  +++ trunk/reactos/dll/ntdll/rtl/libsupp.c       [iso-8859-1] Fri Apr 10
>  19:21:22 2015
>  @@ -16,6 +16,7 @@
>
>   SIZE_T RtlpAllocDeallocQueryBufferSize = PAGE_SIZE;
>   PTEB LdrpTopLevelDllBeingLoadedTeb = NULL;
>  +PVOID MmHighestUserAddress = (PVOID)MI_HIGHEST_USER_ADDRESS;
>
>   /* FUNCTIONS
>  ***************************************************************/
>
>
>  Modified: trunk/reactos/include/ndk/amd64/mmtypes.h
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/include/ndk/amd64/mmtypes.…
>
>  ==============================================================================
>  --- trunk/reactos/include/ndk/amd64/mmtypes.h   [iso-8859-1] (original)
>  +++ trunk/reactos/include/ndk/amd64/mmtypes.h   [iso-8859-1] Fri Apr 10
>  19:21:22 2015
>  @@ -36,6 +36,11 @@
>   #define MM_ALLOCATION_GRANULARITY         0x10000
>   #define MM_ALLOCATION_GRANULARITY_SHIFT   16L
>   #define MM_PAGE_FRAME_NUMBER_SIZE         52
>  +
>  +//
>  +// User space range limit
>  +//
>  +#define MI_HIGHEST_USER_ADDRESS         (PVOID)0x000007FFFFFEFFFFULL
>
>   //
>   // Address of the shared user page
>
>  Modified: trunk/reactos/include/ndk/i386/mmtypes.h
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/include/ndk/i386/mmtypes.h…
>
>  ==============================================================================
>  --- trunk/reactos/include/ndk/i386/mmtypes.h    [iso-8859-1] (original)
>  +++ trunk/reactos/include/ndk/i386/mmtypes.h    [iso-8859-1] Fri Apr 10
>  19:21:22 2015
>  @@ -35,6 +35,11 @@
>   #define MM_ALLOCATION_GRANULARITY         0x10000
>   #define MM_ALLOCATION_GRANULARITY_SHIFT   16L
>   #define MM_PAGE_FRAME_NUMBER_SIZE         20
>  +
>  +//
>  +// User space range limit
>  +//
>  +#define MI_HIGHEST_USER_ADDRESS                 (PVOID)0x7FFEFFFF
>
>   //
>   // Address of the shared user page
>
>  Modified: trunk/reactos/lib/rtl/image.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/lib/rtl/image.c?rev=67140&…
>
>  ==============================================================================
>  --- trunk/reactos/lib/rtl/image.c       [iso-8859-1] (original)
>  +++ trunk/reactos/lib/rtl/image.c       [iso-8859-1] Fri Apr 10 19:21:22
>  2015
>  @@ -137,32 +137,42 @@
>    */
>   NTSTATUS
>   NTAPI
>  -RtlImageNtHeaderEx(IN ULONG Flags,
>  -                   IN PVOID Base,
>  -                   IN ULONG64 Size,
>  -                   OUT PIMAGE_NT_HEADERS *OutHeaders)
>  +RtlImageNtHeaderEx(
>  +    _In_ ULONG Flags,
>  +    _In_ PVOID Base,
>  +    _In_ ULONG64 Size,
>  +    _Out_ PIMAGE_NT_HEADERS *OutHeaders)
>   {
>       PIMAGE_NT_HEADERS NtHeaders;
>       PIMAGE_DOS_HEADER DosHeader;
>       BOOLEAN WantsRangeCheck;
>  +    ULONG NtHeaderOffset;
>
>       /* You must want NT Headers, no? */
>  -    if (!OutHeaders) return STATUS_INVALID_PARAMETER;
>  +    if (OutHeaders == NULL)
>  +    {
>  +        DPRINT1("OutHeaders is NULL\n");
>  +        return STATUS_INVALID_PARAMETER;
>  +    }
>
>       /* Assume failure */
>       *OutHeaders = NULL;
>
>       /* Validate Flags */
>  -    if (Flags &~ RTL_IMAGE_NT_HEADER_EX_FLAG_NO_RANGE_CHECK)
>  -    {
>  -        DPRINT1("Invalid flag combination... check for new API flags?\n");
>  +    if (Flags & ~RTL_IMAGE_NT_HEADER_EX_FLAG_NO_RANGE_CHECK)
>  +    {
>  +        DPRINT1("Invalid flags: 0x%lx\n", Flags);
>           return STATUS_INVALID_PARAMETER;
>       }
>
>       /* Validate base */
>  -    if (!(Base) || (Base == (PVOID)-1)) return STATUS_INVALID_PARAMETER;
>  -
>  -    /* Check if the caller wants validation */
>  +    if ((Base == NULL) || (Base == (PVOID)-1))
>  +    {
>  +        DPRINT1("Invalid base address: %p\n", Base);
>  +        return STATUS_INVALID_PARAMETER;
>  +    }
>  +
>  +    /* Check if the caller wants range checks */
>       WantsRangeCheck = !(Flags &
>  RTL_IMAGE_NT_HEADER_EX_FLAG_NO_RANGE_CHECK);
>       if (WantsRangeCheck)
>       {
>  @@ -179,56 +189,56 @@
>       if (DosHeader->e_magic != IMAGE_DOS_SIGNATURE)
>       {
>           /* Not a valid COFF */
>  -        DPRINT1("Not an MZ file\n");
>  +        DPRINT1("Invalid image DOS signature!\n");
>  +        return STATUS_INVALID_IMAGE_FORMAT;
>  +    }
>  +
>  +    /* Get the offset to the NT headers (and copy from LONG to ULONG) */
>  +    NtHeaderOffset = DosHeader->e_lfanew;
>  +
>  +    /* The offset must not be larger than 256MB, as a hard-coded check.
>  +       In Windows this check is only done in user mode, not in kernel
>  mode,
>  +       but it shouldn't harm to have it anyway. Note that without this
>  check,
>  +       other overflow checks would become necessary! */
>  +    if (NtHeaderOffset >= (256 * 1024 * 1024))
>  +    {
>  +        /* Fail */
>  +        DPRINT1("NT headers offset is larger than 256MB!\n");
>           return STATUS_INVALID_IMAGE_FORMAT;
>       }
>
>       /* Check if the caller wants validation */
>       if (WantsRangeCheck)
>       {
>  -        /* The offset should fit in the passsed-in size */
>  -        if (DosHeader->e_lfanew >= Size)
>  +        /* Make sure the file header fits into the size */
>  +        if ((NtHeaderOffset +
>  +             RTL_SIZEOF_THROUGH_FIELD(IMAGE_NT_HEADERS, FileHeader)) >=
>  Size)
>           {
>               /* Fail */
>  -            DPRINT1("e_lfanew is larger than PE file\n");
>  +            DPRINT1("NT headers beyond image size!\n");
>               return STATUS_INVALID_IMAGE_FORMAT;
>           }
>  -
>  -        /* It shouldn't be past 4GB either */
>  -        if (DosHeader->e_lfanew >=
>  -            (MAXULONG - sizeof(IMAGE_DOS_SIGNATURE) -
>  sizeof(IMAGE_FILE_HEADER)))
>  -        {
>  -            /* Fail */
>  -            DPRINT1("e_lfanew is larger than 4GB\n");
>  +    }
>  +
>  +    /* Now get a pointer to the NT Headers */
>  +    NtHeaders = (PIMAGE_NT_HEADERS)((ULONG_PTR)Base + NtHeaderOffset);
>  +
>  +    /* Check if the mapping is in user space */
>  +    if (Base <= MmHighestUserAddress)
>  +    {
>  +        /* Make sure we don't overflow into kernel space */
>  +        if ((PVOID)(NtHeaders + 1) > MmHighestUserAddress)
>  +        {
>  +            DPRINT1("Image overflows from user space into kernel
>  space!\n");
>               return STATUS_INVALID_IMAGE_FORMAT;
>           }
>  -
>  -        /* And the whole file shouldn't overflow past 4GB */
>  -        if ((DosHeader->e_lfanew +
>  -            sizeof(IMAGE_DOS_SIGNATURE) - sizeof(IMAGE_FILE_HEADER)) >=
>  Size)
>  -        {
>  -            /* Fail */
>  -            DPRINT1("PE is larger than 4GB\n");
>  -            return STATUS_INVALID_IMAGE_FORMAT;
>  -        }
>  -    }
>  -
>  -    /* The offset also can't be larger than 256MB, as a hard-coded check
>  */
>  -    if (DosHeader->e_lfanew >= (256 * 1024 * 1024))
>  -    {
>  -        /* Fail */
>  -        DPRINT1("PE offset is larger than 256MB\n");
>  -        return STATUS_INVALID_IMAGE_FORMAT;
>  -    }
>  -
>  -    /* Now it's safe to get the NT Headers */
>  -    NtHeaders = (PIMAGE_NT_HEADERS)((ULONG_PTR)Base +
>  DosHeader->e_lfanew);
>  +    }
>
>       /* Verify the PE Signature */
>       if (NtHeaders->Signature != IMAGE_NT_SIGNATURE)
>       {
>           /* Fail */
>  -        DPRINT1("PE signature missing\n");
>  +        DPRINT1("Invalid image NT signature!\n");
>           return STATUS_INVALID_IMAGE_FORMAT;
>       }
>
>  @@ -245,6 +255,10 @@
>   RtlImageNtHeader(IN PVOID Base)
>   {
>       PIMAGE_NT_HEADERS NtHeader;
>  +
>  +    ULONG c = 1;
>  +    ULONG s = FIELD_OFFSET(IMAGE_OPTIONAL_HEADER32, DataDirectory[c]);
>  +    (void)s;
>
>       /* Call the new API */
>       RtlImageNtHeaderEx(RTL_IMAGE_NT_HEADER_EX_FLAG_NO_RANGE_CHECK,
>
>  Modified: trunk/reactos/lib/rtl/rtlp.h
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/lib/rtl/rtlp.h?rev=67140&a…
>
>  ==============================================================================
>  --- trunk/reactos/lib/rtl/rtlp.h        [iso-8859-1] (original)
>  +++ trunk/reactos/lib/rtl/rtlp.h        [iso-8859-1] Fri Apr 10 19:21:22
>  2015
>  @@ -34,6 +34,8 @@
>
>   #define RVA(m, b) ((PVOID)((ULONG_PTR)(b) + (ULONG_PTR)(m)))
>
>  +extern PVOID MmHighestUserAddress;
>  +
>   NTSTATUS
>   NTAPI
>   RtlpSafeCopyMemory(
>
>  Modified: trunk/reactos/ntoskrnl/include/internal/amd64/mm.h
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/…
>
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/include/internal/amd64/mm.h  [iso-8859-1]
>  (original)
>  +++ trunk/reactos/ntoskrnl/include/internal/amd64/mm.h  [iso-8859-1] Fri
>  Apr 10 19:21:22 2015
>  @@ -5,7 +5,6 @@
>
>   /* Memory layout base addresses */
>   #define MI_LOWEST_VAD_ADDRESS           (PVOID)0x000000007FF00000ULL
>  -#define MI_HIGHEST_USER_ADDRESS         (PVOID)0x000007FFFFFEFFFFULL
>   #define MI_USER_PROBE_ADDRESS           (PVOID)0x000007FFFFFF0000ULL
>   #define MI_DEFAULT_SYSTEM_RANGE_START   (PVOID)0xFFFF080000000000ULL
>   #define MI_REAL_SYSTEM_RANGE_START             0xFFFF800000000000ULL
>
>  Modified: trunk/reactos/ntoskrnl/mm/ARM3/miarm.h
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/miarm.h?r…
>
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/mm/ARM3/miarm.h      [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/mm/ARM3/miarm.h      [iso-8859-1] Fri Apr 10
>  19:21:22 2015
>  @@ -29,7 +29,6 @@
>
>   #define MI_SYSTEM_VIEW_SIZE                     (32 * _1MB)
>
>  -#define MI_HIGHEST_USER_ADDRESS                 (PVOID)0x7FFEFFFF
>   #define MI_USER_PROBE_ADDRESS                   (PVOID)0x7FFF0000
>   #define MI_DEFAULT_SYSTEM_RANGE_START           (PVOID)0x80000000
>   #define MI_SYSTEM_CACHE_WS_START                (PVOID)0xC0C00000
>
>
>