Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gcc: Improve security #22216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

gcc: Improve security #22216

wants to merge 1 commit into from

Conversation

lhmouse
Copy link
Contributor

@lhmouse lhmouse commented Oct 17, 2024

gcc: Improve security

The language-specific compilers (cc1, cc1plus, lto-wrapper, etc.) are not in
PATH, but in '/lib/gcc//'. When these compilers are
invoked by GCC, they prefer DLLs in the working directory to those in PATH [1],
which allows, for example, an untrusted source repo to create libgmp-10.dll in
the working directory, which will get picked by cc1plus, resulting in arbitrary
code execution.

These programs shall be linked against all dependencies statically.

[1] https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security

Signed-off-by: LIU Hao [email protected]

@lhmouse lhmouse force-pushed the gcc-pkg branch 2 times, most recently from 38785be to c4d0ae8 Compare October 17, 2024 12:53
@mati865
Copy link
Collaborator

mati865 commented Oct 17, 2024

Nit: packages that are linked statically don't need to be in depends table.

@lhmouse
Copy link
Contributor Author

lhmouse commented Oct 18, 2024

@mati865 Yes that's 100% correct. I have removed those dependencies from compiler packages, leaving only libwinpthread on gcc-libs.

@lhmouse lhmouse force-pushed the gcc-pkg branch 2 times, most recently from 3353630 to 8ea5886 Compare October 18, 2024 02:50
@lhmouse
Copy link
Contributor Author

lhmouse commented Oct 18, 2024

do that for lto-plugin, too.

@lhmouse lhmouse force-pushed the gcc-pkg branch 5 times, most recently from b478bab to a8fc69f Compare October 18, 2024 12:57
@lhmouse
Copy link
Contributor Author

lhmouse commented Oct 18, 2024

It should be fine now. All programs, as well as liblto_plugin.dll, now reference only system DLLs.

@lazka
Copy link
Member

lazka commented Oct 22, 2024

Wouldn't calling SetDllDirectory("") in gcc before invoking /lib/gcc stuff be the more correct fix?

And why is gcc special here, doesn't this affect everything?

@lhmouse
Copy link
Contributor Author

lhmouse commented Oct 22, 2024

Wouldn't calling SetDllDirectory("") in gcc before invoking /lib/gcc stuff be the more correct fix?

It only affects LoadLibrary() and the Ex one. It does not seem to have any effect on DLLs that are implicitly loaded.

And why is gcc special here, doesn't this affect everything?

In reality gcc.exe is not affected. https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#standard-search-order-for-unpackaged-apps:

  1. DLL Redirection.
  2. API sets.
  3. SxS manifest redirection.
  4. Loaded-module list.
  5. Known DLLs.
  6. Windows 11, version 21H2 (10.0; Build 22000), and later. The package dependency graph of the process. This is the application's package plus any dependencies specified as <PackageDependency> in the <Dependencies> section of the application's package manifest. Dependencies are searched in the order they appear in the manifest.
  7. The folder from which the application loaded.
  8. The system folder. Use the GetSystemDirectory function to retrieve the path of this folder.
  9. The 16-bit system folder. There's no function that obtains the path of this folder, but it is searched.
  10. The Windows folder. Use the GetWindowsDirectory function to get the path of this folder.
  11. The current folder.
  12. The directories that are listed in the PATH environment variable. This doesn't include the per-application path specified by the App Paths registry key. The App Paths key isn't used when computing the DLL search path.

gcc.exe and libgcc_s_seh-1.dll are in the same directory, so when this DLL is loaded by gcc.exe it has priority 7, which trumps always everything else - even system DLLs.

But if cc1.exe is spawned by gcc.exe, it can't find libgcc_s_seh-1.dll in 'the folder from which the application loaded', which is later found in PATH according to rule 12. Then the risk is if there's a DLL with the same name in 'the current folder', it will take precedence over 12.

@lazka
Copy link
Member

lazka commented Oct 22, 2024

It only affects LoadLibrary() and the Ex one. It does not seem to have any effect on DLLs that are implicitly loaded.

ah, unfortunate.. the docs are confusing..

And why is gcc special here, doesn't this affect everything?

In reality gcc.exe is not affected. https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#standard-search-order-for-unpackaged-apps:
...

ah, right, thanks


Doing some more searching, would https://learn.microsoft.com/en-us/cpp/build/reference/dependentloadflag?view=msvc-170 help in theory? i.e. /DEPENDENTLOADFLAG:0x2000 would exclude the current dir from the search path. seems to be a msvc only thing though (??)

@lhmouse
Copy link
Contributor Author

lhmouse commented Oct 22, 2024

Doing some more searching, would https://learn.microsoft.com/en-us/cpp/build/reference/dependentloadflag?view=msvc-170 help in theory? i.e. /DEPENDENTLOADFLAG:0x2000 would exclude the current dir from the search path. seems to be a msvc only thing though (??)

There does not seem to be support in GNU LD? objdump -x /ucrt/bin/gcc.exe prints a Load Configuration Directory of size zero, indicating that there is no such section.

@lazka
Copy link
Member

lazka commented Oct 22, 2024

yes, also not in mingw lld I think, though it's in lld llvm/llvm-project#71537

I was just wondering if (1) this would help in this case (2) and if yes we can ask upstreams to add support.

Not saying that we should wait on new upstream features, it would just be nice to know that we can remove static linking again one day.

@lhmouse
Copy link
Contributor Author

lhmouse commented Oct 22, 2024

yes, also not in mingw lld I think, though it's in lld llvm/llvm-project#71537

LLD supports /dependentloadflag:<value> when invoked as lld-link.

I was just wondering if (1) this would help in this case (2) and if yes we can ask upstreams to add support.

Definitely. It would be good to have.

Not saying that we should wait on new upstream features, it would just be nice to know that we can remove static linking again one day.

I am not familiar with binutils; after a quick glance I suspect relevant code is in 'bfd/peXXigen.c' around where the TLS directory is set.

@mati865
Copy link
Collaborator

mati865 commented Oct 22, 2024

Adding it to LLD should be trivial, just like llvm/llvm-project@276283d

Currently it should work if you pass -Xlink=-dependentloadflag=<value> or something like that but having it natively supported is the way to go.

mati865 added a commit to mati865/llvm-project that referenced this pull request Oct 27, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.
I'm not sure about the name as ld.bfd doesn't support it (yet?).

There is no solid need for it yet, but it's being considered: msys2/MINGW-packages#22216 (comment)
mati865 added a commit to mati865/llvm-project that referenced this pull request Oct 27, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.
I'm not sure about the name as ld.bfd doesn't support it (yet?).

There is no solid need for it yet, but it's being considered: msys2/MINGW-packages#22216 (comment)
The language-specific compilers (cc1, cc1plus, lto-wrapper, etc.) are not in
PATH, but in '<prefix>/lib/gcc/<triplet>/<version>'. When these compilers are
invoked by GCC, they prefer DLLs in the working directory to those in PATH [1],
which allows, for example, an untrusted source repo to create libgmp-10.dll in
the working directory, which will get picked by cc1plus, resulting in arbitrary
code execution.

These programs shall be linked against all dependencies statically. After this
change they depend on only GCC runtime libraries.

[1] https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security

Signed-off-by: LIU Hao <[email protected]>
mati865 added a commit to mati865/llvm-project that referenced this pull request Nov 19, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.
I'm not sure about the name as ld.bfd doesn't support it (yet?).

There is no solid need for it yet, but it's being considered: msys2/MINGW-packages#22216 (comment)
mati865 added a commit to mati865/llvm-project that referenced this pull request Nov 20, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.
I'm not sure about the name as ld.bfd doesn't support it (yet?).

There is no solid need for it yet, but it's being considered: msys2/MINGW-packages#22216 (comment)
mstorsjo pushed a commit to mati865/llvm-project that referenced this pull request Dec 6, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.
I'm not sure about the name as ld.bfd doesn't support it (yet?).

There is no solid need for it yet, but it's being considered: msys2/MINGW-packages#22216 (comment)
mstorsjo pushed a commit to llvm/llvm-project that referenced this pull request Dec 6, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.

ld.bfd doesn't support it, yet at least, but if they later add support for something similar, hopefully they’d agree to the same option name.

There is no solid need for it yet, but it's being considered:
msys2/MINGW-packages#22216 (comment)
broxigarchen pushed a commit to broxigarchen/llvm-project that referenced this pull request Dec 10, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.

ld.bfd doesn't support it, yet at least, but if they later add support for something similar, hopefully they’d agree to the same option name.

There is no solid need for it yet, but it's being considered:
msys2/MINGW-packages#22216 (comment)
chrsmcgrr pushed a commit to RooflineAI/llvm-project that referenced this pull request Dec 12, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.

ld.bfd doesn't support it, yet at least, but if they later add support for something similar, hopefully they’d agree to the same option name.

There is no solid need for it yet, but it's being considered:
msys2/MINGW-packages#22216 (comment)
TIFitis pushed a commit to TIFitis/llvm-project that referenced this pull request Dec 18, 2024
Implement MSVC's `/DEPENDENTLOADFLAG` as `--dependent-load-flag` and
forward it to COFF.

ld.bfd doesn't support it, yet at least, but if they later add support for something similar, hopefully they’d agree to the same option name.

There is no solid need for it yet, but it's being considered:
msys2/MINGW-packages#22216 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants