[Sanitizers][Darwin] Intercept CCRandomGenerateBytes

[devnexen]

Summary

• Intercept the native apple (both iOs and macOS) which is even allowed on App Store (contrary to getentropy for example), thus checking its proper usage in code consumers.

[yln]

Hi David, thanks for the patch for this previously unintercepted API. I am happy with the interceptor implementation itself; it follows our "recipe" nicely! :)

I have two small asks:

• In the commit message, can you explain which issue (crash, false positive) for which sanitizer this is addressing?
• Can we write a test that shows this issue? I think the current test only shows that we don't crash, right?

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

7246

Nitpick: can we use the parameter names "bytes" and "count" from the header declaration?

CCRNGStatus CCRandomGenerateBytes(void *bytes, size_t count)
__OSX_AVAILABLE_STARTING(__MAC_10_10, __IPHONE_8_0);

compiler-rt/test/sanitizer_common/TestCases/Darwin/ccrandomgeneratebytes.c

1

Are we testing here simply that we don't crash in our interceptor? Can we add a comment what we intend to test here.

4

I don't think this will work as intended. We would try to compile and run an empty file.
However, I think no one is actually testing on macOS version < 10.10, so just omitting this guard should be fine.

12

I don't think we will ever return "false", i.e., failing the test here?

success -> n = 0
failure -> n = 1
return n <= 0   // n is never <= 0, right?!

Maybe just return the return value of CCRandomGenerateBytes() (assuming kCCSuccess is 0)?

[devnexen]

Update unit test case.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

9836–9845

Why this renaming? As it it matches https://man7.org/linux/man-pages/man2/getrandom.2.html
Either way it looks unrelated to the patch.

[vitalybuka]

It's titled "Intercept CCRandomGenerateBytes", but I don't see changes in interceptors, only new test.

[devnexen]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

9836–9845

My bad wrong lines.

[yln]

Hi David,

Before I approve this can we do the following?

• In the commit message, can you explain which issue (crash, false positive) for which sanitizer this is addressing?
• Can we write a test that shows this issue, i.e., a test that fails without the added interceptor? (Maybe this test needs to be for a specific sanitizer instead of sanitizer_common?)

Note: my understanding is that the added test is "green" even without the new interceptor. Ignore my second point if that is not the case.

[devnexen]

you re right after second thoughts it does not bring anything on the table.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

   return res;
 }
 
 #define INIT_READLINKAT COMMON_INTERCEPT_FUNCTION(readlinkat)
 #else
 #define INIT_READLINKAT
 #endif
 
+#if SANITIZER_INTERCEPT_CCRANDOM_GENERATE_BYTES
+INTERCEPTOR(int, CCRandomGenerateBytes, void *buf, SIZE_T buflen) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, CCRandomGenerateBytes, buf, buflen);
+  int ret = REAL(CCRandomGenerateBytes)(buf, buflen);
+  if (ret == 0) {
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
+  }
+  return ret;
+}
+#define INIT_CCRANDOM_GENERATE_BYTES COMMON_INTERCEPT_FUNCTION(CCRandomGenerateBytes);
+#else
+#define INIT_CCRANDOM_GENERATE_BYTES
+#endif
+
 #if SANITIZER_INTERCEPT_NAME_TO_HANDLE_AT
 INTERCEPTOR(int, name_to_handle_at, int dirfd, const char *pathname,
             struct file_handle *handle, int *mount_id, int flags) {
   void* ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, name_to_handle_at, dirfd, pathname, handle,
                            mount_id, flags);
   COMMON_INTERCEPTOR_READ_RANGE(ctx, pathname, REAL(strlen)(pathname) + 1);
 

   COMMON_INTERCEPT_FUNCTION(sl_init); \
   COMMON_INTERCEPT_FUNCTION(sl_add);  \
   COMMON_INTERCEPT_FUNCTION(sl_find); \
   COMMON_INTERCEPT_FUNCTION(sl_free);
 #else
 #define INIT_SL_INIT
 #endif
 
 #if SANITIZER_INTERCEPT_GETRANDOM
 INTERCEPTOR(SSIZE_T, getrandom, void *buf, SIZE_T buflen, unsigned int flags) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, getrandom, buf, buflen, flags);
   SSIZE_T n = REAL(getrandom)(buf, buflen, flags);
   if (n > 0) {
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, n);
   }
   return n;
 }
 #define INIT_GETRANDOM COMMON_INTERCEPT_FUNCTION(getrandom)
 #else
 #define INIT_GETRANDOM
 #endif
 
 #if SANITIZER_INTERCEPT_CRYPT
 INTERCEPTOR(char *, crypt, char *key, char *salt) {
   void *ctx;

   INIT_GROUP_FROM_GID;
   INIT_GID_FROM_GROUP;
   INIT_ACCESS;
   INIT_FACCESSAT;
   INIT_GETGROUPLIST;
   INIT_GETGROUPMEMBERSHIP;
   INIT_READLINK;
   INIT_READLINKAT;
+  INIT_CCRANDOM_GENERATE_BYTES;
   INIT_NAME_TO_HANDLE_AT;
   INIT_OPEN_BY_HANDLE_AT;
   INIT_STRLCPY;
   INIT_DEVNAME;
   INIT_DEVNAME_R;
   INIT_FGETLN;
   INIT_STRMODE;
   INIT_TTYENT;

compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h

 #if defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && \
     __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 101000
 #define SI_MAC_DEPLOYMENT_BELOW_10_10 1
 #else
 #define SI_MAC_DEPLOYMENT_BELOW_10_10 0
 #endif
 #define SANITIZER_INTERCEPT_READLINKAT \
   (SI_POSIX && !SI_MAC_DEPLOYMENT_BELOW_10_10)
+#define SANITIZER_INTERCEPT_CCRANDOM_GENERATE_BYTES \
+  (SI_MAC && !SI_MAC_DEPLOYMENT_BELOW_10_10)
 
 #define SANITIZER_INTERCEPT_DEVNAME (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_DEVNAME_R (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_FGETLN (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_STRMODE (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_TTYENT SI_NETBSD
 #define SANITIZER_INTERCEPT_PROTOENT (SI_NETBSD || SI_LINUX)
 #define SANITIZER_INTERCEPT_PROTOENT_R SI_GLIBC

compiler-rt/test/sanitizer_common/TestCases/Darwin/ccrandomgeneratebytes.c

+// RUN: %clang -O2 %s -o %t && %run %t
+
+#include <Availability.h>
+#if __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ > 101000
+#include <CommonCrypto/CommonRandom.h>
+
+int main() {
+  char buf[16];
+  int n = 0;
+  if (CCRandomGenerateBytes(buf, sizeof(buf)) != kCCSuccess)
+    n = 1;
+  return n <= 0;
+}
+#endif