Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add simple test of C/C++ hardening flags #270

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Conversation

david-a-wheeler
Copy link
Contributor

No description provided.

@david-a-wheeler
Copy link
Contributor Author

Add a simple test of the C/C++ hardening flags; just run "make" (presuming this is GNU make).

@david-a-wheeler
Copy link
Contributor Author

I should note that the tests currently fail on at my Apple MacOS system:

Apple clang version 14.0.3 (clang-1403.0.22.14.1) fails with:

make CC=clang
clang -O2 -Wall -Wformat=2 -Wconversion -Wtrampolines -Werror  -D_FORTIFY_SOURCE=3  -D_GLIBCXX_ASSERTIONS  -fstack-clash-protection -fstack-protector-strong  -Wl,-z,nodlopen -Wl,-z,noexecstack  -Wl,-z,relro -Wl,-z,now  -fPIE -pie -fPIC -shared     demo.c   -o demo
clang: error: argument unused during compilation: '-fstack-clash-protection' [-Werror,-Wunused-command-line-argument]
clang: error: argument unused during compilation: '-pie' [-Werror,-Wunused-command-line-argument]

I think clang wants -fpie not -pie, per:
https://clang.llvm.org/docs/ClangCommandLineReference.html
... and it doesn't support -Wtrampolines. We should probably fix the former and discuss the latter.

make CC=/usr/local/Cellar/gcc/13.2.0/bin/gcc-13
/usr/local/Cellar/gcc/13.2.0/bin/gcc-13 -O2 -Wall -Wformat=2 -Wconversion -Wtrampolines -Werror  -D_FORTIFY_SOURCE=3  -D_GLIBCXX_ASSERTIONS  -fstack-clash-protection -fstack-protector-strong  -Wl,-z,nodlopen -Wl,-z,noexecstack  -Wl,-z,relro -Wl,-z,now  -fPIE -pie -fPIC -shared     demo.c   -o demo
ld: unknown option: -z

I think this is a configuration problem on my system. I think gcc is trying to use the Apple MacOS ld command at /usr/bin/ld, which is a hobbled mess, instead of GNU ld. But I suspect other users will see this, so we probably ought to discuss the issue of different loaders (ld) supporting different options.

@david-a-wheeler
Copy link
Contributor Author

@thomasnyman @gkunz - at the very least, I think -pie should be -fpie per this test. We also may need to move -Wtrampolines into a new gcc-only category, and warn that some other flags may or may not work depending on details such as compiler selected and its version, linker selected and its version, etc.

Do you see any other changes that this test suggests?

@gkunz
Copy link
Contributor

gkunz commented Oct 30, 2023

Hi @david-a-wheeler, I really like the idea of adding a test - even though (or just because?) it surfaces some issues with the current set of options.

I can confirm your findings on my Mac (and I assume we basically the same version of MacOS 14.1).

Moreover, after getting too annoyed by my Mac, I ran the demo on an older Ubuntu 22.04.3 LTS with the following result:

cc -O2 -Wall -Wformat=2 -Wconversion -Wtrampolines -Werror  -D_FORTIFY_SOURCE=3  -D_GLIBCXX_ASSERTIONS  -fstack-clash-protection -fstack-protector-strong  -Wl,-z,nodlopen -Wl,-z,noexecstack  -Wl,-z,relro -Wl,-z,now  -fPIE -pie -fPIC -shared     demo.c   -o demo
<command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror]
<built-in>: note: this is the location of the previous definition

It seems that gcc on Ubuntu has predefined FORTIFY_SOURCE as something else but 3 and therefore it complains about redefining it. One solution is to undefine FORTIFY_SOURCE first and then setting it again.

cc -O2 -Wall -Wformat=2 -Wconversion -Wtrampolines -Werror -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3  -D_GLIBCXX_ASSERTIONS  -fstack-clash-protection -fstack-protector-strong  -Wl,-z,nodlopen -Wl,-z,noexecstack  -Wl,-z,relro -Wl,-z,now  -fPIE -pie -fPIC -shared     demo.c   -o demo

Other findings?

@david-a-wheeler
Copy link
Contributor Author

One solution is to undefine FORTIFY_SOURCE first and then setting it again.

That sounds like a good solution. We should propose it and explain why (in a separate PR). Will you write that up @gkunz ?

@gkunz
Copy link
Contributor

gkunz commented Oct 31, 2023

@david-a-wheeler yes

@david-a-wheeler
Copy link
Contributor Author

All agreed to adding this simple test, so I'm merging it. The test has revealed some problems - they need to be handled by other PRs. I plan to create a separate PR to turn -pie into -fpie but obviously that's not a test.

@david-a-wheeler david-a-wheeler merged commit 2a2991d into main Nov 3, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants