Enabling hardening options for Wesnoth's executables

Discussion among members of the development team.

Moderator: Forum Moderators

Post Reply
User avatar
Pentarctagon
Project Manager
Posts: 5496
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Enabling hardening options for Wesnoth's executables

Post by Pentarctagon »

It was mentioned on Discord that it would be beneficial to enable by default the hardening/security features added in PR #3015. To expand a bit on what it does:
  • Makes Wesnoth a Position Independent Executable(PIE, info). This allows it to make better use of ASLR, and is done vs the -fPIE -pie options.
  • Protects against stack smashing/buffer overflow attacks. This is done by the -D_FORTIFY_SOURCE=2 define(info) and the -fstack-protector-strong option(info, more info).
  • Uses full RELRO(info) via the options -Wl,-z,now,-z,relro.
The main potential issues being that I don't believe Wesnoth's unit tests have ever been run with this options and so it is possible they fail and require some additional changes, as well as there being some minimal performance impact from -fstack-protector-strong as well as PIE on certain architectures.
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
jyrkive
Inactive Developer
Posts: 36
Joined: July 7th, 2016, 6:20 pm
Location: Helsinki, Finland

Re: Enabling hardening options for Wesnoth's executables

Post by jyrkive »

Pentarctagon wrote: June 11th, 2018, 6:02 am The main potential issues being that I don't believe Wesnoth's unit tests have ever been run with this options and so it is possible they fail and require some additional changes, as well as there being some minimal performance impact from -fstack-protector-strong as well as PIE on certain architectures.
Absolutely nothing in any halfway sane codebase should be relying on things which these security mitigations block.

I'm in favor of enabling the hardening options by default.
User avatar
Pentarctagon
Project Manager
Posts: 5496
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon »

Based on the Debian build log, Wesnoth is already built there with -D_FORTIFY_SOURCE=2, -fstack-protector-strong, and -Wl,-z,relro. The new options from above then would only be -Wl,-z,now and enabling PIE.
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
jyrkive
Inactive Developer
Posts: 36
Joined: July 7th, 2016, 6:20 pm
Location: Helsinki, Finland

Re: Enabling hardening options for Wesnoth's executables

Post by jyrkive »

Pentarctagon wrote: June 11th, 2018, 8:46 am Based on the Debian build log, Wesnoth is already built there with -D_FORTIFY_SOURCE=2, -fstack-protector-strong, and -Wl,-z,relro. The new options from above then would only be -Wl,-z,now and enabling PIE.
Of course, some players build Wesnoth themselves, and there are also other GNU/Linux distributions than Debian.
Shiki
Developer
Posts: 344
Joined: July 13th, 2015, 9:53 pm
Location: Germany

Re: Enabling hardening options for Wesnoth's executables

Post by Shiki »

Arch builds with -D_FORTIFY_SOURCE=2, -fstack-protector-strong -fno-plt and with the linker flags -Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now.

Edit: these are added by default to every arch package, our own flags will take higher priority.
Try out the dark board theme.
User avatar
Pentarctagon
Project Manager
Posts: 5496
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon »

jyrkive wrote: June 11th, 2018, 9:28 am
Pentarctagon wrote: June 11th, 2018, 8:46 am Based on the Debian build log, Wesnoth is already built there with -D_FORTIFY_SOURCE=2, -fstack-protector-strong, and -Wl,-z,relro. The new options from above then would only be -Wl,-z,now and enabling PIE.
Of course, some players build Wesnoth themselves, and there are also other GNU/Linux distributions than Debian.
Right, my point wasn't that it shouldn't be done, but rather that other places already use some of these flags, so the potential for any unexpected issues is smaller. Arch goes a bit further than Debian it looks like, using the full -Wl,-z,relro,-z,now, so it looks like the only truly new option would be enabling PIE. That's a bit unexpected, though I suppose it might be because PIE is expected to have a much larger impact on things that are 32-bit still, so it wasn't enabled everywhere by default? Ubuntu as of 17.10 should have it enabled everywhere, though.
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
User avatar
Pentarctagon
Project Manager
Posts: 5496
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon »

Another question, I suppose, would be whether to enable the Wformat-security option. Wesnoth currently doesn't output any warnings when it's enabled though, and I don't know how useful it would actually be.
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
jyrkive
Inactive Developer
Posts: 36
Joined: July 7th, 2016, 6:20 pm
Location: Helsinki, Finland

Re: Enabling hardening options for Wesnoth's executables

Post by jyrkive »

Pentarctagon wrote: June 11th, 2018, 10:02 pm Another question, I suppose, would be whether to enable the Wformat-security option. Wesnoth currently doesn't output any warnings when it's enabled though, and I don't know how useful it would actually be.
We already ensure with Travis CI that build passes even with full -Wall -Werror (with the exception of some warnings we have suppressed, but -Wformat isn't among them).
User avatar
Pentarctagon
Project Manager
Posts: 5496
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon »

Alright, in that case, I'll enable hardening by default tomorrow, unless there are any objections raised before then.
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
Shiki
Developer
Posts: 344
Joined: July 13th, 2015, 9:53 pm
Location: Germany

Re: Enabling hardening options for Wesnoth's executables

Post by Shiki »

How about adding -Wl,-O1,--sort-common,--as-needed to the Optimized builds?
Try out the dark board theme.
User avatar
Pentarctagon
Project Manager
Posts: 5496
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon »

Could you provide more information on what they do?
  • -Wl,-O1: It looks like this wouldn't apply to regular executables?
  • --sort-common: I'm not quite sure what to make of it.
  • --as-needed: Sounds like an equivalent functionality could be achieved by just not providing unused libraries on the command line?
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
Shiki
Developer
Posts: 344
Joined: July 13th, 2015, 9:53 pm
Location: Germany

Re: Enabling hardening options for Wesnoth's executables

Post by Shiki »

I'm not having deeper information besides the manpage either.
  • -Wl,-O1: I think that would apply for the hardened builds though.
  • --sort-common: It sounds like it would slightly reduce the size.
  • --as-needed: Sounds so...
Try out the dark board theme.
User avatar
Pentarctagon
Project Manager
Posts: 5496
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon »

The commands:

Code: Select all

scons wesnoth --option-cache="" --debug=time ctool=gcc cxxtool=g++ ccache=yes build=release nls=no jobs=12 extra_flags_release=""
3bb4a7e5f6154a9589b0102f6e85b872ea8b39ab51625676f44865a215b4e1b5
25962480 bytes

scons wesnoth --option-cache="" --debug=time ctool=gcc cxxtool=g++ ccache=yes build=release nls=no jobs=12 extra_flags_release="-Wl,--sort-common"
3bb4a7e5f6154a9589b0102f6e85b872ea8b39ab51625676f44865a215b4e1b5
25962480 bytes

scons wesnoth --option-cache="" --debug=time ctool=gcc cxxtool=g++ ccache=yes build=release nls=no jobs=12 extra_flags_release="-Wl,--as-needed"
3bb4a7e5f6154a9589b0102f6e85b872ea8b39ab51625676f44865a215b4e1b5
25962480 bytes

scons wesnoth --option-cache="" --debug=time ctool=gcc cxxtool=g++ ccache=yes build=release nls=no jobs=12 extra_flags_release="-Wl,-O1"
2a06a81d9f29c409861384ddffcd6325de507d458b64110d4d47208f7c7df1f3
25962480 bytes
all produce an executable with the exact same size, so without more information I'm not sure they provide any benefit, at least in Wesnoth's case.

---edit---

To add to that a bit - I'm not opposed to adding additional optimization flags, especially seeing as I've added a fair few myself, but there should definitely be some definable benefit from adding them(ie: adding -fno-plt to the hardening flags once Wesnoth's minimum GCC/clang versions support it). So in the cases of --sort-common and --as-needed, it seems to me that, at least for Wesnoth, they don't actually do anything. As far as -Wl,-O1 goes, I'm not opposed per se, but I am somewhat suspicious of it both because it produced an executable of the *exact* same size(which seems strange/unlikely to me, even if the hash differed) and because it also seems strange to me that it hasn't become the default behavior of the linker in the ~12 years since it was introduced if it provided noticeable benefits.
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
User avatar
Pentarctagon
Project Manager
Posts: 5496
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon »

To add some more/summarize:
  • fno-plt: GCC 6+, is a minor optimization flag that can be enabled along with full RELRO.
  • -mmitigate-rop: GCC 6+, is an attempt to try to mitigate Return Oriented Programming. However, even GCC's documentation currently says that this is "limited in what it can do and should not be relied on to provide serious protection". Some additional information is here.
  • -D_GLIBCXX_ASSERTIONS: Enables bounds checking for strings and containers as well as null pointer checks when dereferencing smart pointers, as per Redhat's recommended flags and the GCC documentation. This does however seem to generate a false positive warning with GCC 7, possible related to this bug.
  • -fstack-clash-protection: GCC 8+, improved stack protections, also as per the above Redhat recommended flags as well as the GCC documentation. There is the more widely available fstack-check option, however it apparently has some issues and although it was brought up for being enabled by default in Arch, this does not seem to have happened.
99 little bugs in the code, 99 little bugs
take one down, patch it around
-2,147,483,648 little bugs in the code
Post Reply