msys2 / MINGW-packages

Package scripts for MinGW-w64 targets to build under MSYS2.
https://packages.msys2.org
BSD 3-Clause "New" or "Revised" License
2.25k stars 1.21k forks source link

gcc: DEP and ASLR not enabled by default #6674

Closed salowenh closed 3 years ago

salowenh commented 4 years ago

The libgsf is flagged by tools such as BinSkim due to apparently not enabled safe exception handlers, DEP and ASLR security options. Any thoughts on how to create a more secure compilation?

Biswa96 commented 4 years ago

mingw-w64 gcc environment has many reputation of being flagged by anti virus programs. Search "mingw-w64 virus alert" and you will get many related issues. The alerts are mainly false positive which may not be related to disabled DEP and ASLR.

salowenh commented 4 years ago

@Biswa96 Unfortunately in this case I believe this is not a false positive as I can't see these flags in any of the binaries. Do you have any reason to think otherwise?

lazka commented 4 years ago

Last time I looked gcc didn't support DEP and ASLR by default, maybe that has changed in the meantime. I'd certainly welcome it.

salowenh commented 4 years ago

@lazka support was added in gcc 10.1.0, it's indeed disabled by default (i'm not sure why)

mati865 commented 4 years ago

@salowenh because various LD versions had problem with it.

revelator commented 4 years ago

Also gobject-introspection has problems with it, try enabling it and then watch what happens heh.

revelator commented 4 years ago

btw you can also pretty much forget using PCH if you enable ASLR and DEP they are incompatible as is. Im not sure how microsoft got around this problem.

jeremyd2019 commented 4 years ago

https://sourceware.org/bugzilla/show_bug.cgi?id=19011

I have no idea about GCC, but they're just flags on the PE file, controlled by the linker (ld). I actually looked at this a bit. You can turn the options on by default in a gcc spec file, but there is no corresponding negative option to turn them back off on a case-by-case basis. peflags.exe in the rebase package comes in handy in that case :wink:

mati865 commented 4 years ago

@jeremyd2019 we can add flags in https://github.com/msys2/MSYS2-packages/blob/master/pacman/makepkg_mingw64.conf Then all repo packages will have it enabled.

lazka commented 4 years ago

I've uncommented the LDFLAGS stuff and built ninja as a test:

Before:

Get-PESecurity -file C:\msys64\home\user\M\mingw-w64-ninja\pkg\mingw-w64-x86_64-ninja\mingw64\bin\ninja.exe

FileName         : C:\msys64\home\user\M\mingw-w64-ninja\pkg\mingw-w64-x86_64-ninja\mingw64\bin\ninja.exe
ARCH             : AMD64
DotNET           : False
ASLR             : False
DEP              : False
Authenticode     : False
StrongNaming     : N/A
SafeSEH          : N/A
ControlFlowGuard : False
HighentropyVA    : False

After:

Get-PESecurity -file C:\msys64\home\user\M\mingw-w64-ninja\pkg\mingw-w64-x86_64-ninja\mingw64\bin\ninja.exe

FileName         : C:\msys64\home\user\M\mingw-w64-ninja\pkg\mingw-w64-x86_64-ninja\mingw64\bin\ninja.exe
ARCH             : AMD64
DotNET           : False
ASLR             : True
DEP              : True
Authenticode     : False
StrongNaming     : N/A
SafeSEH          : N/A
ControlFlowGuard : False
HighentropyVA    : True

Looks like the last remaining upstream changes (https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0) are in binutils 2.34, which we have.

Should we try enabling this by default? What about 32 bit?

jeremyd2019 commented 4 years ago

Everything but high entropy va should work on 32-bit as well. MS tools actually let you set highentropyva on 32-bit modules, but ld doesn't, and I don't think it means anything there.

lazka commented 4 years ago

gtk3 works, python (which uses pgo/lto) segfaults at link time.

mati865 commented 4 years ago

We should upgrade to binutils 2.35 first.

jeremyd2019 commented 4 years ago

btw you can also pretty much forget using PCH if you enable ASLR and DEP they are incompatible as is. Im not sure how microsoft got around this problem.

I wonder if @revelator meant PGO rather than PCH in that comment. I was scratching my head how these would break precompiled headers, but now that you saw they broke profile-guided optimization it makes perfect sense why that could be a problem.

jeremyd2019 commented 4 years ago

Another option to consider adding to those that were commented out in makepkg_mingw64.conf would be to set the image base above 2^32. This appears to be the default for -cygwin (or -msys in this patch). Unfortunately, the default base for exes and dlls is supposed to be different, and it isn't known when setting LDFLAGS which is being built.

mati865 commented 4 years ago

@jeremyd2019 appropriate bases for executables and libraries have to be set in those lines: https://github.com/bminor/binutils-gdb/blob/d262797294039fc828e36f76c45e462966c5c72a/ld/emultempl/pep.em#L102

lazka commented 4 years ago

I wonder if @revelator meant PGO rather than PCH in that comment.

The last time this was tried also mentions PCH: https://github.com/msys2/MINGW-packages/commit/2602139640f15fb6c58c11c83903f2f3f48b7f13#diff-c8386c579acf251c791f3c68b4d22e65

jeremyd2019 commented 4 years ago

I was wondering why nobody had a patch for that binutils bug in all this time. 😕 Well, I finished mine and added to the bug in any event, maybe save the next guy some confusion.

salowenh commented 4 years ago

I was wondering why nobody had a patch for that binutils bug in all this time. 😕 Well, I finished mine and added to the bug in any event, maybe save the next guy some confusion.

Are you relating to the base address bug? can you please share how to apply it when compiling with mingw?

jeremyd2019 commented 4 years ago

The base address was just one part of this. https://sourceware.org/bugzilla/show_bug.cgi?id=19011 has my two patches, one to provide options to turn off dll characteristics flags, and a second one to change the defaults. This second one does change the default on x86_64 to use base addresses greater than 2^32, using the same variation that was already in place for cygwin/msys. They could be applied in the PKGBUILD for binutils if desired.

It sounds though like this might open a can of worms.

jeremyd2019 commented 4 years ago

The last time this was tried also mentions PCH

Ah, GCC puts pointers in their PCH output, so can't use it if gcc's base address changed. Sounds like this is a problem for everyone... https://bugzilla.redhat.com/show_bug.cgi?id=1721553#c34

mati865 commented 4 years ago

