There are a number of issues that ld has on mingw-w64 (or otherwise targeting mingw-w64). Most of them stem from the fact that ld's defaults for mingw-w64 targets are terrible and unfortunately most or all of these settings have security implications. I would like this thread to be a consolidation of any previous threads that may have reported a subset of these issues. Put simply, for mingw-w64 targets (32 and 64-bit) binutil's ld should do the following: - Default to enabling dynamicbase and nxcompat. - Never strip the reloc section. - Default to enabling HEASLR (high-entropy-va) - Use a base address > 4GB for 64-bit binaries. - Default to disable-auto-image-base All of the above will bring binaries created with gcc/ld in line with what's produced by msvc (visual studio/Microsoft's compiler/linker). Any compatibility issues caused by these changes should be acceptably small and msvc has been defaulting to all of these since they were brought about in VS2008 (yes 2008 for dynamicbase and nxcompat). Going through the list in a little more detail, dynamicbase and nxcompat is universally safe for mingw-w64 targets (and only mingw-w64 targets on windows) and should cause no compatibility issues at all. Furthermore if you specify dynamicbase it should automatically cause the linker to output a reloc section (or not strip it or however it works internally). Otherwise you get weird stuff like this[1] where another option is being invented for the wrong reasons. There's no reason for this option because ld should never be stripping the reloc section from executables in the first place (or perhaps better logic should be if you specify dynamicbase it should just have the reloc section, no reason to split this into another option). Defaulting to HEASLR is also a no brainer. It's ignored on targets which don't support it. Specifying a base address > 4GB is a compatibility single to the loader that there are no "latent pointer truncation issues" and it can use extra entropy (8 -> 17 bits) for the base address randomization[2]. Executables should have a base address of 0x140000000 and dll's should use 0x180000000. All of the above ties into my last point of switching the default to --disable-auto-image-base. Thanks to ASLR there's no reason to be generating random image base's anymore for mingw-w64 binaries. The above should bring binaries produced with ld in line with what Microsoft's linker uses and is way more sane than what's currently used so we don't have to use a million workarounds in order to get something sane[3][4][5]. [3] is especially hilarious. [1] https://sourceware.org/bugzilla/show_bug.cgi?id=17321 [2] http://blogs.technet.com/b/srd/archive/2013/12/11/software-defense-mitigating-common-exploitation-techniques.aspx [3] https://github.com/TheRyuu/FFmpeg/commit/91b668acd6decec0a6f8d20bf56e2644f96adcb9 [4] https://github.com/TheRyuu/FFmpeg/commit/f2b805d02ee12af5593130ef06f0924422e8622e [5] https://github.com/TheRyuu/FFmpeg/commit/ae4e2e6cc32541ce19a716136995d605a723ac5e
Hi Alex, Is it possible for you to produce a consolidated patch that addresses all of these issues ? I am all in favour of these changes unless someone has strenuous objections - and I seriously doubt that anyone will. But it will make my life easier if I only have one patch to test instead of 5. Cheers Nick
I don't mind having a go but I'm unfamiliar with the binutils/ld source so I'll have to figure out where everything is first. On that note are you (or anyone who might be able to help) on irc?
Sorry - I am not on IRC, but do feel free to email me direct. Cheers Nick
Created attachment 11152 [details] Don't strip reloc sections when building with dynamicbase This should address the "- Never strip the reloc section." part.
The attached patch fixes the IMAGE_FILE_RELOCS_STRIPPED bit to be set in the headers when no symbol is exported, when building with -Wl,--dynamicbase. Please let me know if some corrections are to be made! AFAIU I need to explicitly state that I'm ok with the copyright being assigned to the FSF, so I'm ok with it. Regards,
Well, apparently the attached fixes the header, but makes windows fail to run the resulting executable, so I guess something's missing. Any help would be appreciated!
This is a big drive-by, as I don't have much understanding on the details of the problem; but Tor uses the following patch to add a relocation section so Windows builds of Tor Browser can have ASLR: https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/binutils/enable-reloc-section-ld.patch
Googling for "mingw-w64 aslr" turned up a CERT vulnerability note[1] for this issue along with CVE-2018-5392. It wasn't apparent from the VN or the CVE whether or not Sourceware had been notified of the CVE assignment. [1] https://www.kb.cert.org/vuls/id/307144
(In reply to Alex Smith from comment #0) > Furthermore if you specify dynamicbase it should automatically cause the > linker to output a reloc section (or not strip it or however it works > internally). This is now done since https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0. --enable-reloc-section was implemented, but it is also automatically enabled with -dynamicbase.
Created attachment 12800 [details] [PATCH 1/2] Add options to disable dll characteristics flags.
Created attachment 12801 [details] [PATCH 2/2] LD/PR19011: more secure default PE options.
Created attachment 12802 [details] [PATCH 2/2] LD/PR19011: more secure default PE options. It was pointed out that the default base addresses for cygwin are not the same as the default base addresses for normal Windows binaries, so I updated the non-cygwin values to the defaults from MSVC, and came up with 0x1C0000000 for the auto image base range.
Created attachment 12804 [details] [PATCH 2/2] LD/PR19011: more secure default PE options. Also needed to turn pe_dll_enable_reloc_section on by default to correspond with dynamicbase being on by default.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=514b4e191d5f46de8e142fe216e677a35fa9c4bb commit 514b4e191d5f46de8e142fe216e677a35fa9c4bb Author: Jeremy Drake <sourceware-bugzilla@jdrake.com> Date: Thu Aug 27 12:58:27 2020 +0100 Change the default characteristics of DLLs built by the linker to more secure settings. PR 19011 * emultempl/pe.em (DEFAULT_DLL_CHARACTERISTICS): Define. (pe_dll_characteristics): Initialise to DEFAULT_DLL_CHARACTERISTICS. (add_options): Add options to disable DLL characteristics. (list_options): List the new options. (handle_options): Handle the new options. * emultempl/pep.em: Similar changes to above. (NT_EXE_IMAGE_BASE): Default to an address above 4G. (NT_DLL_IMAGE_BASE, NT_DLL_AUTO_IMAGE_BASE, (NT_DLL_AUTO_IMAGE_MASK): Likewise. * ld.texi: Document the new options. * pe-dll.c (pe_dll_enable_reloc_section): Change to default to true. (generate_reloc): Do nothing if there is no reloc section. (pe_exe_fill_sections): Only assign the reloc section contents if the section exists. * testsuite/ld-pe/pe.exp: Add the --disable-reloc-section flag to the .secrel32 tests. * testsuite/ld-scripts/provide-8.d: Expect for fail on PE targets. * NEWS: Mention the change in DLL generation.
Thanks Jeremy - I have now applied your two patches. I made two small additions - documenting the new options in ld/ld.texi and mentioning the change in linker behaviour in the ld/NEWS file. I also had to fix a couple of problems with the generation of reloc information when none was expected, but overall the patches proved to be easy to apply and test. Are these changes sufficient, or is there more that needs to be done ? Cheers Nick
This patch apparently caused x86_64-w64-mingw32 +FAIL: plugin claimfile lost symbol x86_64-w64-mingw32 +FAIL: plugin claimfile replace file x86_64-w64-mingw32 +FAIL: plugin error x86_64-w64-mingw32 +FAIL: plugin warning x86_64-w64-mingw32 +FAIL: plugin ignore lib x86_64-w64-mingw32 +FAIL: plugin claimfile replace lib x86_64-w64-mingw32 +FAIL: plugin with empty archive x86_64-w64-mingw32 +FAIL: PR ld/20070
It also fixed these by removing the "zero vma section reloc detected" error, where no --just-symbols was involved so I'm not sure this is a good thing. sh-pe +FAIL: objcopy executable (pr25662) sh-pe -FAIL: PE-COFF Long section names in objects (default) sh-pe -FAIL: PE-COFF Long section names in objects (disabled) sh-pe -FAIL: PE-COFF Long section names in objects (enabled) sh-pe -FAIL: align1 sh-pe -FAIL: ld-scripts/align2a sh-pe -FAIL: ld-scripts/align2b sh-pe -FAIL: ld-scripts/align5 sh-pe -FAIL: ALIGNOF sh-pe -FAIL: ASSERT sh-pe +XPASS: ld-scripts/fill16 sh-pe -FAIL: ld-scripts/defined2 sh-pe -FAIL: ld-scripts/defined3 sh-pe -FAIL: ld-scripts/defined4 sh-pe -FAIL: ld-scripts/defined5 sh-pe -FAIL: ld-scripts/pr24008 sh-pe -FAIL: ld-scripts/empty-address-1 sh-pe -FAIL: ld-scripts/empty-address-2a sh-pe -FAIL: ld-scripts/empty-address-2b sh-pe -FAIL: ld-scripts/empty-address-3a sh-pe -FAIL: ld-scripts/empty-address-3b sh-pe -FAIL: ld-scripts/empty-address-3c sh-pe -FAIL: ld-scripts/pr22267 sh-pe -FAIL: EXTERN sh-pe -FAIL: include-1 sh-pe -FAIL: binary logarithm sh-pe -FAIL: ld-scripts/pr20302 sh-pe +XPASS: SEGMENT_START expression not absolute (default) sh-pe +XPASS: SEGMENT_START expression not absolute (overridden) sh-pe -FAIL: SIZEOF
(In reply to Alan Modra from comment #17) > It also fixed these by removing the "zero vma section reloc detected" error, > where no --just-symbols was involved so I'm not sure this is a good thing. Ah - I did wonder if I was being too eager in suppressing that error. I will look into it, but I have to wonder, does anyone actually use the sh-pe target anymore ? Cheers Nick
Perhaps the default enabling of --enable-reloc-section should have been more limited? That was a last-minute change on my part, when I noticed the executables I built were not actually having ASLR applied to them due to lack of relocations. I did not consider non-x86 targets there.
From ld.log before/after the patch it looks like all of the regressions on x86_64-w64-mingw32 are due to additional "relocation truncated to fit" errors. good: ./ld-new -o tmpdir/pr20070.x -L/home/alan/src/binutils-gdb/ld/testsuite/ld-plugin -Bstatic -plugin /home/alan/build/gas/x86_64-w64-mingw32/ld/.libs/libldtestplug4.so.0 -plugin-opt registerclaimfile -plugin-opt registerallsymbolsread -plugin-opt registercleanup -plugin-opt claim:/home/alan/src/binutils-gdb/ld/testsuite/ld-plugin/pr20070b.c -plugin-opt claim:tmpdir/libpr20070.a -plugin-opt dumpresolutions tmpdir/pr20070a.o tmpdir/text.o tmpdir/libpr20070.a --defsym __stack_chk_fail=0 --defsym __main=0 --defsym ___main=0 --defsym printf=main --defsym puts=main hook called: all symbols read. Input: pr20070b.c (tmpdir/libpr20070.a) Sym: 'def' Resolution: LDPR_PREVAILING_DEF_IRONLY Sym: 'weakdef' Resolution: LDPR_PREVAILING_DEF_IRONLY Sym: 'undef' Resolution: LDPR_UNDEF Sym: 'weakundef' Resolution: LDPR_UNDEF Sym: 'common' Resolution: LDPR_PREVAILING_DEF_IRONLY hook called: cleanup. bad: ./ld-new -o tmpdir/pr20070.x -L/home/alan/src/binutils-gdb/ld/testsuite/ld-plugin -Bstatic -plugin /home/alan/build/gas/x86_64-w64-mingw32/ld/.libs/libldtestplug4.so.0 -plugin-opt registerclaimfile -plugin-opt registerallsymbolsread -plugin-opt registercleanup -plugin-opt claim:/home/alan/src/binutils-gdb/ld/testsuite/ld-plugin/pr20070b.c -plugin-opt claim:tmpdir/libpr20070.a -plugin-opt dumpresolutions tmpdir/pr20070a.o tmpdir/text.o tmpdir/libpr20070.a --defsym __stack_chk_fail=0 --defsym __main=0 --defsym ___main=0 --defsym printf=main --defsym puts=main hook called: all symbols read. Input: pr20070b.c (tmpdir/libpr20070.a) Sym: 'def' Resolution: LDPR_PREVAILING_DEF_IRONLY Sym: 'weakdef' Resolution: LDPR_PREVAILING_DEF_IRONLY Sym: 'undef' Resolution: LDPR_UNDEF Sym: 'weakundef' Resolution: LDPR_UNDEF Sym: 'common' Resolution: LDPR_PREVAILING_DEF_IRONLY tmpdir/pr20070a.o: in function `main': /home/alan/src/binutils-gdb/ld/testsuite/ld-plugin/pr20070a.c:6:(.text.startup+0x5): relocation truncated to fit: R_X86_64_PC32 against symbol `__main' defined in *ABS* section in tmpdir/pr20070.x hook called: cleanup. Fixing these regressions can probably be done by passing --disable-reloc-section on pe targets for these tests.
It turned out that the regressions were caused by the change to default image base. I'll commit a fix after running a few tests.
The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=16f9c644c7fcec6f4aa6f4e8a1458b57d2b28982 commit 16f9c644c7fcec6f4aa6f4e8a1458b57d2b28982 Author: Alan Modra <amodra@gmail.com> Date: Fri Aug 28 10:51:28 2020 +0930 mingw plugin test regressions due to commit 514b4e191d5f Fixes new failures due to image base change. PR 19011 * testsuite/ld-plugin/plugin.exp: Use modified CFLAGS throughout file. Add --image-base for pecoff.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6194b866b7a89969d8c66f8a97d40acc028373b1 commit 6194b866b7a89969d8c66f8a97d40acc028373b1 Author: Nick Clifton <nickc@redhat.com> Date: Fri Aug 28 09:43:13 2020 +0100 Fixes for testsuite failures introduced by the changes made for PR 19011. PR19011 bfd * cofflink.c (_bfd_coff_generic_relocate_section): Provide a value for undefined symbols which will not generate extra warning messages about truncated relocs. ld * testsuite/lib/ld-lib.exp (ld_link_defsyms): For PE based targets define the __main and ___main symbols in terms of the main symbol.
(In reply to cvs-commit@gcc.gnu.org from comment #14) > The master branch has been updated by Nick Clifton <nickc@sourceware.org>: > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git; > h=514b4e191d5f46de8e142fe216e677a35fa9c4bb > I received an email about this commit, from someone who didn't want to comment on bugzilla but said that I could copy-paste the comment, so here it is: > +@itemx --disable-tsaware > +The image is Terminal Server aware. This option is disabled by > +default. > > The /TSAWARE option is enabled by default for Windows and console > applications. > https://docs.microsoft.com/en-us/cpp/build/reference/tsaware-create-terminal-server-aware-application?view=vs-2019 > > > > +@itemx --disable-no-seh > The image does not use SEH. No SE handler may be called from > -this image. > +this image. This option is disabled by default. > > This option is mandatory for Windows certification/security. > https://code.videolan.org/videolan/vlc-winrt/-/issues/303 >
(In reply to sourceware-bugzilla from comment #24) I'm not sure if these are recommending documentation updates or changes to defaults, but if the latter... > > +@itemx --disable-tsaware > > +The image is Terminal Server aware. This option is disabled by > > +default. > > > > The /TSAWARE option is enabled by default for Windows and console > > applications. > > https://docs.microsoft.com/en-us/cpp/build/reference/tsaware-create-terminal-server-aware-application?view=vs-2019 This option only makes sense on executables, not DLLs, so would be a little more tricky to turn on by default (though it shouldn't hurt DLLs to have it set). > > +@itemx --disable-no-seh > > The image does not use SEH. No SE handler may be called from > > -this image. > > +this image. This option is disabled by default. > > > > This option is mandatory for Windows certification/security. > > https://code.videolan.org/videolan/vlc-winrt/-/issues/303 > > Note that WACK is looking for SafeSEH, not necessarily NO SEH. I do not know what would be required for SafeSEH, but I'm sure it would be a lot more involved than just flipping a few bits in an image header.
> #define NT_DLL_AUTO_IMAGE_MASK \ > ((bfd_vma) (${move_default_addr_high} ? 0x1ffff0000LL \ >- : 0x0ffc0000LL)) >+ : 0x1ffff0000LL)) I just noticed that I had copied the mask from the 'cygwin' move_default_addr_high case, but had done the math in my head as though it were 0x1fff0000. I don't know that there's a problem with it being 0x1ffff0000 but thought I should point out that it wasn't what I thought it was