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 branch protection flags #260

Merged
merged 8 commits into from
Nov 8, 2023
Merged

Add branch protection flags #260

merged 8 commits into from
Nov 8, 2023

Conversation

david-a-wheeler
Copy link
Contributor

No description provided.

Signed-off-by: David A. Wheeler <[email protected]>
Signed-off-by: David A. Wheeler <[email protected]>
@david-a-wheeler
Copy link
Contributor Author

@eslerm @thomasnyman - what do you think?

@kees
Copy link

kees commented Oct 18, 2023

This looks good to me!

Copy link

@eslerm eslerm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @david-a-wheeler !

@siddhesh
Copy link
Contributor

Should there be a separate Architecture-specific hardening flags section? Also, recommending both flags in the TL;DR section seems wrong since the flags won't get recognized anywhere other than the platform they're implemented for.

@david-a-wheeler
Copy link
Contributor Author

siddhesh commented yesterday asked:

Should there be a separate Architecture-specific hardening flags section? Also, recommending both flags in the TL;DR section seems wrong since the flags won't get recognized anywhere other than the platform they're implemented for.

I disagree. Many projects compile for multiple architectures. Even if they only use one today, they may add another one tomorrow. I think it's safer to enable the hardening flags where you can, even if they don't apply to the architecture you're using today. It hurts nothing.

Signed-off-by: David A. Wheeler <[email protected]>
@david-a-wheeler
Copy link
Contributor Author

I've posted an update that I believe resolves all posted issues.

@siddhesh
Copy link
Contributor

I disagree. Many projects compile for multiple architectures. Even if they only use one today, they may add another one tomorrow. I think it's safer to enable the hardening flags where you can, even if they don't apply to the architecture you're using today. It hurts nothing.

I'm not saying we shouldn't recommend the architecture-specific flags in the TL;DR; section, I'm saying that the way it is represented right now is incorrect because it will result in a compiler error on all architectures since the architecture-specific flags are recognized only on the supported target. Maybe something like this instead:

When compiling C or C++ code on compilers such as GCC and clang, turn on these generic flags for detecting vulnerabilities at compile time and enable run-time protection mechanisms:
~~~~sh
-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
~~~~

Additionally, turn on `-fcf-protection=full` on x86 or  `-mbranch-protection=standard` on aarch64 to get protection against ROP based security attacks on hardware platforms that support it.

Instead of "on Intel" I'll just say "on many x86 architectures". If they need to know precisely which ones they'll need to consult their specific compiler documentation anyway.

The instructions for both PAC/BTI and SHSTK/IBT are in the NOP space, so they'll work seamlessly even on chips that don't have those features. In fact FWIW, SHSTK only got kernel support in 6.6, so at the moment it's still NOP pretty much everywhere in the Linux world.

@david-a-wheeler
Copy link
Contributor Author

I'm not saying we shouldn't recommend the architecture-specific flags in the TL;DR; section, I'm saying that the way it is represented right now is incorrect because it will result in a compiler error on all architectures since the architecture-specific flags are recognized only on the supported target.

Are you sure? I don't see that behavior. I tried this on both clang and GCC on Mac Intel - including the ARM options - and had no problems.

Here's my text Makefile:

# Set CC to the compiler to use. E.g.:

CC=/usr/local/Cellar/gcc/13.2.0/bin/gcc-13

CFLAGS_HARDENING = \
-O2 -Wall -Wformat=2 -Wconversion -Wtrampolines -Werror \
-D_FORTIFY_SOURCE=3 \
-D_GLIBCXX_ASSERTIONS \
-fstack-clash-protection -fstack-protector-strong \
-fcf-protection=full -mbranch-protection=standard \
-Wl,-z,nodlopen -Wl,-z,noexecstack \
-Wl,-z,relro -Wl,-z,now \
-fPIE -pie -fPIC -shared

demo: demo.c
	$(CC) $(CFLAGS_HARDNING) $(CFLAGS) -o demo demo.c

@eslerm
Copy link

eslerm commented Oct 24, 2023

It might be more accurate to say that these flags mitigate code reuse attacks, rather than branch protections or specifying ROP. -fcfp is not exclusively for branch protection.

iiuc, the stronger portion of -fcfp is it's shadow stack protection to prevent ROP, where as the endbranch protection is for COP/JOP. -mbranch-protections' BTI and PAC are both branch specific.

@david-a-wheeler
Copy link
Contributor Author

Ah! My makefile had a bug. Once fixed, I do see clang generating errors for unknown flags. Well, that's annoying.

I guess we can list options "per architecture". Frankly the best solution here is something like autoconf, so you can add all the options that work, but not everyone will use autoconf. Is there a better approach?

@david-a-wheeler
Copy link
Contributor Author

I've pulled out the architecture-specific flags. How is this?

Signed-off-by: David A. Wheeler <[email protected]>
@david-a-wheeler
Copy link
Contributor Author

I haven't resolved this one: eslerm suggested changes last week - suggestions welcome.

Signed-off-by: David A. Wheeler <[email protected]>
This fixes a merge conflict.

Signed-off-by: David A. Wheeler <[email protected]>
@david-a-wheeler
Copy link
Contributor Author

I managed to eliminate the merge conflicts (for the moment). I propose merging this (as it's an improvement), and we can make future changes without fighting as many merge conflicts.

Signed-off-by: David A. Wheeler <[email protected]>
@david-a-wheeler
Copy link
Contributor Author

Per meeting today, we agreed to merge this.

@david-a-wheeler david-a-wheeler merged commit 252af4c into main Nov 8, 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.

4 participants