[2/2] Allow ASLR to be disabled on Windows

Message ID 20211006204704.2175701-3-tromey@adacore.com
State New
Headers show
Series
  • Make "set disable-randomization" work on Windows
Related show

Commit Message

Lancelot SIX via Gdb-patches Oct. 6, 2021, 8:47 p.m.
On Windows, it is possible to disable ASLR when creating a process.
This patch adds code to do this, and hooks it up to gdb's existing
disable-randomization feature.  Because the Windows documentation
cautions that this isn't available on all versions of Windows, the
CreateProcess wrapper function is updated to make the attempt, and
then fall back to the current approach if it fails.

Note that, in order to see the necessary declarations for this code to
work, the patch bumps _WIN32_WINNT from 0x0501 to 0x0602 -- that is,
Windows 8.  I don't know whether this may is a problem for anyone.
---
 gdb/NEWS                 |  4 ++
 gdb/nat/windows-nat.c    | 87 ++++++++++++++++++++++++++++++++++++----
 gdb/nat/windows-nat.h    | 11 ++++-
 gdb/windows-nat.c        |  7 ++++
 gdbserver/win32-low.cc   |  1 +
 gdbserver/win32-low.h    |  5 +++
 gdbsupport/common-defs.h |  8 ++--
 7 files changed, 110 insertions(+), 13 deletions(-)

-- 
2.31.1

Comments

Lancelot SIX via Gdb-patches Oct. 7, 2021, 6:46 a.m. | #1
> Date: Wed,  6 Oct 2021 14:47:04 -0600

> From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>

> Cc: Tom Tromey <tromey@adacore.com>

> 

> Note that, in order to see the necessary declarations for this code to

> work, the patch bumps _WIN32_WINNT from 0x0501 to 0x0602 -- that is,

> Windows 8.  I don't know whether this may is a problem for anyone.


It is for me.

Please, let's not unsupport old Windows versions just because some
optional feature needs a new API.  Instead, we should test for the
availability of that API at run time, and disable the feature if the
API is unavailable.  We already do that with several APIs, see
initialize_loadable in nat/windows-nat.c.  Why not do the same with
the APIs you need to support this feature?  Using that technique will
makes GDB able to run on older versions of Windows if the user doesn't
need to disable ASLR.

For example, I use GDB a lot on Windows XP, where ASLR doesn't even
exist, and therefore the problem you want to solve doesn't exist.  But
the changes you propose will prevent GDB from even starting on XP or
any other systems older than Windows 8.

> +#include <winbase.h>

> +#include <processthreadsapi.h>


I don't think this is the right way of including Windows headers.
Doesn't it work to just include <windows.h> (which nat/windows-nat.h
already does)?  The MS documentation seems to say it should be enough.

