Emacs master, security concernes, ms-windows

Emacs master, security concernes, ms-windows

[Fabrice Popineau]

[-- Attachment #1.1: Type: text/plain, Size: 531 bytes --]

Since there seems to be a lot of concerns wrt to security,
I am submitting the attached patch.

The reason for this patch is to limit the search for dlls loaded at
runtime to the win32 system directory and/or the emacs application
directory.
In the current state, dlls can be picked up in any directory in the path.
Some one could fake one of these dlls (xpm, png, etc.) and use it for
mean reasons.
It is not bullet proof, but it levels up security and
many other projects have applied such a restriction.

Best regards,

Fabrice

[-- Attachment #1.2: Type: text/html, Size: 772 bytes --]

[-- Attachment #2: emacs-loadlibrary.diff --]
[-- Type: text/plain, Size: 15300 bytes --]

diff --git a/src/unexw32.c b/src/unexw32.c
index 5259b2a52b..10f720f734 100644
--- a/src/unexw32.c
+++ b/src/unexw32.c
@@ -772,7 +820,7 @@ unexec (const char *new_name, const char *old_name)
   {
     PIMAGE_DOS_HEADER dos_header;
     PIMAGE_NT_HEADERS nt_header;
-    HANDLE hImagehelp = LoadLibrary ("imagehlp.dll");
+    HANDLE hImagehelp = LoadLibraryEx ("imagehlp.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
     DWORD  headersum;
     DWORD  checksum;
 
diff --git a/src/w32.c b/src/w32.c
index f583d5e76c..a437d3fd83 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -565,7 +565,7 @@ open_process_token (HANDLE ProcessHandle,
   if (g_b_init_open_process_token == 0)
     {
       g_b_init_open_process_token = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Open_Process_Token =
         (OpenProcessToken_Proc) GetProcAddress (hm_advapi32, "OpenProcessToken");
     }
@@ -597,7 +597,7 @@ get_token_information (HANDLE TokenHandle,
   if (g_b_init_get_token_information == 0)
     {
       g_b_init_get_token_information = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Get_Token_Information =
         (GetTokenInformation_Proc) GetProcAddress (hm_advapi32, "GetTokenInformation");
     }
@@ -633,7 +633,7 @@ lookup_account_sid (LPCTSTR lpSystemName,
   if (g_b_init_lookup_account_sid == 0)
     {
       g_b_init_lookup_account_sid = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Lookup_Account_Sid =
         (LookupAccountSid_Proc) GetProcAddress (hm_advapi32, LookupAccountSid_Name);
     }
@@ -666,7 +666,7 @@ get_sid_sub_authority (PSID pSid, DWORD n)
   if (g_b_init_get_sid_sub_authority == 0)
     {
       g_b_init_get_sid_sub_authority = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Get_Sid_Sub_Authority =
         (GetSidSubAuthority_Proc) GetProcAddress (
             hm_advapi32, "GetSidSubAuthority");
@@ -691,7 +691,7 @@ get_sid_sub_authority_count (PSID pSid)
   if (g_b_init_get_sid_sub_authority_count == 0)
     {
       g_b_init_get_sid_sub_authority_count = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Get_Sid_Sub_Authority_Count =
         (GetSidSubAuthorityCount_Proc) GetProcAddress (
             hm_advapi32, "GetSidSubAuthorityCount");
@@ -722,7 +722,7 @@ get_security_info (HANDLE handle,
   if (g_b_init_get_security_info == 0)
     {
       g_b_init_get_security_info = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Get_Security_Info =
         (GetSecurityInfo_Proc) GetProcAddress (
             hm_advapi32, "GetSecurityInfo");
@@ -758,7 +758,7 @@ get_file_security (const char *lpFileName,
       if (g_b_init_get_file_security_w == 0)
 	{
           g_b_init_get_file_security_w = 1;
-	  hm_advapi32 = LoadLibrary ("Advapi32.dll");
+          hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
           s_pfn_Get_File_SecurityW =
             (GetFileSecurityW_Proc) GetProcAddress (hm_advapi32,
                                                    "GetFileSecurityW");
@@ -780,7 +780,7 @@ get_file_security (const char *lpFileName,
       if (g_b_init_get_file_security_a == 0)
 	{
           g_b_init_get_file_security_a = 1;
-	  hm_advapi32 = LoadLibrary ("Advapi32.dll");
+          hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
           s_pfn_Get_File_SecurityA =
             (GetFileSecurityA_Proc) GetProcAddress (hm_advapi32,
                                                    "GetFileSecurityA");
@@ -817,7 +817,7 @@ set_file_security (const char *lpFileName,
       if (g_b_init_set_file_security_w == 0)
 	{
           g_b_init_set_file_security_w = 1;
-	  hm_advapi32 = LoadLibrary ("Advapi32.dll");
+          hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
           s_pfn_Set_File_SecurityW =
             (SetFileSecurityW_Proc) GetProcAddress (hm_advapi32,
                                                     "SetFileSecurityW");
@@ -838,7 +838,7 @@ set_file_security (const char *lpFileName,
       if (g_b_init_set_file_security_a == 0)
 	{
           g_b_init_set_file_security_a = 1;
-	  hm_advapi32 = LoadLibrary ("Advapi32.dll");
+          hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
           s_pfn_Set_File_SecurityA =
             (SetFileSecurityA_Proc) GetProcAddress (hm_advapi32,
                                                     "SetFileSecurityA");
@@ -878,7 +878,7 @@ set_named_security_info (LPCTSTR lpObjectName,
       if (g_b_init_set_named_security_info_w == 0)
 	{
           g_b_init_set_named_security_info_w = 1;
-	  hm_advapi32 = LoadLibrary ("Advapi32.dll");
+          hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
           s_pfn_Set_Named_Security_InfoW =
             (SetNamedSecurityInfoW_Proc) GetProcAddress (hm_advapi32,
                                                          "SetNamedSecurityInfoW");
@@ -900,7 +900,7 @@ set_named_security_info (LPCTSTR lpObjectName,
       if (g_b_init_set_named_security_info_a == 0)
 	{
           g_b_init_set_named_security_info_a = 1;
-	  hm_advapi32 = LoadLibrary ("Advapi32.dll");
+          hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
           s_pfn_Set_Named_Security_InfoA =
             (SetNamedSecurityInfoA_Proc) GetProcAddress (hm_advapi32,
                                                          "SetNamedSecurityInfoA");
@@ -932,7 +932,7 @@ get_security_descriptor_owner (PSECURITY_DESCRIPTOR pSecurityDescriptor,
   if (g_b_init_get_security_descriptor_owner == 0)
     {
       g_b_init_get_security_descriptor_owner = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Get_Security_Descriptor_Owner =
         (GetSecurityDescriptorOwner_Proc) GetProcAddress (
             hm_advapi32, "GetSecurityDescriptorOwner");
@@ -961,7 +961,7 @@ get_security_descriptor_group (PSECURITY_DESCRIPTOR pSecurityDescriptor,
   if (g_b_init_get_security_descriptor_group == 0)
     {
       g_b_init_get_security_descriptor_group = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Get_Security_Descriptor_Group =
         (GetSecurityDescriptorGroup_Proc) GetProcAddress (
             hm_advapi32, "GetSecurityDescriptorGroup");
@@ -991,7 +991,7 @@ get_security_descriptor_dacl (PSECURITY_DESCRIPTOR pSecurityDescriptor,
   if (g_b_init_get_security_descriptor_dacl == 0)
     {
       g_b_init_get_security_descriptor_dacl = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Get_Security_Descriptor_Dacl =
         (GetSecurityDescriptorDacl_Proc) GetProcAddress (
             hm_advapi32, "GetSecurityDescriptorDacl");
@@ -1018,7 +1018,7 @@ is_valid_sid (PSID sid)
   if (g_b_init_is_valid_sid == 0)
     {
       g_b_init_is_valid_sid = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Is_Valid_Sid =
         (IsValidSid_Proc) GetProcAddress (
             hm_advapi32, "IsValidSid");
@@ -1042,7 +1042,7 @@ equal_sid (PSID sid1, PSID sid2)
   if (g_b_init_equal_sid == 0)
     {
       g_b_init_equal_sid = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Equal_Sid =
         (EqualSid_Proc) GetProcAddress (
             hm_advapi32, "EqualSid");
@@ -1066,7 +1066,7 @@ get_length_sid (PSID sid)
   if (g_b_init_get_length_sid == 0)
     {
       g_b_init_get_length_sid = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Get_Length_Sid =
         (GetLengthSid_Proc) GetProcAddress (
             hm_advapi32, "GetLengthSid");
@@ -1090,7 +1090,7 @@ copy_sid (DWORD destlen, PSID dest, PSID src)
   if (g_b_init_copy_sid == 0)
     {
       g_b_init_copy_sid = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Copy_Sid =
         (CopySid_Proc) GetProcAddress (
             hm_advapi32, "CopySid");
@@ -1362,7 +1362,7 @@ get_adapters_info (PIP_ADAPTER_INFO pAdapterInfo, PULONG pOutBufLen)
   if (g_b_init_get_adapters_info == 0)
     {
       g_b_init_get_adapters_info = 1;
-      hm_iphlpapi = LoadLibrary ("Iphlpapi.dll");
+      hm_iphlpapi = LoadLibraryEx ("Iphlpapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       if (hm_iphlpapi)
 	s_pfn_Get_Adapters_Info = (GetAdaptersInfo_Proc)
           GetProcAddress (hm_iphlpapi, "GetAdaptersInfo");
@@ -6620,7 +6624,7 @@ open_thread_token (HANDLE ThreadHandle,
   if (g_b_init_open_thread_token == 0)
     {
       g_b_init_open_thread_token = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Open_Thread_Token =
         (OpenThreadToken_Proc) GetProcAddress (hm_advapi32, "OpenThreadToken");
     }
@@ -6650,7 +6654,7 @@ impersonate_self (SECURITY_IMPERSONATION_LEVEL ImpersonationLevel)
   if (g_b_init_impersonate_self == 0)
     {
       g_b_init_impersonate_self = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Impersonate_Self =
         (ImpersonateSelf_Proc) GetProcAddress (hm_advapi32, "ImpersonateSelf");
     }
@@ -6673,7 +6677,7 @@ revert_to_self (void)
   if (g_b_init_revert_to_self == 0)
     {
       g_b_init_revert_to_self = 1;
-      hm_advapi32 = LoadLibrary ("Advapi32.dll");
+      hm_advapi32 = LoadLibraryEx ("Advapi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_Revert_To_Self =
         (RevertToSelf_Proc) GetProcAddress (hm_advapi32, "RevertToSelf");
     }
@@ -6698,7 +6702,7 @@ get_process_memory_info (HANDLE h_proc,
   if (g_b_init_get_process_memory_info == 0)
     {
       g_b_init_get_process_memory_info = 1;
-      hm_psapi = LoadLibrary ("Psapi.dll");
+      hm_psapi = LoadLibraryEx ("Psapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       if (hm_psapi)
 	s_pfn_Get_Process_Memory_Info = (GetProcessMemoryInfo_Proc)
           GetProcAddress (hm_psapi, "GetProcessMemoryInfo");
@@ -7348,7 +7352,7 @@ init_winsock (int load_now)
     = (void *) GetProcAddress (GetModuleHandle ("kernel32.dll"),
                                "SetHandleInformation");
 
-  winsock_lib = LoadLibrary ("Ws2_32.dll");
+  winsock_lib = LoadLibraryEx ("Ws2_32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
 
   if (winsock_lib != NULL)
     {
@@ -9316,7 +9320,8 @@ w32_delayed_load (Lisp_Object library_id)
 		wchar_t name_w[MAX_PATH];
 
 		filename_to_utf16 (SSDATA (dll), name_w);
-		dll_handle = LoadLibraryW (name_w);
+		dll_handle = LoadLibraryExW (name_w, NULL, LOAD_LIBRARY_SEARCH_APPLICATION_DIR|LOAD_LIBRARY_SEARCH_SYSTEM32);
+
 		if (dll_handle)
                   {
                     res = GetModuleFileNameW (dll_handle, name_w,
@@ -9330,7 +9335,8 @@ w32_delayed_load (Lisp_Object library_id)
 		char name_a[MAX_PATH];
 
 		filename_to_ansi (SSDATA (dll), name_a);
-		dll_handle = LoadLibraryA (name_a);
+		dll_handle = LoadLibraryExA (name_a, NULL, LOAD_LIBRARY_SEARCH_APPLICATION_DIR|LOAD_LIBRARY_SEARCH_SYSTEM32);
+
 		if (dll_handle)
                   {
                     res = GetModuleFileNameA (dll_handle, name_a,
@@ -9590,7 +9596,7 @@ maybe_load_unicows_dll (void)
 {
   if (os_subtype == OS_9X)
     {
-      HANDLE ret = LoadLibrary ("Unicows.dll");
+      HANDLE ret = LoadLibraryEx ("Unicows.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       if (ret)
 	{
           /* These two functions are present on Windows 9X as stubs
@@ -9638,7 +9644,7 @@ maybe_load_unicows_dll (void)
         multiByteToWideCharFlags = 0;
       else
         multiByteToWideCharFlags = MB_ERR_INVALID_CHARS;
-      return LoadLibrary ("Gdi32.dll");
+      return LoadLibraryEx ("Gdi32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
     }
 }
 
diff --git a/src/w32fns.c b/src/w32fns.c
index 6b93afa8b8..c566ebc24e 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -10897,9 +10898,9 @@ w32_backtrace (void **buffer, int limit)
   static CaptureStackBackTrace_proc s_pfn_CaptureStackBackTrace = NULL;
   HMODULE hm_kernel32 = NULL;
 
   if (!s_pfn_CaptureStackBackTrace)
     {
-      hm_kernel32 = LoadLibrary ("Kernel32.dll");
+      hm_kernel32 = LoadLibraryEx ("Kernel32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       s_pfn_CaptureStackBackTrace =
 	(CaptureStackBackTrace_proc) GetProcAddress (hm_kernel32,
                                                      "RtlCaptureStackBackTrace");
diff --git a/src/w32heap.c b/src/w32heap.c
index cd1324cc86..30b4304aaa 100644
--- a/src/w32heap.c
+++ b/src/w32heap.c
@@ -249,7 +249,7 @@ init_heap (void)
 
 #ifndef MINGW_W64
       /* Set the low-fragmentation heap for OS before Vista.  */
-      HMODULE hm_kernel32dll = LoadLibrary ("kernel32.dll");
+      HMODULE hm_kernel32dll = LoadLibraryEx ("kernel32.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       HeapSetInformation_Proc s_pfn_Heap_Set_Information = (HeapSetInformation_Proc) GetProcAddress (hm_kernel32dll, "HeapSetInformation");
       if (s_pfn_Heap_Set_Information != NULL)
 	{
@@ -279,7 +279,7 @@ init_heap (void)
       /* Find the RtlCreateHeap function.  Headers for this function
          are provided with the w32 DDK, but the function is available
          in ntdll.dll since XP.  */
-      HMODULE hm_ntdll = LoadLibrary ("ntdll.dll");
+      HMODULE hm_ntdll = LoadLibraryEx ("ntdll.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);
       RtlCreateHeap_Proc s_pfn_Rtl_Create_Heap
 	= (RtlCreateHeap_Proc) GetProcAddress (hm_ntdll, "RtlCreateHeap");
       /* Specific parameters for the private heap.  */
diff --git a/src/w32proc.c b/src/w32proc.c
index 4459ebe324..7b51043c99 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -3609,7 +3620,7 @@ w32_compare_strings (const char *s1, const char *s2, char *locname,
       if (os_subtype == OS_9X)
 	{
           pCompareStringW =
-            (CompareStringW_Proc) GetProcAddress (LoadLibrary ("Unicows.dll"),
+            (CompareStringW_Proc) GetProcAddress (LoadLibraryEx ("Unicows.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32),
                                                   "CompareStringW");
           if (!pCompareStringW)
             {

Re: Emacs master, security concernes, ms-windows

[Óscar Fuentes]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

> Since there seems to be a lot of concerns wrt to security,
> I am submitting the attached patch.
>
> The reason for this patch is to limit the search for dlls loaded at
> runtime to the win32 system directory and/or the emacs application
> directory.
> In the current state, dlls can be picked up in any directory in the path.
> Some one could fake one of these dlls (xpm, png, etc.) and use it for
> mean reasons.
> It is not bullet proof, but it levels up security and
> many other projects have applied such a restriction.
>
> Best regards,
>
> Fabrice
> diff --git a/src/unexw32.c b/src/unexw32.c
> index 5259b2a52b..10f720f734 100644
> --- a/src/unexw32.c
> +++ b/src/unexw32.c
> @@ -772,7 +820,7 @@ unexec (const char *new_name, const char *old_name)
>    {
>      PIMAGE_DOS_HEADER dos_header;
>      PIMAGE_NT_HEADERS nt_header;
> -    HANDLE hImagehelp = LoadLibrary ("imagehlp.dll");
> +    HANDLE hImagehelp = LoadLibraryEx ("imagehlp.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32);

According do MSDN, LoadLibraryEx does not support
LOAD_LIBRARY_SEARCH_SYSTEM32 nor LOAD_LIBRARY_SEARCH_APPLICATION_DIR on
Windows XP, and on Windows 7 and some other version it requires certain
security system patch to be installed.

Apart from that, the security provided by this approach is questionable.
If the attacker has enough control to install a DLL and modify the PATH,
it is game over.

Finally, this patch can be a hindrance for those who build Emacs. After
the build is over, you need to copy the required extra dlls (for image
support, etc) to the build binary directory to test or use Emacs. Not a
huge inconvenience, but it isn't irrelevant either.

Re: Emacs master, security concernes, ms-windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

2017-09-14 15:33 GMT+02:00 Óscar Fuentes <ofv@wanadoo.es>:

> Fabrice Popineau <fabrice.popineau@gmail.com> writes:
>
> Apart from that, the security provided by this approach is questionable.
> If the attacker has enough control to install a DLL and modify the PATH,
> it is game over.
>

At the moment, any libpng.dll (for example) on the PATH can be loaded by
emacs.
With this restriction, only the one provided with an emacs package will be.

I came to 'fix' this because I am using the Anaconda Python distribution
which also
provides its own set of dlls. At some point I got a failure because their
dlls got loaded,
instead of the mingw64 ones.

Finally, this patch can be a hindrance for those who build Emacs. After
> the build is over, you need to copy the required extra dlls (for image
> support, etc) to the build binary directory to test or use Emacs. Not a
> huge inconvenience, but it isn't irrelevant either.
>
>
That's true.

-- 
Fabrice

[-- Attachment #2: Type: text/html, Size: 1792 bytes --]

Re: Emacs master, security concernes, ms-windows

[Óscar Fuentes]

Fabrice Popineau <fabrice.popineau@centralesupelec.fr> writes:

> At the moment, any libpng.dll (for example) on the PATH can be loaded by
> emacs.
> With this restriction, only the one provided with an emacs package will be.
>
> I came to 'fix' this because I am using the Anaconda Python distribution
> which also
> provides its own set of dlls. At some point I got a failure because their
> dlls got loaded,
> instead of the mingw64 ones.

I suffered a similar problem on the past.

The real concern about this patch is that it raises quite a bit the
minimum supported OS version, which is a big no-no, AFAIK.

Although I'm not sure what happens if you pass an unsupported flag to
LoadLibraryEx.

It is possible to implement your idea by using explicit paths.

Re: Emacs master, security concernes, ms-windows

[Fabrice Popineau]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

2017-09-14 17:20 GMT+02:00 Óscar Fuentes <ofv@wanadoo.es>:

> Fabrice Popineau <fabrice.popineau@centralesupelec.fr> writes:
>
> > At the moment, any libpng.dll (for example) on the PATH can be loaded by
> > emacs.
> > With this restriction, only the one provided with an emacs package will
> be.
> >
> > I came to 'fix' this because I am using the Anaconda Python distribution
> > which also
> > provides its own set of dlls. At some point I got a failure because their
> > dlls got loaded,
> > instead of the mingw64 ones.
>
> I suffered a similar problem on the past.
>
> The real concern about this patch is that it raises quite a bit the
> minimum supported OS version, which is a big no-no, AFAIK.
>
> Although I'm not sure what happens if you pass an unsupported flag to
> LoadLibraryEx.
>
>


> It is possible to implement your idea by using explicit paths.
>

Yes, emacs could itself decide where to look for these dlls.
At least for the ones  that are non system.
And the system ones should be looked for only in the System32 directory.

I know that my patch was a radical one. I mainly wanted to raise the issue.

Fabrice

[-- Attachment #2: Type: text/html, Size: 1939 bytes --]

Re: Emacs master, security concernes, ms-windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@centralesupelec.fr>
> Date: Thu, 14 Sep 2017 17:56:46 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> And the system ones should be looked for only in the System32 directory.

Actually, I think all of the system DLLs we try to load dynamically
don't need to be loaded except on Windows 9X.  So we could install a
patch that refrains from loading dynamically any system DLLs on the NT
family of Windows.  Any volunteers?

Re: Emacs master, security concernes, ms-windows

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Thu, 14 Sep 2017 17:20:39 +0200
> 
> The real concern about this patch is that it raises quite a bit the
> minimum supported OS version, which is a big no-no, AFAIK.

That could be solved by having this code run only on systems that
support the flag.

So I think the real concern is the other issues that this patch
raises.

Re: Emacs master, security concernes, ms-windows

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Thu, 14 Sep 2017 15:33:40 +0200
> 
> Finally, this patch can be a hindrance for those who build Emacs. After
> the build is over, you need to copy the required extra dlls (for image
> support, etc) to the build binary directory to test or use Emacs. Not a
> huge inconvenience, but it isn't irrelevant either.

Indeed.  And in addition, there are Emacs modules, which are also
DLLs, but loaded from different places.  (The patch didn't touch that
facility, but they should probably get the same treatment.)

Re: Emacs master, security concernes, ms-windows

[Andy Moreton]

On Thu 14 Sep 2017, Fabrice Popineau wrote:

> Since there seems to be a lot of concerns wrt to security,
> I am submitting the attached patch.
>
> The reason for this patch is to limit the search for dlls loaded at
> runtime to the win32 system directory and/or the emacs application
> directory.
> In the current state, dlls can be picked up in any directory in the path.
> Some one could fake one of these dlls (xpm, png, etc.) and use it for
> mean reasons.
> It is not bullet proof, but it levels up security and
> many other projects have applied such a restriction.

Restricting the path used to locate DLLs is reasonable, but this is too
strict.

For an emacs built from source and run from the build tree (i.e. not
installed), this requires copying all of the distro DLLs to the emacs
build directory, and keeping them up to date.

Also note that the patch uses LOAD_LIBRARY_SEARCH_* flags, which won't
work at all on Windows XP, or on newer Windows versions that do not have
the correct updates installed. See:

Dynamic-Link Library Security:
https://msdn.microsoft.com/en-us/library/windows/desktop/ff919712(v=vs.85).aspx

Dynamic-Link Library Search Order:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682586(v=vs.85).aspx

LoadLibraryEx:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx

    AndyM

Re: Emacs master, security concernes, ms-windows

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@gmail.com>
> Date: Thu, 14 Sep 2017 09:58:36 +0200
> 
> Since there seems to be a lot of concerns wrt to security,
> I am submitting the attached patch.
> 
> The reason for this patch is to limit the search for dlls loaded at 
> runtime to the win32 system directory and/or the emacs application
> directory.
> In the current state, dlls can be picked up in any directory in the path.
> Some one could fake one of these dlls (xpm, png, etc.) and use it for
> mean reasons. 

I see your point, and I'm sympathetic to the concerns.  However, given
the accepted practices of installing packages on Windows, I don't
think we can do this by default.  Many times people install optional
libraries in separate directories and just add them to PATH.  Some
even install each individual package into its own directory.  I just
had a conversation off-list with one such user.  Disabling DLL look up
on PATH by default will completely screw up those users.

In addition, as others pointed out, this mode is not supported before
Vista, so we shouldn't pass that constant to LoadLibraryEx on systems
that don't support it, even if we accept such a feature as an optional
one.

Thanks.