CVE-2019-11936: Insufficient Information (NVD-CWE-noinfo)
value: Various APC functions accept keys containing null bytes as input, leading to premature truncation of input. This issue affects HHVM versions prior to 3.30.12, all versions between 4.0.0 and 4.8.5, all versions between 4.9.0 and 4.23.1, as well as 4.24.0, 4.25.0, 4.26.0, 4.27.0, 4.28.0, and 4.28.1.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 9.8
Published: 2019-12-04T17:16Z
Repository: https://github.com/facebook/hhvm
Language: PHP
Files changed: hphp/runtime/ext/apc/ext_apc.cpp, hphp/test/quick/apc.php, hphp/test/quick/apc.php.expectf
Commit date: 2019-10-28 12:53:54 -0700
Commit message: Prevent APC keys with nulls

Summary:
This diff prevents storing APC keys that contain the null byte.
APC uses a backing data structure ConcurrentTableSharedStore that uses const char* as its key type. This leads to situations where inserting APC keys with null bytes will cause key truncation, like so...
localhost> p apc_clear_cache();
true
localhost> p apc_fetch("test")
false
localhost> p apc_fetch("test\x00suffix")
false
localhost> p apc_add("test\x00suffix", 5)
true
localhost> p apc_fetch("test")
5
localhost> p apc_fetch("test\x00suffix")
false
This change will make APC throw an error if a key containing a null byte is passed to the store/add methods.

Reviewed By: markw65

Differential Revision: D17608626

fbshipit-source-id: 915e179f41e66c99c718364ec4a8639d204c4ea2

Diff: diff --git a/hphp/runtime/ext/apc/ext_apc.cpp b/hphp/runtime/ext/apc/ext_apc.cpp
index 482275b636fd..97b9678b9b8b 100644
--- a/hphp/runtime/ext/apc/ext_apc.cpp
+++ b/hphp/runtime/ext/apc/ext_apc.cpp
@@ -93,10 +93,15 @@ ConcurrentTableSharedStore& apc_store() {
   }
   void* vpStore = &s_apc_storage;
   return *static_cast<ConcurrentTableSharedStore*>(vpStore);
 }
 
+bool isKeyInvalid(const String &key) {
+  // T39154441 - check if invalid chars exist
+  return key.find('\0') != -1;
+}
+
 }
 
 //////////////////////////////////////////////////////////////////////
 
 void initialize_apc() {
@@ -305,21 +310,31 @@ Variant HHVM_FUNCTION(apc_store,
       if (!key.isString()) {
         throw_invalid_argument("apc key: (not a string)");
         return Variant(false);
       }
       Variant v = iter.second();
-      apc_store().set(key.toString(), v, ttl);
-    }
 
+      auto const& strKey = key.toCStrRef();
+      if (isKeyInvalid(strKey)) {
+        throw_invalid_argument("apc key: (contains invalid characters)");
+        return Variant(false);
+      }
+      apc_store().set(strKey, v, ttl);
+    }
     return Variant(ArrayData::Create());
   }
 
   if (!key_or_array.isString()) {
     throw_invalid_argument("apc key: (not a string)");
     return Variant(false);
   }
   String strKey = key_or_array.toString();
+
+  if (isKeyInvalid(strKey)) {
+    throw_invalid_argument("apc key: (contains invalid characters)");
+    return Variant(false);
+  }
   apc_store().set(strKey, var, ttl);
   return Variant(true);
 }
 
 /**
@@ -328,10 +343,14 @@ Variant HHVM_FUNCTION(apc_store,
  */
 bool HHVM_FUNCTION(apc_store_as_primed_do_not_use,
                    const String& key,
                    const Variant& var) {
   if (!apcExtension::Enable) return false;
+  if (isKeyInvalid(key)) {
+    throw_invalid_argument("apc key: (contains invalid characters)");
+    return false;
+  }
   apc_store().setWithoutTTL(key, var);
   return true;
 }
 
 Variant HHVM_FUNCTION(apc_add,
@@ -351,22 +370,33 @@ Variant HHVM_FUNCTION(apc_add,
       if (!key.isString()) {
         throw_invalid_argument("apc key: (not a string)");
         return false;
       }
       Variant v = iter.second();
-      if (!apc_store().add(key.toString(), v, ttl)) {
-        errors.add(key, -1);
+
+      auto const& strKey = key.toCStrRef();
+      if (isKeyInvalid(strKey)) {
+        throw_invalid_argument("apc key: (contains invalid characters)");
+        return false;
+      }
+
+      if (!apc_store().add(strKey, v, ttl)) {
+        errors.add(strKey, -1);
       }
     }
     return errors.toVariant();
   }
 
   if (!key_or_array.isString()) {
     throw_invalid_argument("apc key: (not a string)");
     return false;
   }
   String strKey = key_or_array.toString();
+  if (isKeyInvalid(strKey)) {
+    throw_invalid_argument("apc key: (contains invalid characters)");
+    return false;
+  }
   return apc_store().add(strKey, var, ttl);
 }
 
 TypedValue HHVM_FUNCTION(apc_fetch, const Variant& key, bool& success) {
   if (!apcExtension::Enable) return make_tv<KindOfBoolean>(false);
diff --git a/hphp/test/quick/apc.php b/hphp/test/quick/apc.php
index adf16fc5cc28..0fa55bc07c4a 100644
--- a/hphp/test/quick/apc.php
+++ b/hphp/test/quick/apc.php
@@ -59,10 +59,20 @@ function testKeyTypes() {
   foreach (array(2, 3, '2', '3') as $k) {
     try { var_dump($arr[$k]); } catch (Exception $e) { echo $e->getMessage()."\n"; }
   }
 }
 
+function testInvalidKeys() {
+    // Reject keys with null bytes
+    apc_add("bar\x00baz", 10);
+    apc_store("test\x00xyz", "hello");
+    apc_store(array("validkey" => "validvalue", "invalid\x00key" => "value"));
+    foreach (array('bar', 'test', 'validkey', 'invalid') as $k) {
+        var_dump(__hhvm_intrinsics\apc_fetch_no_check($k));
+    }
+}
+
 <<__EntryPoint>> function main(): void {
   testApc(array(7, 4, 1776));
   testApc(array("sv0", "sv1"));
   testApc(array("sk0" => "sv0", "sk1" => "sv1"));
 
@@ -71,6 +81,7 @@ function testKeyTypes() {
   $a = __hhvm_intrinsics\apc_fetch_no_check('foo');
   $b = call_user_func_array(fun("strtoupper"), $a);
   var_dump($b);
 
   testKeyTypes();
+  testInvalidKeys();
 }
diff --git a/hphp/test/quick/apc.php.expectf b/hphp/test/quick/apc.php.expectf
index 8a126229e691..5e7e322bcb6f 100644
--- a/hphp/test/quick/apc.php.expectf
+++ b/hphp/test/quick/apc.php.expectf
@@ -70,5 +70,15 @@ array(0) {
 string(1) "A"
 string(3) "two"
 Undefined index: 3
 Undefined index: 2
 string(5) "three"
+
+Warning: Invalid argument: apc key: (contains invalid characters)%S
+
+Warning: Invalid argument: apc key: (contains invalid characters)%S
+
+Warning: Invalid argument: apc key: (contains invalid characters)%S
+bool(false)
+bool(false)
+string(10) "validvalue"
+bool(false)

Vulnerable code:       apc_store().set(key.toString(), v, ttl);
    }
      if (!apc_store().add(key.toString(), v, ttl)) {
        errors.add(key, -1);
Fixed code: bool isKeyInvalid(const String &key) {
  // T39154441 - check if invalid chars exist
  return key.find('\0') != -1;
}

      auto const& strKey = key.toCStrRef();
      if (isKeyInvalid(strKey)) {
        throw_invalid_argument("apc key: (contains invalid characters)");
        return Variant(false);
      }
      apc_store().set(strKey, v, ttl);
    }

  if (isKeyInvalid(strKey)) {
    throw_invalid_argument("apc key: (contains invalid characters)");
    return Variant(false);
  }
  if (isKeyInvalid(key)) {
    throw_invalid_argument("apc key: (contains invalid characters)");
    return false;
  }

      auto const& strKey = key.toCStrRef();
      if (isKeyInvalid(strKey)) {
        throw_invalid_argument("apc key: (contains invalid characters)");
        return false;
      }

      if (!apc_store().add(strKey, v, ttl)) {
        errors.add(strKey, -1);
  if (isKeyInvalid(strKey)) {
    throw_invalid_argument("apc key: (contains invalid characters)");
    return false;
  }
function testInvalidKeys() {
    // Reject keys with null bytes
    apc_add("bar\x00baz", 10);
    apc_store("test\x00xyz", "hello");
    apc_store(array("validkey" => "validvalue", "invalid\x00key" => "value"));
    foreach (array('bar', 'test', 'validkey', 'invalid') as $k) {
        var_dump(__hhvm_intrinsics\apc_fetch_no_check($k));
    }
}

  testInvalidKeys();

Warning: Invalid argument: apc key: (contains invalid characters)%S

Warning: Invalid argument: apc key: (contains invalid characters)%S

Warning: Invalid argument: apc key: (contains invalid characters)%S
bool(false)
bool(false)
string(10) "validvalue"
bool(false)