[MSAN] add __b64_pton and __b64_ntop intercepts

[kda]

improved comparison

[kda]

remove unused include

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

2508

is "res >= targsize" possible?
Isn't this return -1 on overflow?

2514

COMMON_INTERCEPTOR_READ_RANGE(ctx, src, internal_strlen(src) + 1);

COMMON_INTERCEPTOR_READ_STRING usually use for parsers which may read just a part of the string
As I understand on success __b64_pton will read entire string

2518

is "res > targsize" possible?
also this one does not set trailing \0, so no +1

compiler-rt/test/msan/Linux/b64.cpp

4

that change is sanitizer_common
so we want a the which is executed with check-sanitizer:
e.g. compiler-rt/test/sanitizer_common/TestCases/Linux/b64.cpp

4

in addition to msan/Linux/b64.cpp

[kda]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

2508

Since the code is adding one ('+ 1'), I wanted to avoid writing past the end. So, the check is intended to prevent 'res == targsize'.

2514

READ_STRING depends on null termination, which I believe this method does as well. There is no length provided, how else would the length be determined.

https://elixir.bootlin.com/glibc/glibc-2.17/source/resolv/base64.c#L209

2518

Unit test fails without the '+ 1'.

Similar to above, check is to prevent writing beyond end of target.

compiler-rt/test/msan/Linux/b64.cpp

4

I'm not sure what to do.

Leave this one here, and clone it with different RUN lines to 'sanitizer_common/TestCases/Linux'?

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

2508

https://elixir.bootlin.com/glibc/glibc-2.17/source/resolv/base64.c#L184
if (datalength >= targsize)
		return (-1);
target[datalength] = '\0'

I do not insist, just recommend, COMMON_INTERCEPTOR_WRITE_RANGE(ctx, target, res + 1) is enough
if we have issues with so other implementation we can add workaround later

2514

#define COMMON_INTERCEPTOR_READ_STRING(ctx, s, n) \

COMMON_INTERCEPTOR_READ_RANGE((ctx), (s),                       \
  common_flags()->strict_string_checks ? (internal_strlen(s)) + 1 : (n) )

strict_string_checks (default: false) means even if REAL() reads only few first byte of the string, entire string must be correct up to the null terminator. Otherwise it's UB for some str functions. However this check is hard to enable in real code, a lot code call str functions on non null terminated strings.

So the patch as-is is essentially: COMMON_INTERCEPTOR_READ_RANGE(ctx, src, 0); which is NOOP

COMMON_INTERCEPTOR_READ_STRING(ctx, src, internal_strlen(src) + 1); can work here, but it's equivalent to
COMMON_INTERCEPTOR_READ_RANGE(ctx, src, internal_strlen(src) + 1);

2518

is "res > targsize" possible?
also this one does not set trailing \0, so no +1

Same as above (res < targsize) seem unnecessary
if (res + 1) is overflow REAL() must return -1

2518

Unit test fails without the '+ 1'.

Similar to above, check is to prevent writing beyond end of target.

https://elixir.bootlin.com/glibc/glibc-2.17/source/resolv/base64.c#L248
You are correct, according to the code, the last written char is target[res], so should be res + 1,

compiler-rt/test/msan/Linux/b64.cpp

4

yes, and drop CHECK: WARNING stuff as we can't run different sanitizers there
also you can add some corner-case we discussing above

13

there is a leak of dst

char dst[dst_len]; ?

[kda]

improve unit test (reading and edge cases), add sanitizer_common test, adjust implementation.

[kda]

a few more adjustments

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

2518

PTAL: I think I was seeing a side effect for 'strlen'. I think you were right the first time.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

2513

it's null-terminated string so +1

2518

I see, it writes past "res" in some cases, but it's not considered a part of output
so no +1 here.

[kda]

check trailing null byte of src parameter to pton.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

INTERCEPTOR(int, glob64, const char *pattern, int flags,
   return res;
 }
 #define INIT_GLOB64                \
   COMMON_INTERCEPT_FUNCTION(glob64);
 #else  // SANITIZER_INTERCEPT_GLOB64
 #define INIT_GLOB64
 #endif  // SANITIZER_INTERCEPT_GLOB64
 
+#if SANITIZER_INTERCEPT___B64_TO
+INTERCEPTOR(int, __b64_ntop, unsigned char const *src, SIZE_T srclength,
+            char *target, SIZE_T targsize) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, __b64_ntop, src, srclength, target, targsize);
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, src, srclength);
+  int res = REAL(__b64_ntop)(src, srclength, target, targsize);
+  if (res >= 0)
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, target, res + 1);
+  return res;
+}
+INTERCEPTOR(int, __b64_pton, char const *src, char *target, SIZE_T targsize) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, __b64_pton, src, target, targsize);
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, src, internal_strlen(src) + 1);
+  int res = REAL(__b64_pton)(src, target, targsize);
+  if (res >= 0)
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, target, res);
+  return res;
+}
+#  define INIT___B64_TO                    \
+    COMMON_INTERCEPT_FUNCTION(__b64_ntop); \
+    COMMON_INTERCEPT_FUNCTION(__b64_pton);
+#else  // SANITIZER_INTERCEPT___B64_TO
+#define INIT___B64_TO
+#endif  // SANITIZER_INTERCEPT___B64_TO
+
+
 #if SANITIZER_INTERCEPT_POSIX_SPAWN
 
 template <class RealSpawnPtr>
 static int PosixSpawnImpl(void *ctx, RealSpawnPtr *real_posix_spawn, pid_t *pid,
                           const char *file_or_path, const void *file_actions,
                           const void *attrp, char *const argv[],
                           char *const envp[]) {
   COMMON_INTERCEPTOR_READ_RANGE(ctx, file_or_path,
#endif
   INIT_FGETGRENT_R;
   INIT_SETPWENT;
   INIT_CLOCK_GETTIME;
   INIT_CLOCK_GETCPUCLOCKID;
   INIT_GETITIMER;
   INIT_TIME;
   INIT_GLOB;
   INIT_GLOB64;
+  INIT___B64_TO;
   INIT_POSIX_SPAWN;
   INIT_WAIT;
   INIT_WAIT4;
   INIT_INET;
   INIT_PTHREAD_GETSCHEDPARAM;
   INIT_GETADDRINFO;
   INIT_GETNAMEINFO;
   INIT_GETSOCKNAME;

compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h

 #define SANITIZER_INTERCEPT_CLOCK_GETTIME \
   (SI_FREEBSD || SI_NETBSD || SI_LINUX || SI_SOLARIS)
 #define SANITIZER_INTERCEPT_CLOCK_GETCPUCLOCKID \
   (SI_LINUX || SI_FREEBSD || SI_NETBSD)
 #define SANITIZER_INTERCEPT_GETITIMER SI_POSIX
 #define SANITIZER_INTERCEPT_TIME SI_POSIX
 #define SANITIZER_INTERCEPT_GLOB (SI_GLIBC || SI_SOLARIS)
 #define SANITIZER_INTERCEPT_GLOB64 SI_GLIBC
+#define SANITIZER_INTERCEPT___B64_TO SI_LINUX
 #define SANITIZER_INTERCEPT_POSIX_SPAWN SI_POSIX
 #define SANITIZER_INTERCEPT_WAIT SI_POSIX
 #define SANITIZER_INTERCEPT_INET SI_POSIX
 #define SANITIZER_INTERCEPT_PTHREAD_GETSCHEDPARAM SI_POSIX
 #define SANITIZER_INTERCEPT_GETADDRINFO SI_POSIX
 #define SANITIZER_INTERCEPT_GETNAMEINFO SI_POSIX
 #define SANITIZER_INTERCEPT_GETSOCKNAME SI_POSIX
 #define SANITIZER_INTERCEPT_GETHOSTBYNAME SI_POSIX

compiler-rt/test/msan/Linux/b64.cpp

+// RUN: %clangxx_msan -O0 %s -o %t -lresolv && %run %t
+// RUN: not %run %t NTOP_READ 2>&1 | FileCheck %s --check-prefix=NTOP_READ
+// RUN: not %run %t PTON_READ 2>&1 | FileCheck %s --check-prefix=PTON_READ
+
+#include <assert.h>
+#include <resolv.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+
+#include <sanitizer/msan_interface.h>
+
+int main(int iArgc, char *szArgv[]) {
+  char* test = nullptr;
+  if (iArgc > 1) {
+    test = szArgv[1];
+  }
+
+  if (test == nullptr) {
+    // Check NTOP writing
+    const char *src = "base64 test data";
+    size_t src_len = strlen(src);
+    size_t dst_len = ((src_len + 2) / 3) * 4 + 1;
+    char dst[dst_len];
+    int res = b64_ntop(reinterpret_cast<const unsigned char *>(src), src_len,
+                       dst, dst_len);
+    assert(res >= 0);
+    __msan_check_mem_is_initialized(dst, res);
+
+    // Check PTON writing
+    unsigned char target[dst_len];
+    res = b64_pton(dst, target, dst_len);
+    assert(res >= 0);
+    __msan_check_mem_is_initialized(target, res);
+
+    // Check NTOP writing for zero length src
+    src = "";
+    src_len = strlen(src);
+    assert(((src_len + 2) / 3) * 4 + 1 < dst_len);
+    res = b64_ntop(reinterpret_cast<const unsigned char *>(src), src_len,
+                       dst, dst_len);
+    assert(res >= 0);
+    __msan_check_mem_is_initialized(dst, res);
+
+    // Check PTON writing for zero length src
+    dst[0] = '\0';
+    res = b64_pton(dst, target, dst_len);
+    assert(res >= 0);
+    __msan_check_mem_is_initialized(target, res);
+
+    return 0;
+  }
+
+  if (strcmp(test, "NTOP_READ") == 0) {
+    // Check NTOP reading
+    size_t src_len = 80;
+    char src[src_len];
+    __msan_poison(src, src_len);
+    size_t dst_len = ((src_len + 2) / 3) * 4 + 1;
+    char dst[dst_len];
+    int res = b64_ntop(reinterpret_cast<const unsigned char *>(src), src_len,
+                       dst, dst_len);
+    // NTOP_READ: Uninitialized bytes in __interceptor___b64_ntop
+    return 0;
+  }
+
+  if (strcmp(test, "PTON_READ") == 0) {
+    // Check PTON reading
+    size_t src_len = 80;
+    char src[src_len];
+    strcpy(src, "junk longer than zero");
+    // pretend it is uninitialized
+    __msan_poison(src, src_len);
+    unsigned char target[src_len];
+    int res = b64_pton(src, target, src_len);
+    // PTON_READ: Uninitialized bytes in __interceptor___b64_pton
+    return 0;
+  }
+
+  return 0;
+}

compiler-rt/test/sanitizer_common/TestCases/Linux/b64.cpp

+// RUN: %clangxx %s -o %t -lresolv && %run %t %p
+
+#include <assert.h>
+#include <resolv.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+
+int main(int iArgc, char *szArgv[]) {
+  // Check NTOP writing
+  const char *src = "base64 test data";
+  size_t src_len = strlen(src);
+  size_t dst_len = ((src_len + 2) / 3) * 4 + 1;
+  char dst[dst_len];
+  int res = b64_ntop(reinterpret_cast<const unsigned char *>(src), src_len, dst,
+                     dst_len);
+  assert(res >= 0);
+  assert(strcmp(dst, "YmFzZTY0IHRlc3QgZGF0YQ==") == 0);
+
+  // Check PTON writing
+  unsigned char target[dst_len];
+  res = b64_pton(dst, target, dst_len);
+  assert(res >= 0);
+  assert(strncmp(reinterpret_cast<const char *>(target), src, res) == 0);
+
+  // Check NTOP writing for zero length src
+  src = "";
+  src_len = strlen(src);
+  assert(((src_len + 2) / 3) * 4 + 1 < dst_len);
+  res = b64_ntop(reinterpret_cast<const unsigned char *>(src), src_len, dst,
+                 dst_len);
+  assert(res >= 0);
+  assert(strcmp(dst, "") == 0);
+
+  // Check PTON writing for zero length src
+  dst[0] = '\0';
+  res = b64_pton(dst, target, dst_len);
+  assert(res >= 0);
+  assert(strncmp(reinterpret_cast<const char *>(target), src, res) == 0);
+
+  return 0;
+}