CVE-2016-6870: Out-of-bounds Write (CWE-787)
value: Out-of-bounds write in the (1) mb_detect_encoding, (2) mb_send_mail, and (3) mb_detect_order functions in Facebook HHVM before 3.15.0 allows attackers to have unspecified impact via unknown vectors.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2017-02-17T17:59Z
Repository: https://github.com/facebook/hhvm
Language: PHP
Files changed: hphp/runtime/ext/mbstring/ext_mbstring.cpp, hphp/test/quick/mbstring-oob.php
Commit date: 2016-07-01 10:05:03 -0700
Commit message: Use req::strndup in php_mb_parse_encoding_list to prevent oob memory write.

Summary:
Fix out of bounds write access in mb_detect_encoding.  Using strndup in
php_mb_parse_encoding_list will cause strings with embedded nulls to be
unexpectedly shortened.  The expected length of the string is tracked in
value_length but since strndup may copy fewer characters when there are
mbedded null this can lead to oob writes into tmpstr.

I've found a couple other places in this file that use strndup and replaced
them with req::strndup as well.  The use of strndup in mb_send_mail also seemed
to be a leak.

This replaces uses of strndup with req::strndup which can handle embedded
nulls properly.

It looks like I also accidentally fixed t11337047 at the same time.  Adding it to the list of tasks.

Reviewed By: paulbiss

Differential Revision: D3360065

fbshipit-source-id: 99776cf9105e3789883380bf30240009eec52cec

Diff: diff --git a/hphp/runtime/ext/mbstring/ext_mbstring.cpp b/hphp/runtime/ext/mbstring/ext_mbstring.cpp
index d7f4d56d656..869703d84fa 100644
--- a/hphp/runtime/ext/mbstring/ext_mbstring.cpp
+++ b/hphp/runtime/ext/mbstring/ext_mbstring.cpp
@@ -552,15 +552,15 @@ static int php_mb_parse_encoding_list(const char *value, int value_length,
     identify_list = MBSTRG(default_detect_order_list);
     identify_list_size = MBSTRG(default_detect_order_list_size);
 
     /* copy the value string for work */
     if (value[0]=='"' && value[value_length-1]=='"' && value_length>2) {
-      tmpstr = (char *)strndup(value+1, value_length-2);
+      tmpstr = req::strndup(value + 1, value_length - 2);
       value_length -= 2;
+    } else {
+      tmpstr = req::strndup(value, value_length);
     }
-    else
-      tmpstr = (char *)strndup(value, value_length);
     if (tmpstr == nullptr) {
       return 0;
     }
     /* count the number of listed encoding names */
     endp = tmpstr + value_length;
@@ -638,11 +638,11 @@ static int php_mb_parse_encoding_list(const char *value, int value_length,
       if (return_size) {
         *return_size = 0;
       }
       ret = 0;
     }
-    free(tmpstr);
+    req::free(tmpstr);
   }
 
   return ret;
 }
 
@@ -2252,15 +2252,15 @@ bool HHVM_FUNCTION(mb_parse_str,
   info.to_language            = MBSTRG(current_language);
   info.from_encodings         = MBSTRG(http_input_list);
   info.num_from_encodings     = MBSTRG(http_input_list_size);
   info.from_language          = MBSTRG(current_language);
 
-  char *encstr = strndup(encoded_string.data(), encoded_string.size());
+  char *encstr = req::strndup(encoded_string.data(), encoded_string.size());
   Array resultArr = Array::Create();
   mbfl_encoding *detected =
     _php_mb_encoding_handler_ex(&info, resultArr, encstr);
-  free(encstr);
+  req::free(encstr);
   result.assignIfRef(resultArr);
 
   MBSTRG(http_input_identify) = detected;
   return detected != nullptr;
 }
@@ -4249,11 +4249,11 @@ bool HHVM_FUNCTION(mb_send_mail,
   char *to_r = nullptr;
   int err = 0;
   if (!to.empty()) {
     int to_len = to.size();
     if (to_len > 0) {
-      to_r = strndup(to.data(), to_len);
+      to_r = req::strndup(to.data(), to_len);
       for (; to_len; to_len--) {
         if (!isspace((unsigned char)to_r[to_len - 1])) {
           break;
         }
         to_r[to_len - 1] = '\0';
@@ -4396,10 +4396,13 @@ bool HHVM_FUNCTION(mb_send_mail,
   String cmd = string_escape_shell_cmd(extra_cmd.c_str());
   bool ret = (!err && php_mail(to_r, encoded_subject.data(),
                                encoded_message.data(),
                                all_headers, cmd.data()));
   mbfl_memory_device_clear(&device);
+  if (to_r != to.data()) {
+    req::free(to_r);
+  }
   return ret;
 }
 
 static struct mbstringExtension final : Extension {
   mbstringExtension() : Extension("mbstring", NO_EXTENSION_VERSION_YET) {}
diff --git a/hphp/test/quick/mbstring-oob.php b/hphp/test/quick/mbstring-oob.php
new file mode 100644
index 00000000000..5640beb5f9b
--- /dev/null
+++ b/hphp/test/quick/mbstring-oob.php
@@ -0,0 +1,6 @@
+<?hh // strict
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+$var0 = null;
+$var3 = "\x00";
+$var3 = mb_detect_encoding($var0,$var3);

Vulnerable code:       tmpstr = (char *)strndup(value+1, value_length-2);
    else
      tmpstr = (char *)strndup(value, value_length);
    free(tmpstr);
  char *encstr = strndup(encoded_string.data(), encoded_string.size());
  free(encstr);
      to_r = strndup(to.data(), to_len);
Fixed code:       tmpstr = req::strndup(value + 1, value_length - 2);
    } else {
      tmpstr = req::strndup(value, value_length);
    req::free(tmpstr);
  char *encstr = req::strndup(encoded_string.data(), encoded_string.size());
  req::free(encstr);
      to_r = req::strndup(to.data(), to_len);
  if (to_r != to.data()) {
    req::free(to_r);
  }
<?hh // strict
// Copyright 2004-present Facebook. All Rights Reserved.

$var0 = null;
$var3 = "\x00";
$var3 = mb_detect_encoding($var0,$var3);