In order to avoid DLL preloading attacks, we should always load our system
dependencies using the helper that only searches the system library path.

Thanks to Mohsen Mirzakhani and Utkarsh Satya Prakash for bringing this to
our attention.

Signed-off-by: Chris Waldon <christopher.waldon.dev@gmail.com>
---
 app/log_windows.go          | 5 +++--
 internal/egl/egl_windows.go | 4 ++--
 internal/gl/gl_windows.go   | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/app/log_windows.go b/app/log_windows.go
index f6f654cf..17de2916 100644
--- a/app/log_windows.go
+++ b/app/log_windows.go
@@ -4,14 +4,15 @@ package app
 
 import (
 	"log"
-	"syscall"
 	"unsafe"
+
+	syscall "golang.org/x/sys/windows"
 )
 
 type logger struct{}
 
 var (
-	kernel32           = syscall.NewLazyDLL("kernel32")
+	kernel32           = syscall.NewLazySystemDLL("kernel32")
 	outputDebugStringW = kernel32.NewProc("OutputDebugStringW")
 	debugView          *logger
 )
diff --git a/internal/egl/egl_windows.go b/internal/egl/egl_windows.go
index 4433dd79..4e226c57 100644
--- a/internal/egl/egl_windows.go
+++ b/internal/egl/egl_windows.go
@@ -24,7 +24,7 @@ type (
 )
 
 var (
-	libEGL                  = syscall.NewLazyDLL("libEGL.dll")
+	libEGL                  = syscall.NewLazySystemDLL("libEGL.dll")
 	_eglChooseConfig        = libEGL.NewProc("eglChooseConfig")
 	_eglCreateContext       = libEGL.NewProc("eglCreateContext")
 	_eglCreateWindowSurface = libEGL.NewProc("eglCreateWindowSurface")
@@ -61,7 +61,7 @@ func loadDLLs() error {
 		return err
 	}
 	// d3dcompiler_47.dll is needed internally for shader compilation to function.
-	return loadDLL(syscall.NewLazyDLL("d3dcompiler_47.dll"), "d3dcompiler_47.dll")
+	return loadDLL(syscall.NewLazySystemDLL("d3dcompiler_47.dll"), "d3dcompiler_47.dll")
 }
 
 func loadDLL(dll *syscall.LazyDLL, name string) error {
diff --git a/internal/gl/gl_windows.go b/internal/gl/gl_windows.go
index a30457fd..f3a21525 100644
--- a/internal/gl/gl_windows.go
+++ b/internal/gl/gl_windows.go
@@ -12,7 +12,7 @@ import (
 )
 
 var (
-	LibGLESv2                              = windows.NewLazyDLL("libGLESv2.dll")
+	LibGLESv2                              = windows.NewLazySystemDLL("libGLESv2.dll")
 	_glActiveTexture                       = LibGLESv2.NewProc("glActiveTexture")
 	_glAttachShader                        = LibGLESv2.NewProc("glAttachShader")
 	_glBeginQuery                          = LibGLESv2.NewProc("glBeginQuery")
-- 
2.47.1

gio/patches: FAILED in 9m59s

[app,internal/{egl,gl}: [Windows] always use NewLazySystemDLL][0] from [Chris Waldon][1]

[0]: https://lists.sr.ht/~eliasnaur/gio-patches/patches/56833
[1]: christopher.waldon.dev@gmail.com

✗ #1405125 FAILED  gio/patches/apple.yml   https://builds.sr.ht/~eliasnaur/job/1405125
✓ #1405128 SUCCESS gio/patches/openbsd.yml https://builds.sr.ht/~eliasnaur/job/1405128
✓ #1405127 SUCCESS gio/patches/linux.yml   https://builds.sr.ht/~eliasnaur/job/1405127
✗ #1405126 FAILED  gio/patches/freebsd.yml https://builds.sr.ht/~eliasnaur/job/1405126

On Thu Jan 9, 2025 at 2:15 AM CET, Chris Waldon wrote:
> In order to avoid DLL preloading attacks, we should always load our system
> dependencies using the helper that only searches the system library path.
>
> Thanks to Mohsen Mirzakhani and Utkarsh Satya Prakash for bringing this to
> our attention.
>

Thanks. I applied the change to package app (kernel32.dll is a system dll). Are
you sure you want EGL and GLES loaded from the system DLL directory only? What
if a program supplies its own EGL/GLES libraries along with its executable?
I *think* I remember Plato doing this.

> Signed-off-by: Chris Waldon <christopher.waldon.dev@gmail.com>
> ---
>  app/log_windows.go          | 5 +++--
>  internal/egl/egl_windows.go | 4 ++--
>  internal/gl/gl_windows.go   | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/app/log_windows.go b/app/log_windows.go
> index f6f654cf..17de2916 100644
> --- a/app/log_windows.go
> +++ b/app/log_windows.go
> @@ -4,14 +4,15 @@ package app
>  
>  import (
>  	"log"
> -	"syscall"
>  	"unsafe"
> +
> +	syscall "golang.org/x/sys/windows"
>  )
>  
>  type logger struct{}
>  
>  var (
> -	kernel32           = syscall.NewLazyDLL("kernel32")
> +	kernel32           = syscall.NewLazySystemDLL("kernel32")

Merged.

>  	outputDebugStringW = kernel32.NewProc("OutputDebugStringW")
>  	debugView          *logger
>  )
> diff --git a/internal/egl/egl_windows.go b/internal/egl/egl_windows.go
> index 4433dd79..4e226c57 100644
> --- a/internal/egl/egl_windows.go
> +++ b/internal/egl/egl_windows.go
> @@ -24,7 +24,7 @@ type (
>  )
>  
>  var (
> -	libEGL                  = syscall.NewLazyDLL("libEGL.dll")
> +	libEGL                  = syscall.NewLazySystemDLL("libEGL.dll")
>  	_eglChooseConfig        = libEGL.NewProc("eglChooseConfig")
>  	_eglCreateContext       = libEGL.NewProc("eglCreateContext")
>  	_eglCreateWindowSurface = libEGL.NewProc("eglCreateWindowSurface")
> @@ -61,7 +61,7 @@ func loadDLLs() error {
>  		return err
>  	}
>  	// d3dcompiler_47.dll is needed internally for shader compilation to function.
> -	return loadDLL(syscall.NewLazyDLL("d3dcompiler_47.dll"), "d3dcompiler_47.dll")
> +	return loadDLL(syscall.NewLazySystemDLL("d3dcompiler_47.dll"), "d3dcompiler_47.dll")
>  }
>  
>  func loadDLL(dll *syscall.LazyDLL, name string) error {
> diff --git a/internal/gl/gl_windows.go b/internal/gl/gl_windows.go
> index a30457fd..f3a21525 100644
> --- a/internal/gl/gl_windows.go
> +++ b/internal/gl/gl_windows.go
> @@ -12,7 +12,7 @@ import (
>  )
>  
>  var (
> -	LibGLESv2                              = windows.NewLazyDLL("libGLESv2.dll")
> +	LibGLESv2                              = windows.NewLazySystemDLL("libGLESv2.dll")
>  	_glActiveTexture                       = LibGLESv2.NewProc("glActiveTexture")
>  	_glAttachShader                        = LibGLESv2.NewProc("glAttachShader")
>  	_glBeginQuery                          = LibGLESv2.NewProc("glBeginQuery")

On Thu, Jan 9, 2025 at 3:58 AM Elias Naur <mail@eliasnaur.com> wrote:
>
> On Thu Jan 9, 2025 at 2:15 AM CET, Chris Waldon wrote:
> > In order to avoid DLL preloading attacks, we should always load our system
> > dependencies using the helper that only searches the system library path.
> >
> > Thanks to Mohsen Mirzakhani and Utkarsh Satya Prakash for bringing this to
> > our attention.
> >
>
> Thanks. I applied the change to package app (kernel32.dll is a system dll). Are
> you sure you want EGL and GLES loaded from the system DLL directory only? What
> if a program supplies its own EGL/GLES libraries along with its executable?
> I *think* I remember Plato doing this.

I was actually misremembering how the custom rendering worked in
applications like Plato when I wrote this. Yes, we use this code to
load an application-provided GL implementation. Given that, hardening
this gets considerably harder. I'm inclined to rewrite the load to use
LoadLibraryEx with the LOAD_LIBRARY_REQUIRE_SIGNED_TARGET flag
discussed here [0]. This would ensure that the target DLL is at least
trusted by the system (if I understand it correctly). It would,
however, make building custom rendering applications on Windows harder
by introducing the requirement to get a valid code signature on your
custom GL implementation. There's no way to do that for free; the
signing certs cost $100/yr. Perhaps we should offer something like a
build tag or other mechanism applications can use to opt out of
enforcing signed DLLs?

I'm uncertain of the true scope of damage this DLL preloading opens us
up to. I'm sure it's application-dependent, and some applications
probably want to be really sure they aren't hijacked via DLLs. I'm
inclined to think we should do something to tighten this, but it's
hard to know what that looks like

Cheers,
Chris

[0] https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa