Skip to content
New issue

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

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

Already on GitHub? # to your account

Support source fortification #247

Open
ramosian-glider opened this issue Aug 31, 2015 · 16 comments
Open

Support source fortification #247

ramosian-glider opened this issue Aug 31, 2015 · 16 comments

Comments

@ramosian-glider
Copy link
Member

Originally reported on Google Code with ID 247

Right now we disable source fortification by defining _FORTIFY_SOURCE=0
This may hide a number of bugs that could otherwise be detected by various _chk functions
(__printf_chk, __strcpy_chk etc.)
A better approach would be to wrap all the _chk functions and let the users enable
source fortification.

A suggestion from Jakub Jelinek:

>Well, -D_FORTIFY_SOURCE=2 does things that asan doesn't and can't do, so
>disabling fortification if you build with -fsanitize=address sounds like 
>a very bad idea to me.
>IMHO libasan should intercept also the __*_chk calls, test + branch to 
>__chk_fail if they should fail, otherwise fall through to the 
>intercepted original function.
>For *printf* family __printf_chk etc. also fail on %n if it isn't in >read-only string
literal.

Reported by ramosian.glider on 2013-11-22 13:48:10

@ramosian-glider
Copy link
Member Author

(see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59148)

Reported by ramosian.glider on 2013-11-22 14:39:09

@ramosian-glider
Copy link
Member Author

I think this is becoming more important now that Ubuntu enabled -D_FORTIFY_SOURCE=2
by default. Note that there is not easy way to disable it - passing -D_FORTIFY_SOURCE=0
when -fsanitize=address is active would cause ugly warnings about macro redefinition.

Reported by tetra2005x on 2015-01-14 11:22:36

@ramosian-glider
Copy link
Member Author

Tizen enables -D_FORTIFY_SOURCE by default as well. Looks like this feature has become
very popular...

Reported by tetra2005x on 2015-01-26 09:44:59

@ramosian-glider
Copy link
Member Author

Does it mean that these bugs are out-of-date and clang now supports FORTIFY_SOURCE?
http://llvm.org/bugs/show_bug.cgi?id=18775
http://llvm.org/bugs/show_bug.cgi?id=16821

I guess we should support it then.

Reported by eugenis@google.com on 2015-01-26 10:15:27

@ramosian-glider
Copy link
Member Author

> Does it mean that these bugs are out-of-date
> and clang now supports FORTIFY_SOURCE?

I'm not sure - all my targets use GCC. Simple examples seem to work with TOT Clang
though.

Will you accept libasan patch with fortified interceptors or should we wait until fortification
is stable in Clang?

Reported by tetra2005x on 2015-01-26 10:49:52

@ramosian-glider
Copy link
Member Author

Sure, I don't see any harm in it.

Reported by eugenis@google.com on 2015-01-26 11:52:34

@kcc
Copy link
Contributor

kcc commented Dec 1, 2015

Not working on this.
Please open new bug(s) with more details if needed

@kcc kcc closed this as completed Dec 1, 2015
@ghost
Copy link

ghost commented Dec 3, 2015

Can't file bugs but I'd say the feature is very important. All modern distros enable fortification by default so not supporting this means that we effectively disable checks for memset, memcpy and other extremely interesting functions...

@kcc kcc reopened this Dec 3, 2015
@kcc
Copy link
Contributor

kcc commented Dec 3, 2015

Let's keep it open, although we are still not working on this.

@kcc
Copy link
Contributor

kcc commented Aug 24, 2016

See also: https://sourceware.org/bugzilla/show_bug.cgi?id=20422 (@hannob )
The only reasonable thing is to make sure _FORTIFY_SOURCE is set to 0 when asan/msan/tsan is used.

@sitsofe
Copy link

sitsofe commented Aug 25, 2016

@kcc :

Re sourceware issue 20422: in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59148#c4 Jakaob Jelinek suggests there are things that -D_FORTIFY_SOURCE=2 can do that things like asan can't although sadly it's not stated what those are so it could be that other sanitizers cover them. It might be worth asking what those were...

@kcc
Copy link
Contributor

kcc commented Aug 25, 2016

@sitsofe at the very least fortify has more libc interceptors than asan.
The solution to this is not to add interceptors for foo_chk(), but to add interceptors for foo()
or instrument glibc with asan.

@westurner
Copy link

westurner commented Dec 6, 2019

Making sure I understand:

  • ASan is functionally similar to FORTIFY_SOURCE
  • Things break when FORTIFY_SOURCE and ASan are both enabled
  • Ideally, we could have both FORTIFY_SOURCE and asan
    • The decision made here is that asan is preferable to FORTIFY_SOURCE?
    • Is there a different issue discussing how to make FORTIFY_SOURCE and asan compatible?
  • Is memory sanitizer incompatible with _FORTIFY_SOURCE=2? #689 has some test code

@westurner
Copy link

westurner commented Dec 6, 2019

Should production builds choose FORTIFY_SOURCE (edit) FORTIFY_SOURCE=2?

Should debug builds choose asan?

@yugr
Copy link

yugr commented Dec 6, 2019

ASan is functionally similar to FORTIFY_SOURCE

In general yes but Asan is much more precise (and slower). It's preferred choice for debugging purposes (with -U_FORTIFY_SOURCE to disable fortification). Fortification is preferred for production builds though.

