This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] implement COMMON_INTERCEPTOR_COPY_STRING for asan
Needs RevisionPublic

Authored by aralisza on Oct 8 2021, 5:46 PM.

Details

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

Diff Detail

Event Timeline

aralisza created this revision.Oct 8 2021, 5:46 PM
aralisza requested review of this revision.Oct 8 2021, 5:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2021, 5:46 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
aralisza updated this revision to Diff 379129.Oct 12 2021, 11:49 AM

set linux as unsupported

aralisza edited the summary of this revision. (Show Details)

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 ↗(On Diff #379129)

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
3 ↗(On Diff #379129)

Why unsupported on Linux?

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.

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
3 ↗(On Diff #379129)

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 edited the summary of this revision. (Show Details)Oct 13 2021, 1:19 PM
aralisza updated this revision to Diff 379515.Oct 13 2021, 1:37 PM

move implementation of macro to asan interceptors

aralisza updated this revision to Diff 379520.Oct 13 2021, 1:51 PM

add test for overlap

yln added a comment.Oct 13 2021, 2:51 PM

Please also add a test for strlcat.

compiler-rt/lib/asan/asan_interceptors.cpp
106

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
18

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 updated this revision to Diff 379538.Oct 13 2021, 3:52 PM

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

aralisza updated this revision to Diff 379567.Oct 13 2021, 5:35 PM

fix strndup tests

yln accepted this revision.Oct 14 2021, 2:06 PM

LGTM, thanks!

This revision is now accepted and ready to land.Oct 14 2021, 2:06 PM

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

delcypher accepted this revision.EditedOct 14 2021, 3:38 PM

LGTM

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

vitalybuka added 1 blocking reviewer(s): vitalybuka.Oct 22 2021, 12:42 AM

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

compiler-rt/lib/asan/asan_interceptors.cpp
106

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.

107

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

This revision now requires review to proceed.Oct 22 2021, 12:42 AM
vitalybuka added inline comments.Oct 22 2021, 1:23 AM
compiler-rt/lib/asan/asan_interceptors.cpp
107

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)"

vitalybuka requested changes to this revision.Nov 3 2021, 12:33 PM
This revision now requires changes to proceed.Nov 3 2021, 12:33 PM