[PATCH v2] ntdll: Fix overflow if running 32bit app w/ wine64

[Brendan McGrath]

wine64 was using IMAGE_NT_HEADERS to access header information
regardless of the execution type; hence it would use
IMAGE_OPTIONAL_HEADER64 for a 32bit app.

This could result in an overflow and a request to mmap for a huge amount
of memory causing an out of memory error.

This patch ensures IMAGE_OPTIONAL_HEADER32 is used for a 32-bit app
and IMAGE_OPTIONAL_HEADER64 is used for a 64-bit app

Signed-off-by: Brendan McGrath 
---

Fixed the formatting - sorry about that

dlls/ntdll/virtual.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
index af1509eae5d..a05000f2e2b 100644
--- a/dlls/ntdll/virtual.c
+++ b/dlls/ntdll/virtual.c
@@ -1933,9 +1933,26 @@ NTSTATUS virtual_alloc_thread_stack( TEB *teb, SIZE_T reserve_size, SIZE_T commi

if (!reserve_size || !commit_size)
     {
-        IMAGE_NT_HEADERS *nt = RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress );
-        if (!reserve_size) reserve_size = nt->OptionalHeader.SizeOfStackReserve;
-        if (!commit_size) commit_size = nt->OptionalHeader.SizeOfStackCommit;
+        struct nt
+        {
+            DWORD Signature;
+            IMAGE_FILE_HEADER FileHeader;
+            union
+            {
+                IMAGE_OPTIONAL_HEADER32 hdr32;
+                IMAGE_OPTIONAL_HEADER64 hdr64;
+            } opt;
+        };
+
+        struct nt *nt = (struct nt*) RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress );
+        if (nt->opt.hdr32.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+            if (!reserve_size) reserve_size = nt->opt.hdr32.SizeOfStackReserve;
+            if (!commit_size) commit_size = nt->opt.hdr32.SizeOfStackCommit;
+        } else {
+            if (!reserve_size) reserve_size = nt->opt.hdr64.SizeOfStackReserve;
+            if (!commit_size) commit_size = nt->opt.hdr64.SizeOfStackCommit;
+        }
+        TRACE("reserve_size: %lu, commit_size: %lu", reserve_size, commit_size);
     }

size = max( reserve_size, commit_size );

[Brendan McGrath]

I'm following up on this to patch to better illustrate the problem.

If you run the bash script I have included below - it will create two 
applications:
1. mono32.exe: PE32 executable (console) Intel 80386 Mono/.Net assembly, 
for MS Windows; and
2. c64.exe:    PE32+ executable (console) x86-64, for MS Windows

mono32 being a 32bit mono application and c64 being a 64bit 'C' 
application (this emulates the set-up of the launcher and application 
I'm trying to get working).

When ran, mono32 grabs the MachineGuid from the registry. It then passes 
it to c64.exe, which grabs the same registry value and compares them.

These executables work under Windows but do not work under Wine. This is 
because if you run:
wine mono32

mono32 will get a different value to c64. This is because 'wine mono32' 
will fetch the value from the WoW6432 branch. This currently has a 
different value to the 64bit branch.

(I have already submitted a patch to ensure these values are the same - 
but it was rightly rejected. As Nikolay Sivov pointed out - Windows 
doesn't have a WoW6432 entry and neither should Wine. Another patch has 
been submitted for that).

But if you run:
wine64 mono32

you get the Out of Memory error that is fixed by this patch. Applying 
this patch allows these applications to work as they currently do under 
Windows.

So apart from manually copying the 64bit MachineGuid entry to the 
WoW6432 branch, then without this patch, I'm not aware of any other 
solution. Please let me know if there is one.

Below is the bash script that demonstrates the issue (you can just copy 
and paste the whole lot):

# create mono source
cat << 'END' > mono32.cs
using Microsoft.Win32;
using System.Diagnostics;
using System.Reflection;
using System.IO;
using System;

public class Launcher
{
     static public void Main ()
     {
         RegistryKey rk = 
Registry.LocalMachine.OpenSubKey("SOFTWARE\Microsoft\Cryptography");

if (rk != null) {
            object guid = rk.GetValue("MachineGuid");
            if (guid != null) {
               string strGuid = guid.ToString();
               string cwd = 
Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location);
               ProcessStartInfo processStartInfo = new ProcessStartInfo();
               processStartInfo.FileName = cwd + "\c64.exe";
               processStartInfo.Arguments = strGuid;
               processStartInfo.UseShellExecute = false;
               Process.Start(processStartInfo);
            } else {
               Console.WriteLine ("guid not found - run under 'wine64'");
            }
         } else {
            Console.WriteLine("Mono couldn't find 
'HKLM\SOFTWARE\Microsoft\Cryptography'");
         }
     }
}
END

# create 'C' source
cat << 'END' > c64.c
#include <windows.h>
#include <stdio.h>

int main(int argc, const char *argv[])
{
     LONG rc;
     BYTE data[200];
     DWORD data_size = 200;
     HKEY hkey;

if (argc != 2)
         printf("Usage: c64.exe guid\n");

if (RegOpenKeyExA(HKEY_LOCAL_MACHINE, 
"Software\Microsoft\Cryptography", 0, KEY_QUERY_VALUE, &hkey) == 
ERROR_SUCCESS) {
         rc = RegQueryValueExA(hkey, "MachineGuid", NULL, NULL, data, 
&data_size);
         if (rc == ERROR_SUCCESS) {
             if (strcmp(data, argv[1]) == 0) {
                 printf("We match - I work\n");
             } else {
                 printf("We don't match. I got:\n%s\nMono gave 
me:\n%s\n", data, argv[1]);
             }
         } else {
             printf("guid not found\n", rc);
         }
     } else {
         printf("c64 couldn't find 
'HKLM\SOFTWARE\Microsoft\Cryptography'\n");
     }
}
END

# compile mono
mcs mono32.cs

# compile 'C'
x86_64-w64-mingw32-gcc c64.c -o c64.exe

# try wine (always fails as the guid is wrong or missing)
wine mono32

# try wine64 (fails with out of memory unless the overflow patch is applied)
wine64 mono32

On 17/10/18 8:49 pm, Brendan McGrath wrote:
> wine64 was using IMAGE_NT_HEADERS to access header information
> regardless of the execution type; hence it would use
> IMAGE_OPTIONAL_HEADER64 for a 32bit app.
>
> This could result in an overflow and a request to mmap for a huge amount
> of memory causing an out of memory error.
>
> This patch ensures IMAGE_OPTIONAL_HEADER32 is used for a 32-bit app
> and IMAGE_OPTIONAL_HEADER64 is used for a 64-bit app
> Signed-off-by: Brendan McGrath
>
> Fixed the formatting - sorry about that
>
> dlls/ntdll/virtual.c | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
> index af1509eae5d..a05000f2e2b 100644
> --- a/dlls/ntdll/virtual.c
> +++ b/dlls/ntdll/virtual.c
> @@ -1933,9 +1933,26 @@ NTSTATUS virtual_alloc_thread_stack( TEB *teb, SIZE_T reserve_size, SIZE_T commi
>   if (!reserve_size || !commit_size)
>   {
> -
>    IMAGE_NT_HEADERS *nt = RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress );
> -
>    if (!reserve_size) reserve_size = nt->OptionalHeader.SizeOfStackReserve;
> -
>    if (!commit_size) commit_size = nt->OptionalHeader.SizeOfStackCommit;
> -
>    struct nt
> -
>    {
> -
>        DWORD Signature;
> -
>        IMAGE_FILE_HEADER FileHeader;
> -
>        union
> -
>        {
> -
>            IMAGE_OPTIONAL_HEADER32 hdr32;
> -
>            IMAGE_OPTIONAL_HEADER64 hdr64;
> -
>        } opt;
> -
>    };
> -
> -
>    struct nt *nt = (struct nt*) RtlImageNtHeader( NtCurrentTeb()->Peb->ImageBaseAddress );
> -
>    if (nt->opt.hdr32.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> -
>        if (!reserve_size) reserve_size = nt->opt.hdr32.SizeOfStackReserve;
> -
>        if (!commit_size) commit_size = nt->opt.hdr32.SizeOfStackCommit;
> -
>    } else {
> -
>        if (!reserve_size) reserve_size = nt->opt.hdr64.SizeOfStackReserve;
> -
>        if (!commit_size) commit_size = nt->opt.hdr64.SizeOfStackCommit;
> -
>    }
> -
>    TRACE("reserve_size: %lu, commit_size: %lu", reserve_size, commit_size);
> }
>
> size = max( reserve_size, commit_size );

[Alexandre Julliard]

Brendan McGrath  writes:
> But if you run:
> wine64 mono32
>
> you get the Out of Memory error that is fixed by this patch. Applying
> this patch allows these applications to work as they currently do
> under Windows.

For .NET binaries this should be solved by generating a PE64 header to
replace the PE32 one.

[Vincent Povirk]

> > you get the Out of Memory error that is fixed by this patch. Applying
> > this patch allows these applications to work as they currently do
> > under Windows.
>
> For .NET binaries this should be solved by generating a PE64 header to
> replace the PE32 one.

Does the test case work with native .NET? If not, we need to fix that first.

If it does, we should change the header in _CorValidateImage.

[Brendan McGrath]

I installed .NET 4.0 and it doesn't work. It's the same error. It works 
however with this patch.

On 19/10/18 6:10 am, Vincent Povirk wrote:
> > > you get the Out of Memory error that is fixed by this patch. Applying
> > > this patch allows these applications to work as they currently do
> > > under Windows.
> >
> > For .NET binaries this should be solved by generating a PE64 header to
> > replace the PE32 one.
>
> Does the test case work with native .NET? If not, we need to fix that first.
>
> If it does, we should change the header in _CorValidateImage.

[Vincent Povirk]

> I installed .NET 4.0 and it doesn't work. It's the same error. It works
> however with this patch.

If native mscoree is being used (you might want to double check) it
suggests that either ntdll is calling _CorValidateImage too late, or
this is the correct approach. I don't know how to test this.

[Brendan McGrath]

I ran this test with WINEDEBUG=+relay with and without the patch.

With the patch I see this entry:
0067:Call PE DLL (proc=0x7fe6ef3a8610,module=0x7fe6ef380000 
L"mscoree.dll",reason=PROCESS_ATTACH,res=0x22fb00)

Without the patch I don't (in fact the text 'mscoree' isn't in the log 
at all).

So I think the crash happens before the mscoree.dll library is loaded. 
And I guess this makes sense since the issue is with allocation of 
memory for the stack of the main process.

On 20/10/18 12:24 am, Vincent Povirk wrote:
> > I installed .NET 4.0 and it doesn't work. It's the same error. It works
> > however with this patch.
>
> If native mscoree is being used (you might want to double check) it
> suggests that either ntdll is calling _CorValidateImage too late, or
> this is the correct approach. I don't know how to test this.

[Brendan McGrath]

Did you mean something like the below? This also fixes my test case - 
but I feel like my original patch was a lot safer (obviously the 
'imports' reference should be fixed regardless).

diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
index af1509eae5d..9be494ba57f 100644
--- a/dlls/ntdll/virtual.c
+++ b/dlls/ntdll/virtual.c
@@ -1364,7 +1364,7 @@ static NTSTATUS map_image( HANDLE hmapping, 
ACCESS_MASK access, int fd, SIZE_T m
      IMAGE_NT_HEADERS *nt;
      IMAGE_SECTION_HEADER sections[96];
      IMAGE_SECTION_HEADER *sec;
-    IMAGE_DATA_DIRECTORY *imports;
+    IMAGE_DATA_DIRECTORY *imports = NULL;
      NTSTATUS status = STATUS_CONFLICTING_ADDRESSES;
      SIZE_T header_size, total_size = image_info->map_size;
      int i;
@@ -1423,7 +1423,47 @@ static NTSTATUS map_image( HANDLE hmapping, 
ACCESS_MASK access, int fd, SIZE_T m
      memcpy(sections, header_start, sizeof(*sections) * 
nt->FileHeader.NumberOfSections);
      sec = sections;

-    imports = nt->OptionalHeader.DataDirectory + 
IMAGE_DIRECTORY_ENTRY_IMPORT;
+    if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC && 
is_win64 &&
+ !(nt->FileHeader.Characteristics & IMAGE_FILE_DLL)) {
+       IMAGE_OPTIONAL_HEADER32 *hdr32 = (IMAGE_OPTIONAL_HEADER32 *) 
&nt->OptionalHeader;
+       /* check if there is room in the header for generating a PE64 
header,
+        * if not - we'll leave it and hope for the best */
+       if (header_start + sizeof(IMAGE_OPTIONAL_HEADER64) - 
sizeof(IMAGE_OPTIONAL_HEADER32)
+                        + sizeof(*sections) * 
nt->FileHeader.NumberOfSections <= header_end) {
+           /* We'll convert the IMAGE_OPTIONAL_HEADER32 to 
IMAGE_OPTIONAL_HEADER64
+            * we'll splat the sections area - but this was just copied.
+            * we copy from back to front so we don't splat the data we 
still need */
+           memmove(nt->OptionalHeader.DataDirectory, hdr32->DataDirectory,
+                                    hdr32->NumberOfRvaAndSizes * 
sizeof(hdr32->DataDirectory[0]));
+           nt->OptionalHeader.NumberOfRvaAndSizes = 
hdr32->NumberOfRvaAndSizes;
+           nt->OptionalHeader.LoaderFlags = hdr32->LoaderFlags;
+           nt->OptionalHeader.SizeOfHeapCommit = hdr32->SizeOfHeapCommit;
+           nt->OptionalHeader.SizeOfHeapReserve = hdr32->SizeOfHeapReserve;
+           nt->OptionalHeader.SizeOfStackCommit = hdr32->SizeOfStackCommit;
+           nt->OptionalHeader.SizeOfStackReserve = 
hdr32->SizeOfStackReserve;
+           nt->OptionalHeader.ImageBase = hdr32->ImageBase;
+           /* the rest of the values align between the 32bit and 64bit 
optional header */
+
+           /* update the magic value and the size of optional header */
+           nt->OptionalHeader.Magic = IMAGE_NT_OPTIONAL_HDR64_MAGIC;
+           nt->FileHeader.SizeOfOptionalHeader += 
sizeof(IMAGE_OPTIONAL_HEADER64)
+                                                             - 
sizeof(IMAGE_OPTIONAL_HEADER32);
+
+           /* copy the sections back
+            * Will this will break apps that map over the top of the 
section headers?
+            * (hopefully none of these are 32bit .NET/Mono apps anyway) */
+ memcpy((char*)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader,
+                                sections, sizeof(*sections) * 
nt->FileHeader.NumberOfSections);
+       } else {
+           /* even though we can't create a PE64 header, we can still 
make sure
+            * imports points to the correct location */
+           imports = hdr32->DataDirectory + IMAGE_DIRECTORY_ENTRY_IMPORT;
+       }
+    }
+
+    if (!imports)
+        imports = nt->OptionalHeader.DataDirectory + 
IMAGE_DIRECTORY_ENTRY_IMPORT;
+
      if (!imports->Size || !imports->VirtualAddress) imports = NULL;

/* check for non page-aligned binary */

On 19/10/18 5:32 am, Alexandre Julliard wrote:
> Brendan McGrath  writes:
> > But if you run:
> > wine64 mono32
> >
> > you get the Out of Memory error that is fixed by this patch. Applying
> > this patch allows these applications to work as they currently do
> > under Windows.
>
> For .NET binaries this should be solved by generating a PE64 header to
> replace the PE32 one.

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=43495

Your paranoid android.

=== debian9 (build log) ===

error: corrupt patch at line 10
Task: Patch failed to apply

=== debian9 (build log) ===

error: corrupt patch at line 10
Task: Patch failed to apply

[Alexandre Julliard]

Brendan McGrath  writes:
> Did you mean something like the below? This also fixes my test case - 
> but I feel like my original patch was a lot safer (obviously the
> 'imports' reference should be fixed regardless).

I've committed a different fix, but yes, that's the general idea.

[Brendan McGrath]

Awesome. I just tired it out and it works well. Thanks very much.

On 26/10/18 7:27 am, Alexandre Julliard wrote:
> Brendan McGrath  writes:
> > Did you mean something like the below? This also fixes my test case -
> > but I feel like my original patch was a lot safer (obviously the
> > 'imports' reference should be fixed regardless).
>
> I've committed a different fix, but yes, that's the general idea.