[compiler-rt] implement COMMON_INTERCEPTOR_COPY_STRING for asan

[aralisza]

Summary

COMMON_INTERCEPTOR_COPY_STRING only had an implementation in msan, but it would make sense to implement it for asan as well.
This was added due to false negative in ASan on buggy strlcpy usage. Added a regression test for strlcpy as well.

rdar://83965778

[aralisza]

set linux as unsupported

[delcypher]

Note we'll need another test case if we choose to add support for checking overlapping params.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

308

Given that this macro is supposed to model a copy operation we probably need to model the read operation too with something like.

COMMON_INTERCEPTOR_READ_RANGE(from, to, size);

However, this isn't really enough because it's undefined what happens when to and from overlap for strlcpy.

ASan already has support for this kind of thing in its CHECK_RANGES_OVERLAP macro. So maybe we could introduce a COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP() macro

// In `asan_interceptors.cpp`
#define COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP(ctx, name, buffer1, length1, buffer2, length2 ) { \
  CHECK_RANGES_OVERLAP(name, buffer1, length1, buffer2, length2) \
}

// In `sanitizer_common_interceptors.inc`

// FIXME: Figure this out. Maybe we should implement a barebones implementation here?
#ifndef  COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP
#define COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP(ctx, name, buffer1, length1, buffer2, length2 ) { }
#endif


#ifndef COMMON_INTERCEPTOR_COPY_STRING
#define COMMON_INTERCEPTOR_COPY_STRING(ctx, name, to, from, size) { \
  COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP(ctx, name, from, size, to, size) \
  COMMON_INTERCEPTOR_READ_RANGE(ctx, from, size); \
  COMMON_INTERCEPTOR_WRITE_RANGE(ctx, to, size); \
}
#endif

Note I changed COMMON_INTERCEPTOR_CHECK_RANGES_OVERLAP's signature to pass name which is the name of the function being intercepted because the CHECK_RANGES_OVERLAP macro needs it.

compiler-rt/test/asan/TestCases/Posix/strlcpy-buffer-overflow.cpp

4

Why unsupported on Linux?

[delcypher]

Another way we could go about this is just implement an ASan specific implementation, e.g. we already have one for INTERCEPTOR(char *, strcpy, char *to, const char *from) in asan_interceptors.cpp.

[aralisza]

I didn't originally add this to asan_interceptors.cpp because there was already an interceptor in the common interceptors. If I want to copy it to asan interceptors, would I need to delete it from common interceptors? That seems like it might be a riskier change

compiler-rt/test/asan/TestCases/Posix/strlcpy-buffer-overflow.cpp

4

the other strlcpy tests are marked unsupported on linux https://reviews.llvm.org/D44432

also this test fails on debian according to pr testing

[aralisza]

move implementation of macro to asan interceptors

[aralisza]

add test for overlap

[yln]

Please also add a test for strlcat.

compiler-rt/lib/asan/asan_interceptors.cpp

106 ↗

(On Diff #379520)

Note that the passed size argument is the size of the destination buffer. src can be an arbitrarily long string. I think we should model the touched ranges a bit more precisely.

size_t strlcpy(char * restrict dst, const char * restrict src, size_t dstsize):
--->
src_size = internal_strlen(src) + 1
// Is reading a potentially non-terminated string here okay?  How do we handle this in other interceptors?

check_overlap(dst, dst_size, src, src_size)
read_range(src, src_size)
write_range(dst, min(src_size, dst_size))

compiler-rt/test/asan/TestCases/Posix/strlcpy_test.cpp

17 ↗

(On Diff #379520)

Note that the size argument is the size of the destination buffer.

Let's try to express the misuse of the API that we are trying to catch in the simplest way possible without complications (ARRAY_SIZE, malloc):

cont char *src = "long string";
const dst_size = 3;
char dst[dst_size];
strlcpy(dst, src, dst_size+1);

[aralisza]

add test for strlcat and check strlen of src in copy string check

[aralisza]

fix strndup tests

[yln]

LGTM, thanks!

[aralisza]

@krytarowski @vitalybuka Can one of you take a look at this since it is shared with non-Darwin platforms?

[delcypher]

LGTM

[aralisza]

@krytarowski @vitalybuka friendly ping, if there are no comments or concerns, I will land this next week

[vitalybuka]

You can add reviewers as blocking or dashboard shows that to me as already reviewed.

compiler-rt/lib/asan/asan_interceptors.cpp

107 ↗

(On Diff #379567)

All COMMON_INTERCEPTOR_COPY_STRING are preceded by COMMON_INTERCEPTOR_READ_STRING so we have redundant input checks.

106 ↗

(On Diff #379520)

It should be
ASAN_READ_STRING(ctx, from, Min(from_size, to_size))); like in strcat
internally it checks ->strict_string_checks which provides logic you asked
when it's ON it's produce a lot of reports on some "clever" parser.

[vitalybuka]

compiler-rt/lib/asan/asan_interceptors.cpp

107 ↗

(On Diff #379567)

You can try to remove both COMMON_INTERCEPTOR_READ_STRING
but make sure to move COMMON_INTERCEPTOR_COPY_STRING under if(intercept_strndup)
and make appropriate checks here as strlcpy "if (src)"

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

 
 #ifndef COMMON_INTERCEPTOR_MMAP_IMPL
 #define COMMON_INTERCEPTOR_MMAP_IMPL(ctx, mmap, addr, sz, prot, flags, fd, \
                                      off)                                  \
   { return REAL(mmap)(addr, sz, prot, flags, fd, off); }
 #endif
 
 #ifndef COMMON_INTERCEPTOR_COPY_STRING
-#define COMMON_INTERCEPTOR_COPY_STRING(ctx, to, from, size) {}
+#define COMMON_INTERCEPTOR_COPY_STRING(ctx, to, from, size) { \
+  COMMON_INTERCEPTOR_WRITE_RANGE(ctx, to, size); \
+}
 #endif
 
 #ifndef COMMON_INTERCEPTOR_STRNDUP_IMPL
 #define COMMON_INTERCEPTOR_STRNDUP_IMPL(ctx, s, size)                         \
   COMMON_INTERCEPTOR_ENTER(ctx, strndup, s, size);                            \
   uptr copy_length = internal_strnlen(s, size);                               \
   char *new_mem = (char *)WRAP(malloc)(copy_length + 1);                      \
   if (common_flags()->intercept_strndup) {                                    \

compiler-rt/test/asan/TestCases/Posix/strlcpy-buffer-overflow.cpp

+// RUN: %clangxx_asan -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
+
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define ARRAY_SIZE(X) sizeof(X) / sizeof(X[0])
+
+int main() {
+  const char src[] = "@@@@plz_copy_me@@@@";
+  char *buffer = (char *)malloc(1);      // Too small
+  strlcpy(buffer, src, ARRAY_SIZE(src)); // BOOM
+  // CHECK: heap-buffer-overflow
+  printf("should not be reached");
+  // CHECK-NOT: should not be reached
+  free(buffer);
+  return 0;
+}
 No newline at end of file