This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

char* result is not initialized at:

https://cs.chromium.org/chromium/src/third_party/leveldatabase/src/util/arena.cc?rcl=a7bff697baa062c8f6b8fb760eacf658712b611a&l=61

And later referred at:
https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_parser.cc&sq=package:chromium&type=cs&l=697. 

cmumford@chromium.org, can you take a look into this issue since you modified code related. Thanks a lot.

This is blocking next week beta release on Wednesday, 02/08. 
awhalley@ to triage

Looking into this now...

I've run the build attached to the report, but am unable to reproduce. The report doesn't contain the fuzzer-testcases. Is there a place where I can get these?

Still visually inspecting the code.

jdoerrie@ Can you please give this a look? I can't reproduce, but Chrome's been using the same leveldb since April 2016, and you made a change (http://crrev.com/2667893003) on 2 Feb 2017 to JSONParser::Parse(). I'm wondering if the JSON parser has a buffer overflow issue. Please assign back to me if you feel this is unrelated to your JSON parser changes.

I'm looking into this, this in fact might be related to the JSON parser changes. So far I'm unable to reproduce as well, would be great to get some pointers here.

I managed to reliably reproduce this locally. The crash happens in JSONParser with input "1486560455466.0". It will recognize that it is a number and call |ConsumeNumber|. In line 697  (https://cs.chromium.org/chromium/src/base/json/json_parser.cc?sq=package:chromium&type=cs&l=697) it then checks whether an optional exponential part is present. This check happens without end of input check, causing the crash here. I suspect it will also crash for input "1486560455466", since the dot check is not guarded either.

I still don't understand why this only happens now, my change to JSON Parser should be unrelated. I will investigate further and send out a fix for this issue.

A friendly reminder that M57 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Thank you!

I implemented a fix: http://crrev.com/2683143002

This error is caused, because the JSON parser assumes that the input is null terminated. However, this is not always the case. A minimal reproduction case is the following:

auto buffer = MakeUnique<char[]>(1);
buffer[0] = 1;
JSONParser().Parse(StringPiece(buffer, 1));

Prior to my recent change to JSONParser you had to call |JSONParser(JSONParserOptions::JSON_DETACHABLE_CHILDREN).Parse(StringPiece(buffer, 1))| to trigger the same behavior.

jdoerrie: How were you reproing this issue? I tried "1486560455466.0" with a local MSan build and couldn't get it to crash.

I think there's two issues being discussed here:

1) The parser in general expects nul-terminated input. I think this may be a fine thing to enforce (yes, with a one-byte overread for non-terminated input), since JSON should be string data. You note in #14 "However, this is not always the case", but does this actually happen in non-fuzzer/contrived cases? Generally speaking any untrusted data is parsed in a sandboxed process, and trusted data is nul terminated, so I'm not sure this occurs in practice.

2) The bug in ConsumeNumber. I believe this is because, as the comment below states (https://cs.chromium.org/chromium/src/base/json/json_parser.cc?sq=package:chromium&type=cs&l=708), ReadInt is greedy. I think adding checks in the vein of |CanConsume(0)| or |pos_ < end_pos_| after each call to ReadInt is probably the right fix for that.

Initially I downloaded the unminimized test case and build binary from  https://cluster-fuzz.appspot.com/testcase?key=6514290216665088 and ran it with the flags provided above the stack trace on the page. Here I could observe that |Parse| was called with "1486560455466.0" when the crash happens.

Currently it is probably easiest to simply take the patch to json_parser_unittest.cc from http://crrev.com/2684923004 (i.e. https://codereview.chromium.org/download/issue2684923004_1_10002.diff) and run it with an MSan build. The patch simply adds input without null-termination. You should be able to observe crashes is |JSONParserTest.ConsumeNumbers|.

In #14 I described the gist of how I created the non-terminated input (it should have said |buffer[0] = '1';| instead of |buffer[0] = 1;|, though).

Regarding your points:

1) I am not sure myself how often non-terminated input occurs in practice. But I feel like both accepting StringPieces as input and assuming it is always null-terminated is a mistake (see this comment in string_piece.h: https://cs.chromium.org/chromium/src/base/strings/string_piece.h?l=204). In my opinion we should either ensure that the input is null-terminated (preferably without dereferencing uninitialized memory), or update the parser logic to drop the assumption about '\0' being present.

2) You are probably right. If we drop the assumption of null terminated input, every occurrence of |*pos_| should be guarded by |CanConsume(1)|. Furthermore, |*NextChar()| is only safe if |CanConsume(2)| is true.

Do either of the following crash for you under MSan? I haven't been able to reproduce this still... (wondering if I'm holding it wrong).

diff --git a/base/json/json_parser_unittest.cc b/base/json/json_parser_unittest.cc
index d004c480cf66..f48ad3e9ac94 100644
--- a/base/json/json_parser_unittest.cc
+++ b/base/json/json_parser_unittest.cc
@@ -8,6 +8,7 @@
 
 #include <memory>
 
+#include "base/memory/ptr_util.h"
 #include "base/json/json_reader.h"
 #include "base/values.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -342,5 +343,17 @@ TEST_F(JSONParserTest, ReplaceInvalidCharacters) {
   EXPECT_EQ(kUnicodeReplacementString, str);
 }
 
+TEST_F(JSONParserTest,  Bug688086 ) {
+  std::string* s = new std::string("1486560455466.0");
+  EXPECT_TRUE(JSONReader::Read(*s));
+}
+
+TEST_F(JSONParserTest, Bug688086_2) {
+  auto buffer = MakeUnique<char[]>(1);
+  buffer[0] = 1;
+  StringPiece sp(buffer.get(), 1);
+  EXPECT_TRUE(internal::JSONParser(0).Parse(sp));
+}
+
 }  // namespace internal
 }  // namespace base

Reproduction is a bit tricky indeed. I'm getting it to reliable crash with the following setup:


diff --git a/base/json/json_parser_unittest.cc b/base/json/json_parser_unittest.cc
index d004c480cf66..a5d26ebc0e80 100644
--- a/base/json/json_parser_unittest.cc
+++ b/base/json/json_parser_unittest.cc
@@ -9,6 +9,7 @@
 #include <memory>
 
 #include "base/json/json_reader.h"
+#include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -342,5 +343,17 @@ TEST_F(JSONParserTest, ReplaceInvalidCharacters) {
   EXPECT_EQ(kUnicodeReplacementString, str);
 }
 
+TEST_F(JSONParserTest,  Bug688086 ) {
+  std::string* s = new std::string("1486560455466.0");
+  EXPECT_TRUE(JSONReader::Read(*s));
+}
+
+TEST_F(JSONParserTest, Bug688086_2) {
+  auto buffer = MakeUnique<char[]>(1);
+  buffer[0] = '1';
+  StringPiece sp(buffer.get(), 1);
+  EXPECT_TRUE(internal::JSONParser(0).Parse(sp));
+}
+
 }  // namespace internal
 }  // namespace base


This is the same as your patch, except replacing |buffer[0] = 1| with |buffer[0] = '1'|, i.e. use the character '1' instead of the literal 1). Sorry for posting the wrong set up in Comment 14, I tried to correct it in Comment 16.

Using this patch, this crashes reliably for me:
$ ./out/msan/base_unittests --gtest_filter="JSONParserTest.Consume*:JSONParserTest.Bug688086_2"

Curiously enough, running the Consume* tests before is critical, since this is not crashing for me:
$ ./out/msan/base_unittests --gtest_filter="JSONParserTest.Bug688086_2"

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bef4f3ae9bdf490e9de52126f16d1e22ed2890e4

commit bef4f3ae9bdf490e9de52126f16d1e22ed2890e4
Author: rsesek <rsesek@chromium.org>
Date: Mon Feb 27 20:51:59 2017

Fix several potential buffer over-read errors in JSONParser::ConsumeNumber.

BUG= 688086 
TEST=base_unittests --gtest_filter=JSONParser* under MSan

Review-Url: https://codereview.chromium.org/2712013003
Cr-Commit-Position: refs/heads/master@{#453328}

[modify] https://crrev.com/bef4f3ae9bdf490e9de52126f16d1e22ed2890e4/base/json/json_parser.cc
[modify] https://crrev.com/bef4f3ae9bdf490e9de52126f16d1e22ed2890e4/base/json/json_parser_unittest.cc

ClusterFuzz has detected this issue as fixed in range 453299:453344.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6514290216665088

Fuzzer: meacer_chromebot_extensions
Job Type: linux_msan_chrome
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  base::internal::JSONParser::ConsumeNumber
  base::internal::JSONParser::ParseToken
  base::internal::JSONParser::Parse
  
Sanitizer: memory (MSAN)

Recommended Security Severity: Medium

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_msan_chrome&range=447735:447744
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_msan_chrome&range=453299:453344

Reproducer Testcase: https://cluster-fuzz.appspot.com/download/AMIfv95IhgAMWKN2QAT8LDKgWv1ruYxCxcLcXZcEw2_u5PUcvORgqmaC_BATbjVX4q8k0iheFQLden1tCeBanafU2WDQT5xhbfIuGNhPWKKAvm2Zwncgi5u37GMzb2PdcgHu0dWtPoKfL2pz05hTmJqqxjIkuVLfsN2F_--D7GG3Gxf8BP56tYg4EFudjIoRxCU7gqyYb4mPi_CvJTL-f37lpHfhLjqe49Rr6kX8ejCogDY8e1y9I8gXJ9HwC7V7x3uNUee0ckuNI2S3qwDKJe90Mt9mZiVrP_VvfhFBJsqK903p140Bm_2PoqohY8vnBcnGWTY9mC2CP7r9YsAc5dmugk5OoKcvMvfbZ9uJFJm3lKFBVl6y4_w?testcase_id=6514290216665088


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot