CVE-2019-11929: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: Insufficient boundary checks when formatting numbers in number_format allows read/write access to out-of-bounds memory, potentially leading to remote code execution. This issue affects HHVM versions prior to 3.30.10, all versions between 4.0.0 and 4.8.5, all versions between 4.9.0 and 4.18.2, and versions 4.19.0, 4.19.1, 4.20.0, 4.20.1, 4.20.2, 4.21.0, 4.22.0, 4.23.0.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 9.8
Published: 2019-10-02T19:15Z
Repository: https://github.com/facebook/hhvm
Language: PHP
Files changed: hphp/runtime/base/zend-string.cpp, hphp/test/slow/string/number_format_t53795309.php, hphp/test/slow/string/number_format_t53795309.php.expect
Commit date: 2019-09-18 01:48:00 -0700
Commit message: string_number_format: Correctly handles return value of snprintf

Summary: `snprintf` can return a value greater than the number of bytes copied. In case the first byte of the string is not a digit (could be '-'), size of `tmpstr` was being updated without checking `tmplen`. This resulted in either an assertion error or a heap overflow depending on whether the assertion is compiled or not.

Reviewed By: mofarrell, qianxuweiren

Differential Revision: D17327899

fbshipit-source-id: ee53875d21e02608c6d870388eecf1464de24ff1

Diff: diff --git a/hphp/runtime/base/zend-string.cpp b/hphp/runtime/base/zend-string.cpp
index 8a5ec8933691..7caef317c51f 100644
--- a/hphp/runtime/base/zend-string.cpp
+++ b/hphp/runtime/base/zend-string.cpp
@@ -1616,15 +1616,19 @@ String string_number_format(double d, int dec,
 
   if (dec < 0) dec = 0;
   d = php_math_round(d, dec);
 
   // departure from PHP: we got rid of dependencies on spprintf() here.
+  // This actually means 63 bytes for characters + 1 byte for '\0'
   String tmpstr(63, ReserveString);
   tmpbuf = tmpstr.mutableData();
   tmplen = snprintf(tmpbuf, 64, "%.*F", dec, d);
+  // From the man page of snprintf, the return value is:
+  // The number of characters that would have been written if n had been
+  // sufficiently large, not counting the terminating null character.
   if (tmplen < 0) return empty_string();
-  if (tmpbuf == nullptr || !isdigit((int)tmpbuf[0])) {
+  if (tmplen < 64 && (tmpbuf == nullptr || !isdigit((int)tmpbuf[0]))) {
     tmpstr.setSize(tmplen);
     return tmpstr;
   }
   if (tmplen >= 64) {
     // Uncommon, asked for more than 64 chars worth of precision
diff --git a/hphp/test/slow/string/number_format_t53795309.php b/hphp/test/slow/string/number_format_t53795309.php
new file mode 100644
index 000000000000..c6fee71a722f
--- /dev/null
+++ b/hphp/test/slow/string/number_format_t53795309.php
@@ -0,0 +1,9 @@
+<?hh
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+<<__EntryPoint>>
+function main() {
+  $bin_repr = "\x00\x00\x00\x00\x00\x00\x00\x80";
+  $double_num = unpack("dnum", $bin_repr)['num'];
+  var_dump(number_format($double_num, 100));
+}
diff --git a/hphp/test/slow/string/number_format_t53795309.php.expect b/hphp/test/slow/string/number_format_t53795309.php.expect
new file mode 100644
index 000000000000..f4417f7682bb
--- /dev/null
+++ b/hphp/test/slow/string/number_format_t53795309.php.expect
@@ -0,0 +1 @@
+string(103) "-0.0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"

Vulnerable code:   if (tmpbuf == nullptr || !isdigit((int)tmpbuf[0])) {
Fixed code:   // This actually means 63 bytes for characters + 1 byte for '\0'
  // From the man page of snprintf, the return value is:
  // The number of characters that would have been written if n had been
  // sufficiently large, not counting the terminating null character.
  if (tmplen < 64 && (tmpbuf == nullptr || !isdigit((int)tmpbuf[0]))) {
<?hh
// Copyright 2004-present Facebook. All Rights Reserved.

<<__EntryPoint>>
function main() {
  $bin_repr = "\x00\x00\x00\x00\x00\x00\x00\x80";
  $double_num = unpack("dnum", $bin_repr)['num'];
  var_dump(number_format($double_num, 100));
}
string(103) "-0.0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"