Re: [PATCH] ntdll: deal with images which override the section table during mapping

[Marcus Meissner]

On Mon, Oct 10, 2011 at 11:36:33AM +0200, Bernhard Loos wrote:
> ---
>  dlls/ntdll/virtual.c |   38 ++++++++++++++++++++++++++++++++++----
>  1 files changed, 34 insertions(+), 4 deletions(-)

I think this needs tests, the algorithms are very fragile.

Ciao, Marcus
> diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
> index 5e69eb9..7841d9c 100644
> --- a/dlls/ntdll/virtual.c
> +++ b/dlls/ntdll/virtual.c
> @@ -1114,6 +1114,7 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>      IMAGE_DOS_HEADER *dos;
>      IMAGE_NT_HEADERS *nt;
>      IMAGE_SECTION_HEADER *sec;
> +    IMAGE_SECTION_HEADER *section_table = NULL;
>      IMAGE_DATA_DIRECTORY *imports;
>      NTSTATUS status = STATUS_CONFLICTING_ADDRESSES;
>      int i;
> @@ -1123,6 +1124,7 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>      struct file_view *view = NULL;
>      char *ptr, *header_end;
>      INT_PTR delta = 0;
> +    DWORD SizeOfHeaders;
>
> /* zero-map the whole range */
>
> @@ -1158,8 +1160,11 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>      header_end = ptr + ROUND_SIZE( 0, header_size );
>      memset( ptr + header_size, 0, header_end - (ptr + header_size) );
>      if ((char *)(nt + 1) > header_end) goto error;
> -    sec = (IMAGE_SECTION_HEADER*)((char*)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader);
> -    if ((char *)(sec + nt->FileHeader.NumberOfSections) > header_end) goto error;
> +    section_table = (IMAGE_SECTION_HEADER*)((char*)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader);
> +    /* make sure we mapped enough memory */
> +    if ((char *)(section_table + nt->FileHeader.NumberOfSections) > header_end) goto error;
> +    /* windows is only interested in the size as given by OptionalHeader.SizeOfHeaders */
> +    SizeOfHeaders = ROUND_SIZE( 0, nt->OptionalHeader.SizeOfHeaders );
>
> imports = nt->OptionalHeader.DataDirectory + IMAGE_DIRECTORY_ENTRY_IMPORT;
>      if (!imports->Size || !imports->VirtualAddress) imports = NULL;
> @@ -1180,7 +1185,7 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>          if (nt->OptionalHeader.FileAlignment != nt->OptionalHeader.SectionAlignment) goto error;
>          for (i = 0; i < nt->FileHeader.NumberOfSections; i++)
>          {
> -            if (sec[i].VirtualAddress != sec[i].PointerToRawData)
> +            if (section_table[i].VirtualAddress != section_table[i].PointerToRawData)
>                  goto error;  /* Windows refuses to load in that case too */
>          }
>
> @@ -1192,6 +1197,24 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>          goto done;
>      }
>
> +    if ((char *)(section_table + nt->FileHeader.NumberOfSections) > ptr + SizeOfHeaders)
> +    {
> +        /* part of the section table is outside of SizeOfHeaders
> +         * at this point, it's likely that a section will get mapped over
> +         * the section table (Borderlands does this)
> +         * Not sure how it works on Windows, but we make a copy of the
> +         * table at this point */
> +         sec = RtlAllocateHeap( virtual_heap, 0, sizeof(*sec) * nt->FileHeader.NumberOfSections);
> +         if (!sec)
> +         {
> +             status = STATUS_NO_MEMORY;
> +             goto error;
> +         }
> +         memcpy( sec, section_table, sizeof(*sec) * nt->FileHeader.NumberOfSections);
> +         section_table = sec;
> +     }
> +     else
> +        sec = section_table;
>
> /* map all the sections */
>
> @@ -1200,6 +1223,9 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>          static const SIZE_T sector_align = 0x1ff;
>          SIZE_T map_size, file_start, file_size, end;
>
> +        if (sec->VirtualAddress < SizeOfHeaders)
> +            goto error;
> +
>          if (!sec->Misc.VirtualSize)
>              map_size = ROUND_SIZE( 0, sec->SizeOfRawData );
>          else
> @@ -1329,7 +1355,7 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>
> VIRTUAL_SetProt( view, ptr, ROUND_SIZE( 0, header_size ), VPROT_COMMITTED | VPROT_READ );
>
> -    sec = (IMAGE_SECTION_HEADER*)((char *)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader);
> +    sec = section_table;
>      for (i = 0; i < nt->FileHeader.NumberOfSections; i++, sec++)
>      {
>          SIZE_T size;
> @@ -1361,6 +1387,8 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>      }
>
> done:
> +    if (section_table && (char *)section_table != (char *)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader)
> +        RtlFreeHeap( virtual_heap, 0, section_table );
>      view->mapping = dup_mapping;
>      server_leave_uninterrupted_section( &csVirtual, &sigset );
>
> @@ -1373,6 +1401,8 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>
> error:
>      if (view) delete_view( view );
> +    if (section_table && (char *)section_table != (char *)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader)
> +        RtlFreeHeap( virtual_heap, 0, section_table );
>      server_leave_uninterrupted_section( &csVirtual, &sigset );
>      if (dup_mapping) NtClose( dup_mapping );
>      return status;
>