If it still fails with 2.35 then there must be some other Binutils bug. LLD always uses ASLR when targeting MSVC (and soon will enable it by default for MinGW: https://reviews.llvm.org/D86654) so it should work there without issues so it could be used for comparing broken binaries made with BFD.

lazka commented 4 years ago

for the python segfault, only --high-entropy-va makes it crash even without pgo/lto, with 2.35

lazka commented 4 years ago

So, as far as I understand, one way forward would be:

edit: ~also add --disable-auto-image-base~ edit2: patch NT_EXE_IMAGE_BASE and NT_DLL_IMAGE_BASE to match msvc when 64 bit

mati865 commented 4 years ago

maybe patch binutils move_default_addr_high

This sets image base to special ones used by Cygwin. If we are going to change it then we should patch BFD to use the same address as MSVC and LLD. Then try if Python still crashes.

lazka commented 4 years ago

(I don't know how any of this works, so I don't understand the details.. as for python, to clarify, the linking process crashes, not python)

edit: ah, I see, that options is for cygwin, which uses other default addresses.

jeremyd2019 commented 4 years ago

I didn't see a need to --disable-auto-image-base - I don't think it would hurt anything if ASLR is enabled, and could be helpful if something needs to disable ASLR (though I guess they could re-enable it if they needed to)

jeremyd2019 commented 4 years ago

maybe patch binutils move_default_addr_high

This sets image base to special ones used by Cygwin. If we are going to change it then we should patch BFD to use the same address as MSVC and LLD.

I didn't notice this, thanks. I guess I should update my patch to do this too. 0x1 4000 0000 for EXEs and 0x1 8000 0000 for DLLs? That kind of makes move_default_addr_high a misnomer 😁

lazka commented 4 years ago

I didn't see a need to --disable-auto-image-base - I don't think it would hurt anything if ASLR is enabled, and could be helpful if something needs to disable ASLR (though I guess they could re-enable it if they needed to)

looking at the code it seems it's disabled by default anyway. Not sure why it was suggested in the bug report you linked, hm.

mati865 commented 4 years ago

the linking process crashes, not python

Oh that's interesting, hopefully you've got the backtrace. Linker crash should be easier to deal with than miscompiled binary.

looking at the code it seems it's disabled by default anyway.

AFAIK it's disabled in Binutils 2.34+ if ASLR is enabled.

0x1 4000 0000 for EXEs and 0x1 8000 0000 for DLLs?

Yes.

jeremyd2019 commented 4 years ago

It's (--enable-auto-image-base) enabled by the GCC specs. https://github.com/gcc-mirror/gcc/blob/85847fd421d7760f45f0e69c7ae3607f2f898bb8/gcc/config/i386/mingw-w64.h#L98

revelator commented 4 years ago

Aye i ment PCH but it is interresting that it also affects PGO.

Looking on with baithed breath if this can be fixed it would be great.

mati865 commented 4 years ago

@jeremyd2019 then we should patch GCC as well.

jeremyd2019 commented 4 years ago

Can you explain why that's necessary? ISTM that the image base address doesn't matter when ASLR is enabled (except for the special case where it's > 2^32 on x64), so it shouldn't matter whether it's on or off. However, if ASLR is disabled, it would still be as beneficial as ever to not have conflicting addresses requiring the legacy relocation behavior.

mati865 commented 4 years ago

@jeremyd2019 disabling is not necessary but auto image base was confusing people who were inspecting binaries. Disabled ASLR is valid argument so we can leave it as is for now.

jeremyd2019 commented 4 years ago

It looks like my patches were accepted by binutils. https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=514b4e191d5f46de8e142fe216e677a35fa9c4bb

jeremyd2019 commented 4 years ago

The libgsf is flagged by tools such as BinSkim due to apparently not enabled safe exception handlers, DEP and ASLR security options.

SafeSEH has got me going around in circles. Does anybody here know anything about SEH in MinGW-w64? I'm not comfortable setting --no-seh by default, but I have read that GCC doesn't support SEH anyway. If true, it may be a good candidate to set in the default spec in GCC.

mati865 commented 4 years ago

SafeSEH exists only for 32 bit x86 for which we are using Dwarf-2 unwinding. In MinGW mode LLD already sets IMAGE_DLL_CHARACTERISTICS_NO_SEH for ARMv7 and i686.

jeremyd2019 commented 4 years ago

SafeSEH exists only for 32 bit x86 for which we are using Dwarf-2 unwinding. In MinGW mode LLD already sets IMAGE_DLL_CHARACTERISTICS_NO_SEH for ARMv7 and i686.

That sounds like a recommendation to add --no-seh to the makepkg_mingw32 hardened LDFLAGS at least.

revelator commented 4 years ago

hmm still seing some packages fail to build with aslr hardening on for one gobject-introspection. The --no-seh flag also applies to sjlj i suspect ?.

jeremyd2019 commented 4 years ago

I believe the plan is to identify failing packages, and disable the responsible flag(s) on them on a case-by-case basis, so please share them. I am not very confident in my understanding of the --no-seh stuff, but I am pretty sure that neither dwarf2 nor sjlj require SEH.

revelator commented 4 years ago

Ok then i can report capnproto and openshadinglanguage also fails when this is enabled (segfault). Atm im trying to rebuild gcc with aslr and dep on to see if it breaks.

Aye i was under the same impression with sjlj, i use this exception model locally though not as fast as dwarf i found it to be more stable. A 32 bit SEH exception model for gcc would be nice but noone seems to be working on it atm.

jeremyd2019 commented 4 years ago

I did try gcc last night, and found that the final binaries produced did not have the new flags set. It appears gcc does not apply the LDFLAGS to later stages (it looked like it was using it at the beginning of the build, so it may have been only on stage1)

revelator commented 4 years ago

Ok odd mine segfaults halfway into building libiberty using LDFLAGS="-pipe -Wl,--dynamicbase,--high-entropy-va,--nxcompat,--no-seh"

jeremyd2019 commented 4 years ago

Couple of things: 1) I only tried i686, not x86_64, and 2) I do not think that --no-seh should be set on x86_64.

revelator commented 4 years ago

Yeah if i remember correctly you need to use the stage CFLAGS LDFLAGS to get the compiler to pick it up in last stage else it defaults to it's internal flags.

revelator commented 4 years ago

Probably not if i used the official builds from here :) but im using a deriative using sjlj exceptions for both 32 and 64 bit.

revelator commented 4 years ago

Hmm this is weird i tried using the old method with LDFLAGS="-pipe -Wl,--enable-auto-image-base,--no-seh" and this works fine so far ?!?. Ill let it run to completion to see if there will be trouble down the line.

revelator commented 4 years ago

Old method worked fine with --no-seh for both 64 and 32 bit. I decided to rebuild my compiler with alex patches 0300-default-secure-pe-flags.patch and 0310-reproducible-import-libraries.patch merged with the 0320-aslr-compat-base-addr.patch from latest pull and it works so far though im not completely sure about the fallthrough added in OPTION_DYNAMIC_BASE and OPTION_HIGH_ENTROPY_VA should also apply to the flags to set them to off. This has the added benefit that binutils will force aslr and dep by default even in bootstrapped gcc but it becomes interresting if it will also still break pch.

revelator commented 4 years ago

internal compiler error in avx2intrin.h so nope gcc is not happy, also dosnt matter which gcc compiler is used as i tried both the official and my own and they both throw this error while building gcc-9.3.0. Also it seems we need an extra flag -> --enable-reloc-section in LDFLAGS as well as the other flags. Atm im trying a non bootstrap gcc build to see if it triggers it again with the above flag added.