Enabling hardening options for Wesnoth's executables

Discussion among members of the development team.

Moderators: Forum Moderators, Developers

Post Reply
User avatar
Pentarctagon
Forum Administrator
Posts: 3425
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Enabling hardening options for Wesnoth's executables

Post by Pentarctagon » June 11th, 2018, 6:02 am

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
Developer
Posts: 30
Joined: July 7th, 2016, 6:20 pm
Location: Helsinki, Finland

Re: Enabling hardening options for Wesnoth's executables

Post by jyrkive » June 11th, 2018, 6:48 am

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
Forum Administrator
Posts: 3425
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon » 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.
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
Developer
Posts: 30
Joined: July 7th, 2016, 6:20 pm
Location: Helsinki, Finland

Re: Enabling hardening options for Wesnoth's executables

Post by jyrkive » 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.

Shiki
Developer
Posts: 185
Joined: July 13th, 2015, 9:53 pm
Location: Germany

Re: Enabling hardening options for Wesnoth's executables

Post by Shiki » June 11th, 2018, 9:34 am

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.
Maintainer of Era of Myths.

User avatar
Pentarctagon
Forum Administrator
Posts: 3425
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon » June 11th, 2018, 2:55 pm

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
Forum Administrator
Posts: 3425
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon » 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.
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
Developer
Posts: 30
Joined: July 7th, 2016, 6:20 pm
Location: Helsinki, Finland

Re: Enabling hardening options for Wesnoth's executables

Post by jyrkive » June 12th, 2018, 8:00 pm

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
Forum Administrator
Posts: 3425
Joined: March 22nd, 2009, 10:50 pm
Location: Earth (occasionally)

Re: Enabling hardening options for Wesnoth's executables

Post by Pentarctagon » June 14th, 2018, 5:50 am

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

Post Reply