From c4c2df26b3e8fceb660b9b2a197a899ab32b640c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnter=20Obiltschnig?= Date: Tue, 15 Jun 2021 18:55:59 +0200 Subject: [PATCH] #3309: optimize parsing from stream (no copying of entire JSON to memory); limit maximum depth to avoid stack overflow with malicious documents (fuzzing - #3285); code cleanup --- JSON/include/Poco/JSON/Parser.h | 16 ++- JSON/include/Poco/JSON/ParserImpl.h | 9 +- JSON/src/ParseHandler.cpp | 5 +- JSON/src/Parser.cpp | 4 +- JSON/src/ParserImpl.cpp | 181 +++++++++++++++++++--------- JSON/testsuite/src/JSONTest.cpp | 8 -- JSON/testsuite/src/JSONTest.h | 1 - 7 files changed, 142 insertions(+), 82 deletions(-) diff --git a/JSON/include/Poco/JSON/Parser.h b/JSON/include/Poco/JSON/Parser.h index 59fb6e4f0..3e9707f3c 100644 --- a/JSON/include/Poco/JSON/Parser.h +++ b/JSON/include/Poco/JSON/Parser.h @@ -65,7 +65,7 @@ class JSON_API Parser: private ParserImpl { public: - Parser(const Handler::Ptr& pHandler = new ParseHandler, std::size_t bufSize = JSON_PARSE_BUFFER_SIZE); + Parser(const Handler::Ptr& pHandler = new ParseHandler); /// Creates JSON Parser, using the given Handler and buffer size. virtual ~Parser(); @@ -76,6 +76,10 @@ public: void setAllowComments(bool comments); /// Allow or disallow comments. By default, comments are not allowed. + /// + /// If set to true, comments will be filtered out of the input data + /// before passing the JSON on to the parser. This will impact performance, + /// especially when reading from a std::istream. bool getAllowComments() const; /// Returns true if comments are allowed, false otherwise. @@ -85,7 +89,11 @@ public: void setAllowNullByte(bool nullByte); /// Allow or disallow null byte in strings. /// - /// By default, null byte is allowed. + /// By default, null byte is allowed (true). + /// + /// If set to false, an additional check for "\u0000" will be performed + /// before passing the JSON on to the parser. This will impact performance, + /// especially when reading from a std::istream. bool getAllowNullByte() const; /// Returns true if null byte is allowed, false otherwise. @@ -94,6 +102,10 @@ public: void setDepth(std::size_t depth); /// Sets the allowed JSON depth. + /// + /// Default maximum depth is 128. Setting this value too high + /// may result in a stack overflow when parsing a (malicious) + /// JSON document. std::size_t getDepth() const; /// Returns the allowed JSON depth. diff --git a/JSON/include/Poco/JSON/ParserImpl.h b/JSON/include/Poco/JSON/ParserImpl.h index dd92fe84e..5984b2cf6 100644 --- a/JSON/include/Poco/JSON/ParserImpl.h +++ b/JSON/include/Poco/JSON/ParserImpl.h @@ -38,11 +38,9 @@ namespace JSON { class JSON_API ParserImpl { protected: - static const std::size_t JSON_PARSE_BUFFER_SIZE = 4096; - static const std::size_t JSON_PARSER_STACK_SIZE = 128; - static const int JSON_UNLIMITED_DEPTH = -1; + static const std::size_t JSON_DEFAULT_DEPTH = 128; - ParserImpl(const Handler::Ptr& pHandler = new ParseHandler, std::size_t bufSize = JSON_PARSE_BUFFER_SIZE); + ParserImpl(const Handler::Ptr& pHandler = new ParseHandler); /// Creates JSON ParserImpl, using the given Handler and buffer size. virtual ~ParserImpl(); @@ -101,12 +99,13 @@ private: void handleObject(); void handle(); void handle(const std::string& json); + void handle(std::istream& json); void stripComments(std::string& json); bool checkError(); struct json_stream* _pJSON; Handler::Ptr _pHandler; - int _depth; + std::size_t _depth; char _decimalPoint; bool _allowNullByte; bool _allowComments; diff --git a/JSON/src/ParseHandler.cpp b/JSON/src/ParseHandler.cpp index 1b271ff4f..ccc360653 100644 --- a/JSON/src/ParseHandler.cpp +++ b/JSON/src/ParseHandler.cpp @@ -138,10 +138,7 @@ void ParseHandler::setValue(const Var& value) _key.clear(); } } - else - { - throw JSONException("Attempt to set value on an empty stack"); - } + else throw JSONException("Attempt to set value on an empty stack"); } diff --git a/JSON/src/Parser.cpp b/JSON/src/Parser.cpp index 368a3a5d6..5da498589 100644 --- a/JSON/src/Parser.cpp +++ b/JSON/src/Parser.cpp @@ -29,8 +29,8 @@ namespace Poco { namespace JSON { -Parser::Parser(const Handler::Ptr& pHandler, std::size_t bufSize): - ParserImpl(pHandler, bufSize) +Parser::Parser(const Handler::Ptr& pHandler): + ParserImpl(pHandler) { } diff --git a/JSON/src/ParserImpl.cpp b/JSON/src/ParserImpl.cpp index a9227a20e..51523c185 100644 --- a/JSON/src/ParserImpl.cpp +++ b/JSON/src/ParserImpl.cpp @@ -34,10 +34,26 @@ namespace Poco { namespace JSON { -ParserImpl::ParserImpl(const Handler::Ptr& pHandler, std::size_t bufSize): +extern "C" +{ + static int istream_get(void* ptr) + { + std::istream* pIstr = reinterpret_cast(ptr); + return pIstr->get(); + } + + static int istream_peek(void* ptr) + { + std::istream* pIstr = reinterpret_cast(ptr); + return pIstr->peek(); + } +} + + +ParserImpl::ParserImpl(const Handler::Ptr& pHandler): _pJSON(new json_stream), _pHandler(pHandler), - _depth(JSON_UNLIMITED_DEPTH), + _depth(JSON_DEFAULT_DEPTH), _decimalPoint('.'), _allowNullByte(true), _allowComments(false) @@ -68,12 +84,34 @@ void ParserImpl::handle(const std::string& json) // json_open*() call, which calls internal init() json_set_streaming(_pJSON, false); ///////////////////////////////// - handle(); checkError(); + handle(); + checkError(); if (JSON_DONE != json_next(_pJSON)) throw JSONException("Excess characters found after JSON end."); json_close(_pJSON); } - catch (std::exception&) + catch (...) + { + json_close(_pJSON); + throw; + } +} + + +void ParserImpl::handle(std::istream& json) +{ + try + { + json_open_user(_pJSON, istream_get, istream_peek, &json); + checkError(); + json_set_streaming(_pJSON, false); + handle(); + checkError(); + if (JSON_DONE != json_next(_pJSON)) + throw JSONException("Excess characters found after JSON end."); + json_close(_pJSON); + } + catch (...) { json_close(_pJSON); throw; @@ -89,17 +127,33 @@ Dynamic::Var ParserImpl::parseImpl(const std::string& json) stripComments(str); handle(str); } - else handle(json); + else + { + handle(json); + } return asVarImpl(); } -Dynamic::Var ParserImpl::parseImpl(std::istream& in) +Dynamic::Var ParserImpl::parseImpl(std::istream& json) { - std::ostringstream os; - StreamCopier::copyStream(in, os); - return parseImpl(os.str()); + if (_allowComments || !_allowNullByte) + { + std::string str; + Poco::StreamCopier::copyToString(json, str); + if (_allowComments) + { + stripComments(str); + } + handle(str); + } + else + { + handle(json); + } + + return asVarImpl(); } @@ -146,7 +200,10 @@ void ParserImpl::handleArray() tok = json_peek(_pJSON); } - if (tok == JSON_ARRAY_END) handle(); + if (tok == JSON_ARRAY_END) + { + handle(); + } else throw JSONException("JSON array end not found"); } @@ -162,70 +219,74 @@ void ParserImpl::handleObject() tok = json_peek(_pJSON); } - if (tok == JSON_OBJECT_END) handle(); + if (tok == JSON_OBJECT_END) + { + handle(); + } else throw JSONException("JSON object end not found"); } void ParserImpl::handle() { + if (json_get_depth(_pJSON) > _depth) + throw JSONException("Maximum depth exceeded"); + enum json_type type = json_next(_pJSON); switch (type) { - case JSON_DONE: - return; - case JSON_NULL: - _pHandler->null(); - break; - case JSON_TRUE: - if (_pHandler) _pHandler->value(true); - break; - case JSON_FALSE: - if (_pHandler) _pHandler->value(false); - break; - case JSON_NUMBER: + case JSON_DONE: + return; + case JSON_NULL: + _pHandler->null(); + break; + case JSON_TRUE: + if (_pHandler) _pHandler->value(true); + break; + case JSON_FALSE: + if (_pHandler) _pHandler->value(false); + break; + case JSON_NUMBER: + if (_pHandler) { - if (_pHandler) + std::string str(json_get_string(_pJSON, NULL)); + if (str.find(_decimalPoint) != str.npos || str.find('e') != str.npos || str.find('E') != str.npos) { - std::string str(json_get_string(_pJSON, NULL)); - if (str.find(_decimalPoint) != str.npos || str.find('e') != str.npos || str.find('E') != str.npos) - { - _pHandler->value(NumberParser::parseFloat(str)); - } + _pHandler->value(NumberParser::parseFloat(str)); + } + else + { + Poco::Int64 val; + if (NumberParser::tryParse64(str, val)) + _pHandler->value(val); else - { - Poco::Int64 val; - if (NumberParser::tryParse64(str, val)) - _pHandler->value(val); - else - _pHandler->value(NumberParser::parseUnsigned64(str)); - } + _pHandler->value(NumberParser::parseUnsigned64(str)); } - break; } - case JSON_STRING: - if (_pHandler) - { - std::size_t length = 0; - const char* val = json_get_string(_pJSON, &length); - _pHandler->value(std::string(val, length == 0 ? 0 : length - 1)); // Decrease the length by 1 because it also contains the terminating null character - } - break; - case JSON_OBJECT: - if (_pHandler) _pHandler->startObject(); - handleObject(); - break; - case JSON_OBJECT_END: - if (_pHandler) _pHandler->endObject(); - return; - case JSON_ARRAY: - if (_pHandler) _pHandler->startArray(); - handleArray(); - break; - case JSON_ARRAY_END: - if (_pHandler) _pHandler->endArray(); - return; - case JSON_ERROR: + break; + case JSON_STRING: + if (_pHandler) + { + std::size_t length = 0; + const char* val = json_get_string(_pJSON, &length); + _pHandler->value(std::string(val, length == 0 ? 0 : length - 1)); // Decrease the length by 1 because it also contains the terminating null character + } + break; + case JSON_OBJECT: + if (_pHandler) _pHandler->startObject(); + handleObject(); + break; + case JSON_OBJECT_END: + if (_pHandler) _pHandler->endObject(); + return; + case JSON_ARRAY: + if (_pHandler) _pHandler->startArray(); + handleArray(); + break; + case JSON_ARRAY_END: + if (_pHandler) _pHandler->endArray(); + return; + case JSON_ERROR: { const char* pErr = json_get_error(_pJSON); std::string err(pErr ? pErr : "JSON parser error."); diff --git a/JSON/testsuite/src/JSONTest.cpp b/JSON/testsuite/src/JSONTest.cpp index 2d130436d..3cab104e0 100644 --- a/JSON/testsuite/src/JSONTest.cpp +++ b/JSON/testsuite/src/JSONTest.cpp @@ -1921,13 +1921,6 @@ void JSONTest::testUnicode() } -void JSONTest::testSmallBuffer() -{ - Poco::JSON::Parser parser(new Poco::JSON::ParseHandler(), 4); - std::string jsonStr = "{ \"x\" : \"123456789012345678901234567890123456789012345678901234567890\" }"; - parser.parse(jsonStr); -} - void JSONTest::testEscape0() { Poco::JSON::Object::Ptr json = new Poco::JSON::Object(); @@ -2261,7 +2254,6 @@ CppUnit::Test* JSONTest::suite() CppUnit_addTest(pSuite, JSONTest, testInvalidUnicodeJanssonFiles); CppUnit_addTest(pSuite, JSONTest, testTemplate); CppUnit_addTest(pSuite, JSONTest, testUnicode); - CppUnit_addTest(pSuite, JSONTest, testSmallBuffer); CppUnit_addTest(pSuite, JSONTest, testEscape0); CppUnit_addTest(pSuite, JSONTest, testNonEscapeUnicode); CppUnit_addTest(pSuite, JSONTest, testEscapeUnicode); diff --git a/JSON/testsuite/src/JSONTest.h b/JSON/testsuite/src/JSONTest.h index fdb95d577..90482672f 100644 --- a/JSON/testsuite/src/JSONTest.h +++ b/JSON/testsuite/src/JSONTest.h @@ -74,7 +74,6 @@ public: void testTemplate(); void testUnicode(); void testInvalidUnicodeJanssonFiles(); - void testSmallBuffer(); void testEscape0(); void testNonEscapeUnicode(); void testEscapeUnicode();