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 additional undefined behaviors #232

Closed
mikea opened this issue Dec 30, 2016 · 15 comments
Closed

Add additional undefined behaviors #232

mikea opened this issue Dec 30, 2016 · 15 comments

Comments

@mikea
Copy link
Contributor

mikea commented Dec 30, 2016

Current config: bool,signed-integer-overflow,shift,vptr

@Dor1s
Copy link
Contributor

Dor1s commented Dec 30, 2016

What do you think about function? We've seen some of them detected on internal CF.

@inferno-chromium
Copy link
Collaborator

We have https://cs.chromium.org/chromium/src/build/config/sanitizers/BUILD.gn?rcl=bbf5af70f469a5f7807ec4573a65c3710cfbb29a&l=512

Should we enable function, vla-bound. How about others ?

@inferno-chromium
Copy link
Collaborator

@Dor1s - when you get time, can you add parsing signature+test for vla-bound (don't see in current clusterfuzz code) and then we can enable it here.

@Dor1s
Copy link
Contributor

Dor1s commented Feb 7, 2017

I have a feeling that we treat vla-bound errors as heap-buffer-overflow, but not 100% sure... I'll take a look soon. Do we want it filed as a separate issue?

@inferno-chromium
Copy link
Collaborator

Yes probably, but what about if we skip past the left redzone for a large negative value. We can probably wait on @kcc to respond here if vla-bound is even needed since ASAN should probably catch this.

@Dor1s
Copy link
Contributor

Dor1s commented Feb 7, 2017

I mixed up that with -fsanitize=bounds (Out of bounds array indexing, in cases where the array bound can be statically determined), this is what we report as heap-buffer-overflow.

vla-bound is only for cases like this:

$ cat vla.cc 
int main(int argc, char* argv[]) {
  int n = 5;
  n -= 10;
  int a[n];
  return 0;
}

$ clang vla.cc -fsanitize=vla-bound -o vla && ./vla 
vla.cc:4:9: runtime error: variable length array bound evaluates to non-positive value -5

ASan doesn't report an attempt to declare an array with a negative size:

$ clang vla.cc -fsanitize=address -o vla && ./vla 
$ echo $?
0

But reports an attempt to use it, e.g.:

$ cat vla.cc 
#include <stdio.h>

int main(int argc, char* argv[]) {
  int n = 5;
  n -= 10;
  int a[n];
  printf("%i\n", a[0]);
  return 0;
}

$ clang vla.cc -fsanitize=address -o vla && ./vla 
ASAN:DEADLYSIGNAL
=================================================================
==11176==ERROR: AddressSanitizer: stack-overflow on address 0x7ff9f171d358 (pc 0x0000004ec705 bp 0x7ffdf171d440 sp 0x7ff9f171d360 T0)
    #0 0x4ec704 in main (/usr/local/google/home/mmoroz/Projects/clusterfuzz/vla+0x4ec704)
    #1 0x7f5b52872f44 in __libc_start_main /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287
    #2 0x41977b in _start (/usr/local/google/home/mmoroz/Projects/clusterfuzz/vla+0x41977b)

SUMMARY: AddressSanitizer: stack-overflow (/usr/local/google/home/mmoroz/Projects/clusterfuzz/vla+0x4ec704) in main
==11176==ABORTING

$ echo $?
1

@Dor1s
Copy link
Contributor

Dor1s commented Feb 7, 2017

Hm, strange. The following code:

#include <stdio.h>

int main(int argc, char* argv[]) {
  int n = 5;
  n -= 10;
  int a[n];
  printf("%lu\n", sizeof(a));
  return 0;
}

doesn't break with ASan:

$ clang vla.cc -g -fsanitize=address -o vla && ASAN_OPTIONS=symbolzie=1 ./vla 
17179869164

$ echo $?
0

So probably it makes sense to be added, but +1 to wait for @kcc

@kcc
Copy link
Contributor

kcc commented Feb 8, 2017

Hm... I've no opinion and no experience with -fsanitize=vla-bound.
In particular, I don't know a situation when such bug will not lead to a stack overflow.
(in your case, int a[n] is not used and so is optimized away).

Perhaps it's still worth trying.

@Dor1s
Copy link
Contributor

Dor1s commented Feb 8, 2017

I also thought that it might haave been optimized away, but why it is not omitted when building with UBSan...

Ok, let's add it and see if we catch anything unique!

@Dor1s
Copy link
Contributor

Dor1s commented Feb 8, 2017

It seems that declaration of a variable-length array with a negative length ends up as an arbitrary pointer in memory:

  1. VLA of length 5:
mov     [rbp+n], 5
mov     edi, [rbp+n]
mov     esi, edi
mov     rcx, rsp
mov     [rbp+var_20], rcx
  1. VLA of length -5:
mov     [rbp+n], 5
mov     edi, [rbp+n]
add     edi, 0FFFFFFF6h
mov     [rbp+n], edi
mov     edi, [rbp+n]
mov     esi, edi
mov     rcx, rsp
mov     [rbp+var_20], rcx

Due to that, we should mark vla-bound crashes as security issues by default.

@inferno-chromium
Copy link
Collaborator

inferno-chromium commented Feb 8, 2017

Thanks Max for adding the parsing signatures, enabled vla-bound in 921f143.

@inferno-chromium
Copy link
Collaborator

Enabled object-size in 45d8efa.

@inferno-chromium
Copy link
Collaborator

Removing object size due to all targets crashing on

WARNING: Failed to find function "__sanitizer_print_stack_trace".
/usr/local/bin/../include/c++/v1/algorithm:714:10: runtime error: member access within address 0x7ffdfeeb4730 with insufficient space for an object of type 'const std::__1::__less<unsigned long, unsigned long> '
0x7ffdfeeb4730: note: pointer points here
e4 7f 00 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50 64 40 00 00 00 00 00 60 48 eb fe
^
#0 0x439a1b in fuzzer::FuzzerDriver(int
, char***, int ()(unsigned char const, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp
#1 0x438cfc in main /src/libfuzzer/FuzzerMain.cpp:20:10
#2 0x7fe4df41582f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#3 0x406478 in _start (/out/magic_fuzzer+0x406478)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/local/bin/../include/c++/v1/algorithm:714:10 in
ERROR: bad exit code: 1
1 fuzzers total (1 failed).

@inferno-chromium
Copy link
Collaborator

fyi - 48f8d5e, Enable enabled UBSan builtin, null, returns-nonnull-attribute, and unreachable

@inferno-chromium
Copy link
Collaborator

inferno-chromium commented May 31, 2020

Remaining ones - alignment, full-bounds, pointer-overflow (after that noisy null one is fixed), etc.

DavidKorczynski pushed a commit that referenced this issue Jul 9, 2024
In the benchmark page:
1. Use green if the value in the `Builds` column is `True`.
2. Use red if the value in the `Crashes` or `Bug` columns is `False`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants