google / sanitizers

AddressSanitizer, ThreadSanitizer, MemorySanitizer
Other
11.43k stars 1.03k forks source link

Asan does not detect out-of-bounds accesses to argv #762

Open mrigger opened 7 years ago

mrigger commented 7 years ago

Consider the following example:

#include <stdio.h>

int main(int argc, char** argv) {
    printf("%d %s\n", argc, argv[5]);
}

Compile the program with clang-3.9 -fsanitize=address test.c and execute it with ./a.out. Even though argv[5] causes an out-of-bounds access no error is displayed. Instead, execution prints values of the *envp[] array. Also Valgrind did not detect the error.

I found the issue while executing a MD5 implementation with Safe Sulong. Safe Sulong detected the error and aborted the program (see PR). Although the bug is not exploitable in this case, similar errors could allow to leak potentially sensitive data in the environment.

kcc commented 7 years ago

The buffer for argv is created by libc, which we don't instrument, so we can't insert redzones. Probably, a simplest fix would be to add a redzone in libc and make asan poison it, but that's outside of our control.

Another possible fix is to have a special case logic in asan instrumentation pass to copy argv at the very beginning of main into a separate buffer. This should work, but I am not sure if it's worth the effort.

I'll keep this open, but don't expect this to be implemented soon.

dvyukov commented 7 years ago

Do we actually need to add redzones? Can't we just poison what's beyond argc? I assume that argv is not heap allocated, because otherwise asan would add a redzone and poison it. So if it's a large global, and user is supposed to use only a small part of it, we could just poison the rest. Libc itself should not touch beyond argc, but even if it does, it's not instrumented anyway.

yugr commented 7 years ago

Won't you poison contents of environ?

dvyukov commented 7 years ago

I guess we need to try. I see that kernel seems to leave a hole of 1 pointer between argv and envp specifically for us: http://lxr.free-electrons.com/source/fs/binfmt_elf.c#L297 But maybe it is used for something...

kcc commented 7 years ago

On linux x86_64 with glibc if I simply add

  __asan_poison_memory_region(&argv[argc], 8);

at the beginning of main(), I get the desired behavior (looks like).

We can't do it on 32-bit since __asan_poison_memory_region has 8-byte granularity. I am not at all sure about other libc implementations or other OSes -- will need to experiment.

Also, there is at least some evidence that argv[argc] is expected to be NULL (and hence should be accessible). http://publications.gbdirect.co.uk/c_book/chapter10/arguments_to_main.html

argv[argc] is a null pointer.

But I don't see it in the standard (e.g. in http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)

dvyukov commented 7 years ago

Unfortunately C++ states that:

3.6.1/2
... The value of argv[argc] shall be 0.
mrigger commented 7 years ago

I found that it is defined for C (see 5.1.2.2.1.2):

argv[argc] shall be a null pointer

Also real-world code seems to rely on checks like this one (e.g., see here):

// Exit if no argument is passed
if(argv[1] == NULL)
{
    return 0;
}
yugr commented 7 years ago

Interposing argv in main may work in most cases.

morehouse commented 6 years ago

Another possible fix is to have a special case logic in asan instrumentation pass to copy argv at the very beginning of main into a separate buffer. This should work, but I am not sure if it's worth the effort.

We did this for byval arguments already. Maybe we can re-use the machinery for this.