Bug 20422 - do not allow asan/msan/tsan and fortify at the same time.
Summary: do not allow asan/msan/tsan and fortify at the same time.
Status: UNCONFIRMED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-28 14:39 UTC by Kostya Serebryany
Modified: 2018-05-06 01:12 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Untested patch. (437 bytes, patch)
2016-08-29 17:18 UTC, Maxim Ostapenko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kostya Serebryany 2016-07-28 14:39:34 UTC
When fortify is used with msan it will cause msan false positives. 

#include <stdio.h>
#include <string.h>
int main()
{
        char text[100];
        sprintf(text, "hello");
        printf("%lu\n", strlen(text));
}

% clang test.c -fsanitize=memory   -O3 && ./a.out 
5
% clang test.c -fsanitize=memory -D_FORTIFY_SOURCE=2  -O3 && ./a.out 
Uninitialized bytes in __interceptor_strlen at offset 0 inside [0x7ffe259e4d20, 6)
==26297==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x4869cc in main 

With asan, this will not cause false positives, but may case false negatives. 

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
char *T = "hello";
volatile int sink;
int main(int argc, char **argv)
{
        int n = argc == 1 ? 4 : atoi(argv[1]);
        char *text = (char*)malloc(n);
        sprintf(text, "%s", T);
        sink = text[1];
        free(text);
}

% clang asan-test.c -fsanitize=address   -O3  && ./a.out 2>&1 | grep ERROR
==27843==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000fbf4 at pc 0x0000004a7cb0 bp 0x7ffe22795bc0 sp 0x7ffe22795370
% clang asan-test.c -fsanitize=address   -O3 -D_FORTIFY_SOURCE=2 && ./a.out 2>&1 | grep ERROR 
%

same with tsan. 

This is happening because fortify replaces libc functions (e.g. sprintf)
with its own variants (__sprintf_chk) and the sanitizers don't know about these variants. 

supporting fortify in *san makes little sense because fortify does not add anything to the sanitizers and it will only increase the complexity. 

So, the better way is to warn the user that the sanitizers and fortify are incompatible. 

Florian suggested that the simplest way to warn is to modify the glibc headers
to check if fortify and one of the sanitizers is enabled. 

Note that gcc and clang use different ways to tell that a sanitizer is on
(macros in gcc, __has_feature in clang)
Comment 1 Kostya Serebryany 2016-08-24 23:22:38 UTC
Will you consider implementing this?
The most natural fix seems to be inside the glibc headers,
but if you are not planing to do it any time soon we'll have some 
kind of workaround in clang or *san run-times.
Comment 2 Maxim Ostapenko 2016-08-29 17:18:36 UTC
Created attachment 9480 [details]
Untested patch.

Something like this? I wonder how can we test this patch. Can we rely on GCC is recent enough to support -fsanitize=address?
Comment 3 Maxim Ostapenko 2016-08-29 17:20:40 UTC
(In reply to Maxim Ostapenko from comment #2)
> Created attachment 9480 [details]
> Untested patch.
> 
> Something like this? I wonder how can we test this patch. Can we rely on GCC
> is recent enough to support -fsanitize=address?

Or perhaps we can define __SANITIZE_ADDRESS__ manually and check the warning?
Comment 4 Kostya Serebryany 2016-08-29 17:39:08 UTC
Looks great (untested).
IMHO we can rely on recent-enough gcc, but the actual glibc folks would know better.
Comment 5 Florian Weimer 2016-08-30 14:40:40 UTC
These days, _FORTIFY_SOURCE covers additional aspects (including enhanced compile-time type safety), not just buffer-length checking.  I'm not sure what the current expectations regarding the sanitizers are.  Is it the norm to compile twice, for production (with -D_FORTIFY_SOURCE=2), and for special testing with the required sanitizer?

I'm asking this because if there is just a single compile (say with Address Sanitizer), then I think we need to make the enhanced type safety (and other goodies) available to sanitizer users as well.
Comment 6 Kostya Serebryany 2016-08-30 16:21:21 UTC
(In reply to Florian Weimer from comment #5)
> These days, _FORTIFY_SOURCE covers additional aspects (including enhanced
> compile-time type safety), not just buffer-length checking.  I'm not sure
> what the current expectations regarding the sanitizers are.  Is it the norm
> to compile twice, for production (with -D_FORTIFY_SOURCE=2), and for special
> testing with the required sanitizer?
> 
> I'm asking this because if there is just a single compile (say with Address
> Sanitizer), then I think we need to make the enhanced type safety (and other
> goodies) available to sanitizer users as well.

Typically, asan/msan/tsan are used as testing tools, not for production build.
And anyway, there are 3 of them, so you can't have a single build anyway. 
asan is sometimes used for production (e.g. on a small subset of servers)
to find the last remaining bugs. 
Very rarely asan is used as a hardened production build,
but the security value of this is disputable (at least, with the current implementation). 

Can you give examples (or point to docs) of the "additional aspects" of fortify?
If they fit well into asan's architecture we will certainly want to either
have them duplicated in asan or make asan work with those parts of fortify. 

The traditional parts of fortify (where it replaces foo with foo_chk)
are *san-hostile at the moment.
Comment 7 Florian Weimer 2016-08-31 17:28:42 UTC
(In reply to Kostya Serebryany from comment #6)

> Can you give examples (or point to docs) of the "additional aspects" of
> fortify?

An example is the argument type checking for open/openat, in io/bits/fcntl2.h.  I expect more things like this in the future.  Some of them do not inherently conflict with the ASan interceptors, but many others will (like variable argument list length checking for printf and similar functions).