CVE-2016-6873: Other (NVD-CWE-Other)
value: Self recursion in compact in Facebook HHVM before 3.15.0 allows attackers to have unspecified impact via unknown vectors.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 9.8
Published: 2017-02-17T17:59Z
Repository: https://github.com/facebook/hhvm
Language: PHP
Files changed: hphp/runtime/ext/array/ext_array.cpp, hphp/test/slow/array_functions/self_recursive.php, hphp/test/slow/array_functions/self_recursive.php.expectf
Commit date: 2016-08-01 15:48:44 -0700
Commit message: Fix self recursion in compact

Summary: There were no checks at all.

Reviewed By: alexmalyshev

Differential Revision: D3623763

fbshipit-source-id: 9d708deca05bbd121503e8f323b4f295fde8e835

Diff: diff --git a/hphp/runtime/ext/array/ext_array.cpp b/hphp/runtime/ext/array/ext_array.cpp
index d702c9831c6..7b3429a498b 100644
--- a/hphp/runtime/ext/array/ext_array.cpp
+++ b/hphp/runtime/ext/array/ext_array.cpp
@@ -377,14 +377,14 @@ Variant HHVM_FUNCTION(array_keys, int64_t argc,
 
 static void php_array_merge(Array &arr1, const Array& arr2) {
   arr1.merge(arr2);
 }
 
-static bool couldRecur(const Variant& v, const Array& arr) {
+static bool couldRecur(const Variant& v, const ArrayData* arr) {
   return v.isReferenced() ||
-    arr.get()->kind() == ArrayData::kGlobalsKind ||
-    arr.get()->kind() == ArrayData::kProxyKind;
+    arr->kind() == ArrayData::kGlobalsKind ||
+    arr->kind() == ArrayData::kProxyKind;
 }
 
 static void php_array_merge_recursive(PointerSet &seen, bool check,
                                       Array &arr1, const Array& arr2) {
   if (check && !seen.insert((void*)arr1.get()).second) {
@@ -401,11 +401,11 @@ static void php_array_merge_recursive(PointerSet &seen, bool check,
       // There is no need to do toKey() conversion, for a key that is already
       // in the array.
       Variant &v = arr1.lvalAt(key, AccessFlags::Key);
       auto subarr1 = v.toArray().copy();
       php_array_merge_recursive(seen,
-                                couldRecur(v, subarr1),
+                                couldRecur(v, subarr1.get()),
                                 subarr1,
                                 value.toArray());
       v.unset(); // avoid contamination of the value that was strongly bound
       v = subarr1;
     } else {
@@ -591,11 +591,11 @@ static void php_array_replace_recursive(PointerSet &seen, bool check,
     if (arr1.exists(key, true) && value.isArray()) {
       Variant &v = arr1.lvalAt(key, AccessFlags::Key);
       if (v.isArray()) {
         Array subarr1 = v.toArray();
         const ArrNR& arr_value = value.toArrNR();
-        php_array_replace_recursive(seen, couldRecur(v, subarr1),
+        php_array_replace_recursive(seen, couldRecur(v, subarr1.get()),
                                     subarr1, arr_value);
         v = subarr1;
       } else {
         arr1.set(key, value, true);
       }
@@ -1215,15 +1215,23 @@ bool HHVM_FUNCTION(array_walk,
   Variant var(input, Variant::WithRefBind{});
   ArrayUtil::Walk(var, walk_func, &ctx, false, NULL, userdata);
   return true;
 }
 
-static void compact(VarEnv* v, Array &ret, const Variant& var) {
+static void compact(PointerSet& seen,
+                    VarEnv* v, Array &ret, const Variant& var) {
   if (var.isArray()) {
-    for (ArrayIter iter(var.getArrayData()); iter; ++iter) {
-      compact(v, ret, iter.second());
+    auto adata = var.getArrayData();
+    auto check = couldRecur(var, adata);
+    if (check && !seen.insert(adata).second) {
+      raise_warning("compact(): recursion detected");
+      return;
     }
+    for (ArrayIter iter(adata); iter; ++iter) {
+      compact(seen, v, ret, iter.secondRef());
+    }
+    if (check) seen.erase(adata);
   } else {
     String varname = var.toString();
     if (!varname.empty() && v->lookup(varname.get()) != NULL) {
       ret.set(varname, *reinterpret_cast<Variant*>(v->lookup(varname.get())));
     }
@@ -1235,12 +1243,13 @@ Array HHVM_FUNCTION(compact,
                     const Array& args /* = null array */) {
   raise_disallowed_dynamic_call("compact should not be called dynamically");
   Array ret = Array::attach(PackedArray::MakeReserve(args.size() + 1));
   VarEnv* v = g_context->getOrCreateVarEnv();
   if (v) {
-    compact(v, ret, varname);
-    compact(v, ret, args);
+    PointerSet seen;
+    compact(seen, v, ret, varname);
+    if (!args.empty()) compact(seen, v, ret, args);
   }
   return ret;
 }
 
 // __SystemLib\\compact_sl
@@ -1248,12 +1257,13 @@ Array HHVM_FUNCTION(__SystemLib_compact_sl,
                     const Variant& varname,
                     const Array& args /* = null array */) {
   Array ret = Array::attach(PackedArray::MakeReserve(args.size() + 1));
   VarEnv* v = g_context->getOrCreateVarEnv();
   if (v) {
-    compact(v, ret, varname);
-    compact(v, ret, args);
+    PointerSet seen;
+    compact(seen, v, ret, varname);
+    if (!args.empty()) compact(seen, v, ret, args);
   }
   return ret;
 }
 
 static int php_count_recursive(const Array& array) {
diff --git a/hphp/test/slow/array_functions/self_recursive.php b/hphp/test/slow/array_functions/self_recursive.php
index 09920767439..5eb3708be3a 100644
--- a/hphp/test/slow/array_functions/self_recursive.php
+++ b/hphp/test/slow/array_functions/self_recursive.php
@@ -13,8 +13,9 @@ function main() {
   $a = array();
   $a['g'] = &$a;
 
   test($a);
   test($GLOBALS);
+  var_dump(compact($a));
 }
 
 main();
diff --git a/hphp/test/slow/array_functions/self_recursive.php.expectf b/hphp/test/slow/array_functions/self_recursive.php.expectf
index 0a3eff14b49..c378aa8baaf 100644
--- a/hphp/test/slow/array_functions/self_recursive.php.expectf
+++ b/hphp/test/slow/array_functions/self_recursive.php.expectf
@@ -1,6 +1,5 @@
-
 Warning: array_replace_recursive(): recursion detected in %s/test/slow/array_functions/self_recursive.php on line 6
 
 Warning: array_merge_recursive(): recursion detected in %s/test/slow/array_functions/self_recursive.php on line 9
 
 Warning: array_merge_recursive(): recursion detected in %s/test/slow/array_functions/self_recursive.php on line 9
@@ -8,5 +7,9 @@ Warning: array_merge_recursive(): recursion detected in %s/test/slow/array_funct
 Warning: array_merge_recursive(): recursion detected in %s/test/slow/array_functions/self_recursive.php on line 9
 
 Warning: array_merge_recursive(): recursion detected in %s/test/slow/array_functions/self_recursive.php on line 9
 
 Warning: array_merge_recursive(): recursion detected in %s/test/slow/array_functions/self_recursive.php on line 9
+
+Warning: compact(): recursion detected in %s/test/slow/array_functions/self_recursive.php on line 18
+array(0) {
+}
\ No newline at end of file

Vulnerable code: static bool couldRecur(const Variant& v, const Array& arr) {
    arr.get()->kind() == ArrayData::kGlobalsKind ||
    arr.get()->kind() == ArrayData::kProxyKind;
                                couldRecur(v, subarr1),
        php_array_replace_recursive(seen, couldRecur(v, subarr1),
static void compact(VarEnv* v, Array &ret, const Variant& var) {
    for (ArrayIter iter(var.getArrayData()); iter; ++iter) {
      compact(v, ret, iter.second());
    compact(v, ret, varname);
    compact(v, ret, args);
    compact(v, ret, varname);
    compact(v, ret, args);

Fixed code: static bool couldRecur(const Variant& v, const ArrayData* arr) {
    arr->kind() == ArrayData::kGlobalsKind ||
    arr->kind() == ArrayData::kProxyKind;
                                couldRecur(v, subarr1.get()),
        php_array_replace_recursive(seen, couldRecur(v, subarr1.get()),
static void compact(PointerSet& seen,
                    VarEnv* v, Array &ret, const Variant& var) {
    auto adata = var.getArrayData();
    auto check = couldRecur(var, adata);
    if (check && !seen.insert(adata).second) {
      raise_warning("compact(): recursion detected");
      return;
    for (ArrayIter iter(adata); iter; ++iter) {
      compact(seen, v, ret, iter.secondRef());
    }
    if (check) seen.erase(adata);
    PointerSet seen;
    compact(seen, v, ret, varname);
    if (!args.empty()) compact(seen, v, ret, args);
    PointerSet seen;
    compact(seen, v, ret, varname);
    if (!args.empty()) compact(seen, v, ret, args);
  var_dump(compact($a));

Warning: compact(): recursion detected in %s/test/slow/array_functions/self_recursive.php on line 18
array(0) {
}