Things break when FORTIFY_SOURCE and ASan are both enabled

FORTIFY_SOURCE (which is enabled by default in modern distros) prevents Asan from detecting some types of bugs.

Ideally, we could have both FORTIFY_SOURCE and asan

Yes.

Is there a different issue discussing how to make FORTIFY_SOURCE and asan compatible?

The "how" has been discussed on GCC mailing list few years ago, try searching there.

@jiridanek
Copy link

Workaround is to compile with -Wp,-U_FORTIFY_SOURCE appended at the end of CFLAGS.

When there is annobin at play (RPM builds, or if you set your default CFLAGS by running eval "$(rpmbuild --eval '%set_build_flags')"), use -Wp,-U_FORTIFY_SOURCE -fplugin=annobin -fplugin-arg-annobin-no-active-checks to both disable fortification and tell annobin not to complain about it.

lzaoral added a commit to lzaoral/csmock that referenced this issue Jan 30, 2023
lzaoral added a commit to lzaoral/csmock that referenced this issue Feb 1, 2023
jktjkt added a commit to CESNET/CzechLight-dependencies that referenced this issue Feb 2, 2023
...because ASAN does an extended version of this, and it's said to
produce wrong results with fortification.

Bug: google/sanitizers#247
Change-Id: Ia4bbbf96a1c82ee76c332a0136e3f9e44af1f724
iii-i added a commit to iii-i/elfutils that referenced this issue Feb 5, 2023
Add support for clang Memory Sanitizer [1], which detects the usage of
uninitialized values. While elfutils itself is already checked with
valgrind, checking code that depends on elfutils requires elfutils to
be built with MSan.

Unlike the other sanitizers, MSan needs to be configured fairly early,
since we need to drop -D_FORTIFY_SOURCE [2], -Wl,-z,defs and
--no-undefined.

[1] https://clang.llvm.org/docs/MemorySanitizer.html
[2] google/sanitizers#247

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
iii-i added a commit to iii-i/elfutils that referenced this issue Feb 5, 2023
Add support for clang Memory Sanitizer [1], which detects the usage of
uninitialized values. While elfutils itself is already checked with
valgrind, checking code that depends on elfutils requires elfutils to
be built with MSan.

Unlike the other sanitizers, MSan needs to be configured fairly early,
since we need to drop -D_FORTIFY_SOURCE [2], -Wl,-z,defs and
--no-undefined.

[1] https://clang.llvm.org/docs/MemorySanitizer.html
[2] google/sanitizers#247

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
iii-i added a commit to iii-i/elfutils that referenced this issue Feb 5, 2023
Add support for clang Memory Sanitizer [1], which detects the usage of
uninitialized values. While elfutils itself is already checked with
valgrind, checking code that depends on elfutils requires elfutils to
be built with MSan.

MSan is not linked into shared libraries, and is linked into
executables statically. Therefore, unlike the other sanitizers, MSan
needs to be configured fairly early, since we need to drop
-D_FORTIFY_SOURCE [2], -Wl,-z,defs and --no-undefined.

Disable a few tests that run for more than 5 minutes due to test files
being statically linked with MSan.

[1] https://clang.llvm.org/docs/MemorySanitizer.html
[2] google/sanitizers#247

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
lzaoral added a commit to lzaoral/csmock that referenced this issue Feb 6, 2023
lzaoral added a commit to lzaoral/csmock that referenced this issue Feb 6, 2023
jollaitbot pushed a commit to sailfishos-mirror/elfutils that referenced this issue Feb 14, 2023
Add support for clang Memory Sanitizer [1], which detects the usage of
uninitialized values. While elfutils itself is already checked with
valgrind, checking code that depends on elfutils requires elfutils to
be built with MSan.

MSan is not linked into shared libraries, and is linked into
executables statically. Therefore, unlike the other sanitizers, MSan
needs to be configured fairly early, since we need to drop
-D_FORTIFY_SOURCE [2], -Wl,-z,defs and --no-undefined.

Disable a few tests that run for more than 5 minutes due to test files
being statically linked with MSan.

[1] https://clang.llvm.org/docs/MemorySanitizer.html
[2] google/sanitizers#247

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
lzaoral added a commit to lzaoral/csmock that referenced this issue Feb 15, 2023
gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Mar 13, 2023
…and MSAN

This can cause either false positives in warnings from the compiler or false
negatives where the sanitizer misses something.

Bug: google/sanitizers#247
Signed-off-by: Sam James <sam@gentoo.org>
gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Mar 13, 2023
This can cause either false positives in warnings from the compiler or false
negatives where the sanitizer misses something, so don't default-enable FORTIFY_SOURCE
when -fsanitize=address is set to be safe.

Bug: google/sanitizers#247
Signed-off-by: Sam James <sam@gentoo.org>
gentoo-bot pushed a commit to gentoo/gcc-patches that referenced this issue Mar 13, 2023
…bled

This can cause either false positives in warnings from the compiler or false
negatives where the sanitizer misses something.

Bug: google/sanitizers#247
Signed-off-by: Sam James <sam@gentoo.org>
gentoo-bot pushed a commit to gentoo/gcc-patches that referenced this issue Mar 13, 2023
…bled

This can cause either false positives in warnings from the compiler or false
negatives where the sanitizer misses something.

Bug: google/sanitizers#247
Signed-off-by: Sam James <sam@gentoo.org>
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

6 participants