If including windows.h alone doesn't work, then we should have a
configure-time test for processthreadapi.h, instead of including it
unconditionally, as that header might not exist (e.g., my MinGW
doesn't have it).  Moreover, some prototypes and macros that are only
available since Windows 8 should be defined by hand in our sources, if
they are not already defined, because using this:

> -/* We don't support Windows versions before XP, so we define

> +/* We don't support Windows versions before Windows 8, so we define

>     _WIN32_WINNT correspondingly to ensure the Windows API headers

>     expose the required symbols.  */

>  #if defined (__MINGW32__) || defined (__CYGWIN__)

>  # ifdef _WIN32_WINNT

> -#  if _WIN32_WINNT < 0x0501

> +#  if _WIN32_WINNT < 0x0602

>  #   undef _WIN32_WINNT

> -#   define _WIN32_WINNT 0x0501

> +#   define _WIN32_WINNT 0x0602

>  #  endif

>  # else

> -#  define _WIN32_WINNT 0x0501

> +#  define _WIN32_WINNT 0x0602

>  # endif

>  #endif	/* __MINGW32__ || __CYGWIN__ */


will not help when building on older systems, and unnecessarily limits
the GDB support to Windows 8 and later systems.

> +	  if (startup_info != nullptr)

> +	    info_ex.StartupInfo = *startup_info;

> +	  info_ex.StartupInfo.cb = sizeof (info_ex);

> +	  SIZE_T size = 0;

> +	  /* Ignore the result here.  The documentation says the first

> +	     call always fails, by design.  */

> +	  InitializeProcThreadAttributeList (nullptr, 1, 0, &size);

> +	  info_ex.lpAttributeList

> +	    = (PPROC_THREAD_ATTRIBUTE_LIST) alloca (size);

> +	  InitializeProcThreadAttributeList (info_ex.lpAttributeList,

> +					     1, 0, &size);

> +

> +	  gdb::optional<BOOL> return_value;

> +	  DWORD attr_flags =

> +	    (PROCESS_CREATION_MITIGATION_POLICY_FORCE_RELOCATE_IMAGES_ALWAYS_OFF

> +	     | PROCESS_CREATION_MITIGATION_POLICY_BOTTOM_UP_ASLR_ALWAYS_OFF);

> +	  if (!UpdateProcThreadAttribute (info_ex.lpAttributeList, 0,

> +					  PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY,

> +					  &attr_flags,

> +					  sizeof (attr_flags),

> +					  nullptr, nullptr))

> +	    tried_and_failed = true;


As mentioned above, these new APIs should be loaded dynamically and
called through a function pointer.  And the requisite preprocessor
macros should be defined if they aren't defined by the headers.  If
the new APIs are not available, the wrapper should simply call
CreateProcess without doing anything else.

> +  bool supports_disable_randomization () override

> +  {

> +    return true;

> +  }


This cannot be unconditional, it should depend on whether the requisite
APIs could be dynamically loaded at run time.

> --- a/gdbserver/win32-low.h

> +++ b/gdbserver/win32-low.h

> @@ -158,6 +158,11 @@ class win32_process_target : public process_stratum_target

>    bool stopped_by_sw_breakpoint () override;

>  

>    bool supports_stopped_by_sw_breakpoint () override;

> +

> +  bool supports_disable_randomization () override

> +  {

> +    return true;

> +  }


Same here.

Thanks.
Lancelot SIX via Gdb-patches Oct. 12, 2021, 8:52 p.m. | #2
On Thu, Oct 7, 2021 at 2:47 AM Eli Zaretskii via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>

> > Date: Wed,  6 Oct 2021 14:47:04 -0600

> > From: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>

> > Cc: Tom Tromey <tromey@adacore.com>

> >

> > Note that, in order to see the necessary declarations for this code to

> > work, the patch bumps _WIN32_WINNT from 0x0501 to 0x0602 -- that is,

> > Windows 8.  I don't know whether this may is a problem for anyone.

>

> It is for me.


Hm, not sure that that define is an issue for runtime per se.

> > +#include <winbase.h>

> > +#include <processthreadsapi.h>

>

> I don't think this is the right way of including Windows headers.

> Doesn't it work to just include <windows.h> (which nat/windows-nat.h

> already does)?  The MS documentation seems to say it should be enough.


It looks like gdb does not define WIN32_LEAN_AND_MEAN so I agree this
should not be necessary.

> > -/* We don't support Windows versions before XP, so we define

> > +/* We don't support Windows versions before Windows 8, so we define

> >     _WIN32_WINNT correspondingly to ensure the Windows API headers

> >     expose the required symbols.  */

> >  #if defined (__MINGW32__) || defined (__CYGWIN__)

> >  # ifdef _WIN32_WINNT

> > -#  if _WIN32_WINNT < 0x0501

> > +#  if _WIN32_WINNT < 0x0602

> >  #   undef _WIN32_WINNT

> > -#   define _WIN32_WINNT 0x0501

> > +#   define _WIN32_WINNT 0x0602

> >  #  endif

> >  # else

> > -#  define _WIN32_WINNT 0x0501

> > +#  define _WIN32_WINNT 0x0602

> >  # endif

> >  #endif       /* __MINGW32__ || __CYGWIN__ */

>

> will not help when building on older systems, and unnecessarily limits

> the GDB support to Windows 8 and later systems.


This does not affect *building* on older systems at all. But see above
re running on older systems.

Christian

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index 7ca1f842cb1..c8bacffe10e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,10 @@ 
 
 *** Changes since GDB 11
 
+* Windows 8 is the minimum supported version.
+
+* The disable-randomization now works on Windows.
+
 * New commands
 
 maint set backtrace-on-fatal-signal on|off
diff --git a/gdb/nat/windows-nat.c b/gdb/nat/windows-nat.c
index 29f03d37ba5..500782adc9f 100644
--- a/gdb/nat/windows-nat.c
+++ b/gdb/nat/windows-nat.c
@@ -19,6 +19,8 @@ 
 #include "gdbsupport/common-defs.h"
 #include "nat/windows-nat.h"
 #include "gdbsupport/common-debug.h"
+#include <winbase.h>
+#include <processthreadsapi.h>
 
 namespace windows_nat
 {
@@ -579,15 +581,80 @@  wait_for_debug_event (DEBUG_EVENT *event, DWORD timeout)
   return result;
 }
 
-/* Helper template for the CreateProcess wrappers.  */
-template<typename FUNC, typename CHAR, typename INFO>
+/* Helper template for the CreateProcess wrappers.
+
+   The EXTENDED type is the subtype of "startupinfo" to use, e.g.,
+   STARTUPINFOEXA.  FUNC is the type of the underlying CreateProcess
+   call.  CHAR is the character type to use, and INFO is the
+   "startupinfo" type to use.
+
+   DO_CREATE_PROCESS is the underlying CreateProcess function to use;
+   the remaining arguments are passed to it.  */
+template<typename EXTENDED, typename FUNC, typename CHAR, typename INFO>
 BOOL
 create_process_wrapper (FUNC *do_create_process, const CHAR *image,
 			CHAR *command_line, DWORD flags,
 			void *environment, const CHAR *cur_dir,
+			bool no_randomization,
 			INFO *startup_info,
 			PROCESS_INFORMATION *process_info)
 {
+  if (no_randomization)
+    {
+      static bool tried_and_failed;
+
+      if (!tried_and_failed)
+	{
+	  EXTENDED info_ex {};
+
+	  if (startup_info != nullptr)
+	    info_ex.StartupInfo = *startup_info;
+	  info_ex.StartupInfo.cb = sizeof (info_ex);
+	  SIZE_T size = 0;
+	  /* Ignore the result here.  The documentation says the first
+	     call always fails, by design.  */
+	  InitializeProcThreadAttributeList (nullptr, 1, 0, &size);
+	  info_ex.lpAttributeList
+	    = (PPROC_THREAD_ATTRIBUTE_LIST) alloca (size);
+	  InitializeProcThreadAttributeList (info_ex.lpAttributeList,
+					     1, 0, &size);
+
+	  gdb::optional<BOOL> return_value;
+	  DWORD attr_flags =
+	    (PROCESS_CREATION_MITIGATION_POLICY_FORCE_RELOCATE_IMAGES_ALWAYS_OFF
+	     | PROCESS_CREATION_MITIGATION_POLICY_BOTTOM_UP_ASLR_ALWAYS_OFF);
+	  if (!UpdateProcThreadAttribute (info_ex.lpAttributeList, 0,
+					  PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY,
+					  &attr_flags,
+					  sizeof (attr_flags),
+					  nullptr, nullptr))
+	    tried_and_failed = true;
+	  else
+	    {
+	      BOOL result = do_create_process (image, command_line,
+					       nullptr, nullptr,
+					       TRUE,
+					       (flags
+						| EXTENDED_STARTUPINFO_PRESENT),
+					       environment,
+					       cur_dir,
+					       (STARTUPINFO *) &info_ex,
+					       process_info);
+	      if (result)
+		return_value = result;
+	      else if (GetLastError () == ERROR_INVALID_PARAMETER)
+		tried_and_failed = true;
+	      else
+		return_value = FALSE;
+	    }
+
+	  DeleteProcThreadAttributeList (info_ex.lpAttributeList);
+
+	  if (return_value.has_value ())
+	    return *return_value;
+	}
+    }
+
   return do_create_process (image,
 			    command_line, /* command line */
 			    nullptr,	  /* Security */
@@ -605,12 +672,14 @@  create_process_wrapper (FUNC *do_create_process, const CHAR *image,
 BOOL
 create_process (const char *image, char *command_line, DWORD flags,
 		void *environment, const char *cur_dir,
+		bool no_randomization,
 		STARTUPINFOA *startup_info,
 		PROCESS_INFORMATION *process_info)
 {
-  return create_process_wrapper (CreateProcessA, image, command_line, flags,
-				 environment, cur_dir,
-				 startup_info, process_info);
+  return create_process_wrapper<STARTUPINFOEXA>
+    (CreateProcessA, image, command_line, flags,
+     environment, cur_dir, no_randomization,
+     startup_info, process_info);
 }
 
 #ifdef __CYGWIN__
@@ -620,12 +689,14 @@  create_process (const char *image, char *command_line, DWORD flags,
 BOOL
 create_process (const wchar_t *image, wchar_t *command_line, DWORD flags,
 		void *environment, const wchar_t *cur_dir,
+		bool no_randomization,
 		STARTUPINFOW *startup_info,
 		PROCESS_INFORMATION *process_info);
 {
-  return create_process_wrapper (CreateProcessW, image, command_line, flags,
-				 environment, cur_dir,
-				 startup_info, process_info);
+  return create_process_wrapper<STARTUPINFOEXW>
+    (CreateProcessW, image, command_line, flags,
+     environment, cur_dir, no_randomization,
+     startup_info, process_info);
 }
 
 #endif /* __CYGWIN__ */
diff --git a/gdb/nat/windows-nat.h b/gdb/nat/windows-nat.h
index 27f0e1fb4f6..978bd926405 100644
--- a/gdb/nat/windows-nat.h
+++ b/gdb/nat/windows-nat.h
@@ -263,17 +263,21 @@  extern BOOL continue_last_debug_event (DWORD continue_status,
 
 extern BOOL wait_for_debug_event (DEBUG_EVENT *event, DWORD timeout);
 
-/* Wrappers for CreateProcess.  */
+/* Wrappers for CreateProcess.  These exist primarily so that the
+   "disable randomization" feature can be implemented in a single
+   place.  */
 
 extern BOOL create_process (const char *image, char *command_line,
 			    DWORD flags, void *environment,
 			    const char *cur_dir,
+			    bool no_randomization,
 			    STARTUPINFOA *startup_info,
 			    PROCESS_INFORMATION *process_info);
 #ifdef __CYGWIN__
 extern BOOL create_process (const wchar_t *image, wchar_t *command_line,
 			    DWORD flags, void *environment,
 			    const wchar_t *cur_dir,
+			    bool no_randomization,
 			    STARTUPINFOW *startup_info,
 			    PROCESS_INFORMATION *process_info);
 #endif /* __CYGWIN__ */
@@ -282,10 +286,15 @@  extern BOOL create_process (const wchar_t *image, wchar_t *command_line,
 #define DebugActiveProcessStop		dyn_DebugActiveProcessStop
 #define DebugBreakProcess		dyn_DebugBreakProcess
 #define DebugSetProcessKillOnExit	dyn_DebugSetProcessKillOnExit
+#undef EnumProcessModules
 #define EnumProcessModules		dyn_EnumProcessModules
+#undef EnumProcessModulesEx
 #define EnumProcessModulesEx		dyn_EnumProcessModulesEx
+#undef GetModuleInformation
 #define GetModuleInformation		dyn_GetModuleInformation
+#undef GetModuleFileNameExA
 #define GetModuleFileNameExA		dyn_GetModuleFileNameExA
+#undef GetModuleFileNameExW
 #define GetModuleFileNameExW		dyn_GetModuleFileNameExW
 #define LookupPrivilegeValueA		dyn_LookupPrivilegeValueA
 #define OpenProcessToken		dyn_OpenProcessToken
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 87bfd8969fa..468ac63ce9a 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -283,6 +283,11 @@  struct windows_nat_target final : public x86_nat_target<inf_child_target>
   int get_windows_debug_event (int pid, struct target_waitstatus *ourstatus);
 
   void do_initial_windows_stuff (DWORD pid, bool attaching);
+
+  bool supports_disable_randomization () override
+  {
+    return true;
+  }
 };
 
 static windows_nat_target the_windows_nat_target;
@@ -2675,6 +2680,7 @@  windows_nat_target::create_inferior (const char *exec_file,
   windows_init_thread_list ();
   ret = create_process (args, flags, w32_env,
 			inferior_cwd != nullptr ? infcwd : nullptr,
+			disable_randomization,
 			&si, &pi);
   if (w32_env)
     /* Just free the Win32 environment, if it could be created. */
@@ -2794,6 +2800,7 @@  windows_nat_target::create_inferior (const char *exec_file,
 			flags,	/* start flags */
 			w32env,	/* environment */
 			inferior_cwd, /* current directory */
+			disable_randomization,
 			&si,
 			&pi);
   if (tty != INVALID_HANDLE_VALUE)
diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 0e96a22bfed..5570498fde4 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -580,6 +580,7 @@  create_process (const char *program, char *args,
 			(inferior_cwd.empty ()
 			 ? NULL
 			 : gdb_tilde_expand (inferior_cwd.c_str ()).c_str()),
+			get_client_state ().disable_randomization,
 			&si,               /* start info */
 			pi);               /* proc info */
 
diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
index 11c318e25a5..5f45f7a8998 100644
--- a/gdbserver/win32-low.h
+++ b/gdbserver/win32-low.h
@@ -158,6 +158,11 @@  class win32_process_target : public process_stratum_target
   bool stopped_by_sw_breakpoint () override;
 
   bool supports_stopped_by_sw_breakpoint () override;
+
+  bool supports_disable_randomization () override
+  {
+    return true;
+  }
 };
 
 /* Retrieve the context for this thread, if not already retrieved.  */
diff --git a/gdbsupport/common-defs.h b/gdbsupport/common-defs.h
index 56780765756..996d25d5a1e 100644
--- a/gdbsupport/common-defs.h
+++ b/gdbsupport/common-defs.h
@@ -68,17 +68,17 @@ 
 #define _FORTIFY_SOURCE 2
 #endif
 
-/* We don't support Windows versions before XP, so we define
+/* We don't support Windows versions before Windows 8, so we define
    _WIN32_WINNT correspondingly to ensure the Windows API headers
    expose the required symbols.  */
 #if defined (__MINGW32__) || defined (__CYGWIN__)
 # ifdef _WIN32_WINNT
-#  if _WIN32_WINNT < 0x0501
+#  if _WIN32_WINNT < 0x0602
 #   undef _WIN32_WINNT
-#   define _WIN32_WINNT 0x0501
+#   define _WIN32_WINNT 0x0602
 #  endif
 # else
-#  define _WIN32_WINNT 0x0501
+#  define _WIN32_WINNT 0x0602
 # endif
 #endif	/* __MINGW32__ || __CYGWIN__ */