From 89b72e165371bf30d744cec8338f2fcc5cccc4df Mon Sep 17 00:00:00 2001 From: Christopher Dunn Date: Wed, 11 Feb 2015 10:01:47 -0600 Subject: [PATCH 1/5] test stackLimit --- src/test_lib_json/main.cpp | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/test_lib_json/main.cpp b/src/test_lib_json/main.cpp index e4d2dcb..30c80e2 100644 --- a/src/test_lib_json/main.cpp +++ b/src/test_lib_json/main.cpp @@ -1713,6 +1713,34 @@ JSONTEST_FIXTURE(CharReaderTest, parseWithDetailError) { delete reader; } +JSONTEST_FIXTURE(CharReaderTest, parseWithStackLimit) { + Json::CharReaderBuilder b; + Json::Value root; + char const doc[] = + "{ \"property\" : \"value\" }"; + { + b.settings_["stackLimit"] = 2; + Json::CharReader* reader(b.newCharReader()); + std::string errs; + bool ok = reader->parse( + doc, doc + std::strlen(doc), + &root, &errs); + JSONTEST_ASSERT(ok); + JSONTEST_ASSERT(errs == ""); + JSONTEST_ASSERT_EQUAL("value", root["property"]); + delete reader; + } + { + b.settings_["stackLimit"] = 1; + Json::CharReader* reader(b.newCharReader()); + std::string errs; + JSONTEST_ASSERT_THROWS(reader->parse( + doc, doc + std::strlen(doc), + &root, &errs)); + delete reader; + } +} + int main(int argc, const char* argv[]) { JsonTest::Runner runner; JSONTEST_REGISTER_FIXTURE(runner, ValueTest, checkNormalizeFloatingPointStr); @@ -1749,6 +1777,7 @@ int main(int argc, const char* argv[]) { JSONTEST_REGISTER_FIXTURE(runner, CharReaderTest, parseWithOneError); JSONTEST_REGISTER_FIXTURE(runner, CharReaderTest, parseChineseWithOneError); JSONTEST_REGISTER_FIXTURE(runner, CharReaderTest, parseWithDetailError); + JSONTEST_REGISTER_FIXTURE(runner, CharReaderTest, parseWithStackLimit); JSONTEST_REGISTER_FIXTURE(runner, WriterTest, dropNullPlaceholders); JSONTEST_REGISTER_FIXTURE(runner, StreamWriterTest, dropNullPlaceholders); From 99b8e856f676363cfcb66c7849b144f7c10d030a Mon Sep 17 00:00:00 2001 From: Christopher Dunn Date: Wed, 11 Feb 2015 09:49:11 -0600 Subject: [PATCH 2/5] stackLimit_ --- src/lib_json/json_reader.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib_json/json_reader.cpp b/src/lib_json/json_reader.cpp index b4c8965..59409cc 100644 --- a/src/lib_json/json_reader.cpp +++ b/src/lib_json/json_reader.cpp @@ -902,7 +902,8 @@ public: bool strictRoot_; bool allowDroppedNullPlaceholders_; bool allowNumericKeys_; -}; // OldFeatures + int stackLimit_; +}; // OurFeatures // exact copy of Implementation of class Features // //////////////////////////////// @@ -1853,6 +1854,7 @@ CharReader* CharReaderBuilder::newCharReader() const features.strictRoot_ = settings_["strictRoot"].asBool(); features.allowDroppedNullPlaceholders_ = settings_["allowDroppedNullPlaceholders"].asBool(); features.allowNumericKeys_ = settings_["allowNumericKeys"].asBool(); + features.stackLimit_ = settings_["stackLimit"].asInt(); return new OurCharReader(collectComments, features); } static void getValidReaderKeys(std::set* valid_keys) From 249ad9f47fa9125f219e1bdc5cc38020c4ca2667 Mon Sep 17 00:00:00 2001 From: Christopher Dunn Date: Tue, 10 Feb 2015 12:16:03 -0600 Subject: [PATCH 3/5] stackLimit --- include/json/reader.h | 3 +++ src/lib_json/json_reader.cpp | 2 ++ 2 files changed, 5 insertions(+) diff --git a/include/json/reader.h b/include/json/reader.h index 041ae0d..b5b2e97 100644 --- a/include/json/reader.h +++ b/include/json/reader.h @@ -310,6 +310,9 @@ public: - true if dropped null placeholders are allowed. (See StreamWriterBuilder.) - "allowNumericKeys": false or true - true if numeric object keys are allowed. + - "stackLimit": integer + - This is a security issue (seg-faults caused by deeply nested JSON), + so the default is low. You can examine 'settings_` yourself to see the defaults. You can also write and read them just like any diff --git a/src/lib_json/json_reader.cpp b/src/lib_json/json_reader.cpp index 59409cc..95eb4b2 100644 --- a/src/lib_json/json_reader.cpp +++ b/src/lib_json/json_reader.cpp @@ -1865,6 +1865,7 @@ static void getValidReaderKeys(std::set* valid_keys) valid_keys->insert("strictRoot"); valid_keys->insert("allowDroppedNullPlaceholders"); valid_keys->insert("allowNumericKeys"); + valid_keys->insert("stackLimit"); } bool CharReaderBuilder::validate(Json::Value* invalid) const { @@ -1903,6 +1904,7 @@ void CharReaderBuilder::setDefaults(Json::Value* settings) (*settings)["strictRoot"] = false; (*settings)["allowDroppedNullPlaceholders"] = false; (*settings)["allowNumericKeys"] = false; + (*settings)["stackLimit"] = 1000; //! [CharReaderBuilderDefaults] } From 4dca80da49f9dcdd375e2eac100b17fbe9d2bb79 Mon Sep 17 00:00:00 2001 From: Christopher Dunn Date: Wed, 11 Feb 2015 09:44:02 -0600 Subject: [PATCH 4/5] limit stackDepth --- src/lib_json/json_reader.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/lib_json/json_reader.cpp b/src/lib_json/json_reader.cpp index 95eb4b2..6cf23e3 100644 --- a/src/lib_json/json_reader.cpp +++ b/src/lib_json/json_reader.cpp @@ -1034,7 +1034,9 @@ private: Location lastValueEnd_; Value* lastValue_; std::string commentsBefore_; - OurFeatures features_; + int stackDepth_; + + OurFeatures const features_; bool collectComments_; }; // OurReader @@ -1065,6 +1067,7 @@ bool OurReader::parse(const char* beginDoc, nodes_.pop(); nodes_.push(&root); + stackDepth_ = 0; bool successful = readValue(); Token token; skipCommentTokens(token); @@ -1087,6 +1090,8 @@ bool OurReader::parse(const char* beginDoc, } bool OurReader::readValue() { + if (stackDepth_ >= features_.stackLimit_) throw std::runtime_error("Exceeded stackLimit in readValue()."); + ++stackDepth_; Token token; skipCommentTokens(token); bool successful = true; @@ -1158,6 +1163,7 @@ bool OurReader::readValue() { lastValue_ = ¤tValue(); } + --stackDepth_; return successful; } From 56df2068470241f9043b676bfae415ed62a0c172 Mon Sep 17 00:00:00 2001 From: Christopher Dunn Date: Wed, 11 Feb 2015 10:20:53 -0600 Subject: [PATCH 5/5] limit stackDepth for old (deprecated) Json::Reader too This is an improper solution. If multiple Readers exist, then the effect stackLimit is reduced because of side-effects. But our options are limited. We need to address the security hole without breaking binary-compatibility. However, this is not likely to cause any practical problems because: * Anyone using `operator>>(istream, Json::Value)` will be using the new code already * Multiple Readers are uncommon. * The stackLimit is quite high. * Deeply nested JSON probably would have hit the system limits anyway. --- src/lib_json/json_reader.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/lib_json/json_reader.cpp b/src/lib_json/json_reader.cpp index 6cf23e3..1e7db68 100644 --- a/src/lib_json/json_reader.cpp +++ b/src/lib_json/json_reader.cpp @@ -28,6 +28,9 @@ #pragma warning(disable : 4996) #endif +static int const stackLimit_g = 1000; +static int stackDepth_g = 0; // see readValue() + namespace Json { #if __cplusplus >= 201103L @@ -118,6 +121,7 @@ bool Reader::parse(const char* beginDoc, nodes_.pop(); nodes_.push(&root); + stackDepth_g = 0; // Yes, this is bad coding, but options are limited. bool successful = readValue(); Token token; skipCommentTokens(token); @@ -140,6 +144,13 @@ bool Reader::parse(const char* beginDoc, } bool Reader::readValue() { + // This is a non-reentrant way to support a stackLimit. Terrible! + // But this deprecated class has a security problem: Bad input can + // cause a seg-fault. This seems like a fair, binary-compatible way + // to prevent the problem. + if (stackDepth_g >= stackLimit_g) throw std::runtime_error("Exceeded stackLimit in readValue()."); + ++stackDepth_g; + Token token; skipCommentTokens(token); bool successful = true; @@ -211,6 +222,7 @@ bool Reader::readValue() { lastValue_ = ¤tValue(); } + --stackDepth_g; return successful; }