CVE-2018-6345: Out-of-bounds Write (CWE-787)
value: The function number_format is vulnerable to a heap overflow issue when its second argument ($dec_points) is excessively large. The internal implementation of the function will cause a string to be created with an invalid length, which can then interact poorly with other functions. This affects all supported versions of HHVM (3.30.1 and 3.27.5 and below).
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2019-01-15T22:29Z
Repository: https://github.com/facebook/hhvm
Language: PHP
Files changed: hphp/runtime/base/zend-string.cpp, hphp/test/slow/string/number_format_error.php, hphp/test/slow/string/number_format_error.php.expect
Commit date: 2018-12-14 08:49:28 -0800
Commit message: Fix a bug in number_format()

Summary: In case of error while calling snprintf, return empty string instead of malformed string.

Reviewed By: binliu19

Differential Revision: D13379372

fbshipit-source-id: 1e5611598464daccc674d5a96558c9f2aee08fc8

Diff: diff --git a/hphp/runtime/base/zend-string.cpp b/hphp/runtime/base/zend-string.cpp
index 072148f61c0..40267cffd6f 100644
--- a/hphp/runtime/base/zend-string.cpp
+++ b/hphp/runtime/base/zend-string.cpp
@@ -1766,19 +1766,21 @@ String string_number_format(double d, int dec,
 
   // departure from PHP: we got rid of dependencies on spprintf() here.
   String tmpstr(63, ReserveString);
   tmpbuf = tmpstr.mutableData();
   tmplen = snprintf(tmpbuf, 64, "%.*F", dec, d);
+  if (tmplen < 0) return empty_string();
   if (tmpbuf == nullptr || !isdigit((int)tmpbuf[0])) {
     tmpstr.setSize(tmplen);
     return tmpstr;
   }
   if (tmplen >= 64) {
     // Uncommon, asked for more than 64 chars worth of precision
     tmpstr = String(tmplen, ReserveString);
     tmpbuf = tmpstr.mutableData();
     tmplen = snprintf(tmpbuf, tmplen + 1, "%.*F", dec, d);
+    if (tmplen < 0) return empty_string();
     if (tmpbuf == nullptr || !isdigit((int)tmpbuf[0])) {
       tmpstr.setSize(tmplen);
       return tmpstr;
     }
   }
diff --git a/hphp/test/slow/string/number_format_error.php b/hphp/test/slow/string/number_format_error.php
new file mode 100644
index 00000000000..e5899967bc0
--- /dev/null
+++ b/hphp/test/slow/string/number_format_error.php
@@ -0,0 +1,19 @@
+<?php
+  $READ_LENGTH = 0x1000; // choose leak size
+// construct fake iptc header for controlled read
+$iptc_hdr =
+  "\x1c\x01" . // magic
+  "\x00\x80" . // dataset, recnum
+  "\x00" .     // padding
+  pack("N", $READ_LENGTH);
+// spray a bit so it's near the broken string
+$holder = [];
+for($i = 0; $i < 100; $i++)
+  $holder[] = str_pad($iptc_hdr, 96);
+// trigger bug to create string with len=-1
+$badstr = number_format(0,0x7fffffff);
+var_dump($badstr);
+// leak memory :)
+$tmp = iptcparse($badstr);
+var_dump($tmp);
+?>
diff --git a/hphp/test/slow/string/number_format_error.php.expect b/hphp/test/slow/string/number_format_error.php.expect
new file mode 100644
index 00000000000..4cbe7ce9a78
--- /dev/null
+++ b/hphp/test/slow/string/number_format_error.php.expect
@@ -0,0 +1,2 @@
+string(0) ""
+bool(false)

Fixed code:   if (tmplen < 0) return empty_string();
    if (tmplen < 0) return empty_string();
<?php
  $READ_LENGTH = 0x1000; // choose leak size
// construct fake iptc header for controlled read
$iptc_hdr =
  "\x1c\x01" . // magic
  "\x00\x80" . // dataset, recnum
  "\x00" .     // padding
  pack("N", $READ_LENGTH);
// spray a bit so it's near the broken string
$holder = [];
for($i = 0; $i < 100; $i++)
  $holder[] = str_pad($iptc_hdr, 96);
// trigger bug to create string with len=-1
$badstr = number_format(0,0x7fffffff);
var_dump($badstr);
// leak memory :)
$tmp = iptcparse($badstr);
var_dump($tmp);
?>
string(0) ""
bool(false)