CVE-2019-11935: Buffer Copy without Checking Size of Input ('Classic Buffer Overflow') (CWE-120)
value: Insufficient boundary checks when processing a string in mb_ereg_replace allows access to out-of-bounds memory. 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: The product copies an input buffer to an output buffer without verifying that the size of the input buffer is less than the size of the output buffer, leading to a buffer overflow.
CVSS v3 base score: 9.8
Published: 2019-12-04T17:16Z
Repository: https://github.com/facebook/hhvm
Language: PHP
Files changed: hphp/runtime/ext/mbstring/ext_mbstring.cpp, hphp/test/zend/good/ext/mbstring/mb_ereg_replace_invalid_replacement.php, hphp/test/zend/good/ext/mbstring/mb_ereg_replace_invalid_replacement.php.expectf
Commit date: 2019-10-28 20:23:43 -0700
Commit message: Fix buffer overflow in mb_ereg_replace

Summary:
This diff has already been landed to release and to open-source branches. We're now landing it on master.

CVE-2019-11935

Reviewed By: jjergus

Differential Revision: D18177934

fbshipit-source-id: d108a59e38c67f5f5e835febd7255307605ba62c

Diff: diff --git a/hphp/runtime/ext/mbstring/ext_mbstring.cpp b/hphp/runtime/ext/mbstring/ext_mbstring.cpp
index b6f9940829a2..0766bfd210e7 100644
--- a/hphp/runtime/ext/mbstring/ext_mbstring.cpp
+++ b/hphp/runtime/ext/mbstring/ext_mbstring.cpp
@@ -3607,26 +3607,31 @@ static Variant _php_mb_regex_ereg_replace_exec(const Variant& pattern,
       i = 0;
       p = replacement.data();
       while (i < replacement.size()) {
         int fwd = (int)php_mb_mbchar_bytes_ex(p, enc);
         n = -1;
-        if ((replacement.size() - i) >= 2 && fwd == 1 &&
-          p[0] == '\\' && p[1] >= '0' && p[1] <= '9') {
+        auto const remaining = replacement.size() - i;
+        if (remaining >= 2 && fwd == 1 &&
+            p[0] == '\\' && p[1] >= '0' && p[1] <= '9') {
           n = p[1] - '0';
         }
         if (n >= 0 && n < regs->num_regs) {
           if (regs->beg[n] >= 0 && regs->beg[n] < regs->end[n] &&
               regs->end[n] <= str.size()) {
             out_buf.append(str.data() + regs->beg[n],
                            regs->end[n] - regs->beg[n]);
           }
           p += 2;
           i += 2;
-        } else {
+        } else if (remaining >= fwd) {
           out_buf.append(p, fwd);
           p += fwd;
           i += fwd;
+        } else {
+          raise_warning("Replacement ends with unterminated %s: 0x%hhx",
+                        enc->name, *p);
+          break;
         }
       }
       n = regs->end[0];
       if ((pos - (OnigUChar *)str.data()) < n) {
         pos = (OnigUChar *)(str.data() + n);
diff --git a/hphp/test/zend/good/ext/mbstring/mb_ereg_replace_invalid_replacement.php b/hphp/test/zend/good/ext/mbstring/mb_ereg_replace_invalid_replacement.php
new file mode 100644
index 000000000000..949e7d8c90a7
--- /dev/null
+++ b/hphp/test/zend/good/ext/mbstring/mb_ereg_replace_invalid_replacement.php
@@ -0,0 +1,7 @@
+<?hh
+
+<<__EntryPoint>>
+function main(): void {
+  var_dump(mb_ereg_replace("", "\xf1", "", ""));
+  throw new Error("done");
+}
diff --git a/hphp/test/zend/good/ext/mbstring/mb_ereg_replace_invalid_replacement.php.expectf b/hphp/test/zend/good/ext/mbstring/mb_ereg_replace_invalid_replacement.php.expectf
new file mode 100644
index 000000000000..9b91fa9bca3a
--- /dev/null
+++ b/hphp/test/zend/good/ext/mbstring/mb_ereg_replace_invalid_replacement.php.expectf
@@ -0,0 +1,7 @@
+Warning: Replacement ends with unterminated UTF-8: 0xf1 in %s/mb_ereg_replace_invalid_replacement.php on line 5
+string(0) ""
+
+Fatal error: Uncaught Error: done in %s/mb_ereg_replace_invalid_replacement.php:6
+Stack trace:
+#0 (): main()
+#1 {main}

Vulnerable code:         if ((replacement.size() - i) >= 2 && fwd == 1 &&
          p[0] == '\\' && p[1] >= '0' && p[1] <= '9') {
        } else {
Fixed code:         auto const remaining = replacement.size() - i;
        if (remaining >= 2 && fwd == 1 &&
            p[0] == '\\' && p[1] >= '0' && p[1] <= '9') {
        } else if (remaining >= fwd) {
        } else {
          raise_warning("Replacement ends with unterminated %s: 0x%hhx",
                        enc->name, *p);
          break;
<?hh

<<__EntryPoint>>
function main(): void {
  var_dump(mb_ereg_replace("", "\xf1", "", ""));
  throw new Error("done");
}
Warning: Replacement ends with unterminated UTF-8: 0xf1 in %s/mb_ereg_replace_invalid_replacement.php on line 5
string(0) ""

Fatal error: Uncaught Error: done in %s/mb_ereg_replace_invalid_replacement.php:6
Stack trace:
#0 (): main()
#1 {main}