Bug 19011 - Issues with ld on mingw-w64 and bad defaults
Summary: Issues with ld on mingw-w64 and bad defaults
Status: ASSIGNED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.25
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on: 26587 26588 26659
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-27 16:49 UTC by Alex Smith
Modified: 2020-09-23 20:30 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Don't strip reloc sections when building with dynamicbase (1.17 KB, patch)
2018-07-25 09:39 UTC, hugo@beauzee.fr
Details | Diff
[PATCH 1/2] Add options to disable dll characteristics flags. (2.68 KB, patch)
2020-08-26 21:01 UTC, sourceware-bugzilla
Details | Diff
[PATCH 2/2] LD/PR19011: more secure default PE options. (742 bytes, patch)
2020-08-26 21:03 UTC, sourceware-bugzilla
Details | Diff
[PATCH 2/2] LD/PR19011: more secure default PE options. (835 bytes, patch)
2020-08-26 23:01 UTC, sourceware-bugzilla
Details | Diff
[PATCH 2/2] LD/PR19011: more secure default PE options. (1.00 KB, patch)
2020-08-27 04:30 UTC, sourceware-bugzilla
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Smith 2015-09-27 16:49:13 UTC
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
Comment 1 Nick Clifton 2015-09-29 10:59:48 UTC
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
Comment 2 Alex Smith 2015-09-29 17:27:49 UTC
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?
Comment 3 Nick Clifton 2015-09-30 12:40:48 UTC
Sorry - I am not on IRC, but do feel free to email me direct.

Cheers
  Nick
Comment 4 hugo@beauzee.fr 2018-07-25 09:39:19 UTC
Created attachment 11152 [details]
Don't strip reloc sections when building with dynamicbase

This should address the "- Never strip the reloc section." part.
Comment 5 hugo@beauzee.fr 2018-07-25 09:42:31 UTC
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,
Comment 6 hugo@beauzee.fr 2018-07-25 14:59:53 UTC
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!
Comment 7 Tom Ritter 2018-10-11 15:20:32 UTC
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
Comment 8 gerald@wireshark.org 2018-10-11 17:16:16 UTC
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
Comment 9 Hannes Domani 2020-01-11 13:40:09 UTC
(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.
Comment 10 sourceware-bugzilla 2020-08-26 21:01:58 UTC
Created attachment 12800 [details]
[PATCH 1/2] Add options to disable dll characteristics flags.
Comment 11 sourceware-bugzilla 2020-08-26 21:03:50 UTC
Created attachment 12801 [details]
[PATCH 2/2] LD/PR19011: more secure default PE options.
Comment 12 sourceware-bugzilla 2020-08-26 23:01:11 UTC
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.
Comment 13 sourceware-bugzilla 2020-08-27 04:30:13 UTC
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.
Comment 14 cvs-commit@gcc.gnu.org 2020-08-27 11:59:13 UTC
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.
Comment 15 Nick Clifton 2020-08-27 12:01:50 UTC
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
Comment 16 Alan Modra 2020-08-27 15:04:37 UTC
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
Comment 17 Alan Modra 2020-08-27 15:08:10 UTC
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
Comment 18 Nick Clifton 2020-08-27 15:36:38 UTC
(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
Comment 19 sourceware-bugzilla 2020-08-27 17:22:37 UTC
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.
Comment 20 Alan Modra 2020-08-27 23:57:26 UTC
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.
Comment 21 Alan Modra 2020-08-28 02:13:54 UTC
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.
Comment 22 cvs-commit@gcc.gnu.org 2020-08-28 03:44:36 UTC
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.
Comment 23 cvs-commit@gcc.gnu.org 2020-08-28 08:44:06 UTC
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.
Comment 24 sourceware-bugzilla 2020-08-28 18:20:32 UTC
(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
>
Comment 25 sourceware-bugzilla 2020-08-28 18:27:02 UTC
(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.
Comment 26 sourceware-bugzilla 2020-08-28 23:14:23 UTC
>  #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