From 82836a62befb31ef41dd75ccc17992ed575841db Mon Sep 17 00:00:00 2001 From: Vladimir Glavnyy <31897320+vglavnyy@users.noreply.github.com> Date: Tue, 5 Jan 2021 03:39:12 +0700 Subject: [PATCH] [idl_parser] Improve stack overflow protection (#6364) * [idl_parser] Improve stack overflow protection Add stack overflow protection for Flexbuffer and nested Flatbuffer parsers. Replaces the `Recurse()` method by the new ParseDepthGuard RAII class. * Remove move operator from Parser. It was wrong decision to add move ctor and assignment into Parser class. These operators will make it extremely difficult to add constant or reference fields in the future. * Remove ';' from definition of FLATBUFFERS_DELETE_FUNC * Format code * Make this PR compatible with MSVC2010 (it doesn't support inherited ctor) --- include/flatbuffers/base.h | 4 +- include/flatbuffers/flatbuffers.h | 13 +-- include/flatbuffers/idl.h | 17 ++-- src/idl_parser.cpp | 98 ++++++++++++++-------- tests/fuzzer/CMakeLists.txt | 2 +- tests/fuzzer/flatbuffers_monster_fuzzer.cc | 8 +- 6 files changed, 84 insertions(+), 58 deletions(-) diff --git a/include/flatbuffers/base.h b/include/flatbuffers/base.h index 92db55b7c..46340ec85 100644 --- a/include/flatbuffers/base.h +++ b/include/flatbuffers/base.h @@ -197,9 +197,9 @@ namespace flatbuffers { #if (!defined(_MSC_VER) || _MSC_FULL_VER >= 180020827) && \ (!defined(__GNUC__) || (__GNUC__ * 100 + __GNUC_MINOR__ >= 404)) || \ defined(__clang__) - #define FLATBUFFERS_DELETE_FUNC(func) func = delete; + #define FLATBUFFERS_DELETE_FUNC(func) func = delete #else - #define FLATBUFFERS_DELETE_FUNC(func) private: func; + #define FLATBUFFERS_DELETE_FUNC(func) private: func #endif #if (!defined(_MSC_VER) || _MSC_VER >= 1900) && \ diff --git a/include/flatbuffers/flatbuffers.h b/include/flatbuffers/flatbuffers.h index 12fc64c07..c429cc436 100644 --- a/include/flatbuffers/flatbuffers.h +++ b/include/flatbuffers/flatbuffers.h @@ -821,9 +821,9 @@ class DetachedBuffer { #if !defined(FLATBUFFERS_CPP98_STL) // clang-format on // These may change access mode, leave these at end of public section - FLATBUFFERS_DELETE_FUNC(DetachedBuffer(const DetachedBuffer &other)) + FLATBUFFERS_DELETE_FUNC(DetachedBuffer(const DetachedBuffer &other)); FLATBUFFERS_DELETE_FUNC( - DetachedBuffer &operator=(const DetachedBuffer &other)) + DetachedBuffer &operator=(const DetachedBuffer &other)); // clang-format off #endif // !defined(FLATBUFFERS_CPP98_STL) // clang-format on @@ -1066,8 +1066,8 @@ class vector_downward { private: // You shouldn't really be copying instances of this class. - FLATBUFFERS_DELETE_FUNC(vector_downward(const vector_downward &)) - FLATBUFFERS_DELETE_FUNC(vector_downward &operator=(const vector_downward &)) + FLATBUFFERS_DELETE_FUNC(vector_downward(const vector_downward &)); + FLATBUFFERS_DELETE_FUNC(vector_downward &operator=(const vector_downward &)); Allocator *allocator_; bool own_allocator_; @@ -1891,7 +1891,7 @@ class FlatBufferBuilder { } FLATBUFFERS_DELETE_FUNC( - StructKeyComparator &operator=(const StructKeyComparator &)) + StructKeyComparator &operator=(const StructKeyComparator &)); }; /// @endcond @@ -1966,7 +1966,8 @@ class FlatBufferBuilder { vector_downward &buf_; private: - FLATBUFFERS_DELETE_FUNC(TableKeyComparator &operator=(const TableKeyComparator &other)) + FLATBUFFERS_DELETE_FUNC( + TableKeyComparator &operator=(const TableKeyComparator &other)); }; /// @endcond diff --git a/include/flatbuffers/idl.h b/include/flatbuffers/idl.h index 31a41d752..1d57ba164 100644 --- a/include/flatbuffers/idl.h +++ b/include/flatbuffers/idl.h @@ -35,7 +35,7 @@ // Definition Language) / schema file. // Limits maximum depth of nested objects. -// Prevents stack overflow while parse flatbuffers or json. +// Prevents stack overflow while parse scheme, or json, or flexbuffer. #if !defined(FLATBUFFERS_MAX_PARSING_DEPTH) # define FLATBUFFERS_MAX_PARSING_DEPTH 64 #endif @@ -767,8 +767,8 @@ class Parser : public ParserState { opts(options), uses_flexbuffers_(false), source_(nullptr), - anonymous_counter(0), - recurse_protection_counter(0) { + anonymous_counter_(0), + parse_depth_counter_(0) { if (opts.force_defaults) { builder_.ForceDefaults(true); } // Start out with the empty namespace being current. empty_namespace_ = new Namespace(); @@ -806,11 +806,6 @@ class Parser : public ParserState { } } -#ifdef FLATBUFFERS_DEFAULT_DECLARATION - Parser(Parser&&) = default; - Parser& operator=(Parser&&) = default; -#endif - // Parse the string containing either schema or JSON data, which will // populate the SymbolTable's or the FlatBufferBuilder above. // include_paths is used to resolve any include statements, and typically @@ -872,6 +867,8 @@ class Parser : public ParserState { static bool SupportsOptionalScalars(const flatbuffers::IDLOptions &opts); private: + class ParseDepthGuard; + void Message(const std::string &msg); void Warning(const std::string &msg); FLATBUFFERS_CHECKED_ERROR ParseHexNum(int nibbles, uint64_t *val); @@ -1000,8 +997,8 @@ class Parser : public ParserState { std::vector> field_stack_; - int anonymous_counter; - int recurse_protection_counter; + int anonymous_counter_; + int parse_depth_counter_; // stack-overflow guard }; // Utility functions for multiple generators: diff --git a/src/idl_parser.cpp b/src/idl_parser.cpp index 77dbbafe5..92dee3eeb 100644 --- a/src/idl_parser.cpp +++ b/src/idl_parser.cpp @@ -155,18 +155,34 @@ CheckedError Parser::Error(const std::string &msg) { inline CheckedError NoError() { return CheckedError(false); } CheckedError Parser::RecurseError() { - return Error("maximum parsing recursion of " + - NumToString(FLATBUFFERS_MAX_PARSING_DEPTH) + " reached"); + return Error("maximum parsing depth " + NumToString(parse_depth_counter_) + + " reached"); } -template CheckedError Parser::Recurse(F f) { - if (recurse_protection_counter >= (FLATBUFFERS_MAX_PARSING_DEPTH)) - return RecurseError(); - recurse_protection_counter++; - auto ce = f(); - recurse_protection_counter--; - return ce; -} +class Parser::ParseDepthGuard { + public: + explicit ParseDepthGuard(Parser *parser_not_null) + : parser_(*parser_not_null), caller_depth_(parser_.parse_depth_counter_) { + FLATBUFFERS_ASSERT(caller_depth_ <= (FLATBUFFERS_MAX_PARSING_DEPTH) && + "Check() must be called to prevent stack overflow"); + parser_.parse_depth_counter_ += 1; + } + + ~ParseDepthGuard() { parser_.parse_depth_counter_ -= 1; } + + CheckedError Check() { + return caller_depth_ >= (FLATBUFFERS_MAX_PARSING_DEPTH) + ? parser_.RecurseError() + : CheckedError(false); + } + + FLATBUFFERS_DELETE_FUNC(ParseDepthGuard(const ParseDepthGuard &)); + FLATBUFFERS_DELETE_FUNC(ParseDepthGuard &operator=(const ParseDepthGuard &)); + + private: + Parser &parser_; + const int caller_depth_; +}; template std::string TypeToIntervalString() { return "[" + NumToString((flatbuffers::numeric_limits::lowest)()) + "; " + @@ -633,9 +649,11 @@ CheckedError Parser::ParseType(Type &type) { ECHECK(ParseTypeIdent(type)); } } else if (token_ == '[') { + ParseDepthGuard depth_guard(this); + ECHECK(depth_guard.Check()); NEXT(); Type subtype; - ECHECK(Recurse([&]() { return ParseType(subtype); })); + ECHECK(ParseType(subtype)); if (IsSeries(subtype)) { // We could support this, but it will complicate things, and it's // easier to work around with a struct around the inner vector. @@ -1033,6 +1051,8 @@ CheckedError Parser::ParseAnyValue(Value &val, FieldDef *field, } if (next_name == type_name) { EXPECT(':'); + ParseDepthGuard depth_guard(this); + ECHECK(depth_guard.Check()); Value type_val = type_field->value; ECHECK(ParseAnyValue(type_val, type_field, 0, nullptr, 0)); constant = type_val.constant; @@ -1159,6 +1179,9 @@ CheckedError Parser::ParseTableDelimiters(size_t &fieldn, CheckedError Parser::ParseTable(const StructDef &struct_def, std::string *value, uoffset_t *ovalue) { + ParseDepthGuard depth_guard(this); + ECHECK(depth_guard.Check()); + size_t fieldn_outer = 0; auto err = ParseTableDelimiters( fieldn_outer, &struct_def, @@ -1194,9 +1217,7 @@ CheckedError Parser::ParseTable(const StructDef &struct_def, std::string *value, ECHECK( ParseNestedFlatbuffer(val, field, fieldn, struct_def_inner)); } else { - ECHECK(Recurse([&]() { - return ParseAnyValue(val, field, fieldn, struct_def_inner, 0); - })); + ECHECK(ParseAnyValue(val, field, fieldn, struct_def_inner, 0)); } // Hardcoded insertion-sort with error-check. // If fields are specified in order, then this loop exits @@ -1372,9 +1393,7 @@ CheckedError Parser::ParseVector(const Type &type, uoffset_t *ovalue, auto err = ParseVectorDelimiters(count, [&](uoffset_t &) -> CheckedError { Value val; val.type = type; - ECHECK(Recurse([&]() { - return ParseAnyValue(val, field, fieldn, nullptr, count, true); - })); + ECHECK(ParseAnyValue(val, field, fieldn, nullptr, count, true)); field_stack_.push_back(std::make_pair(val, nullptr)); return NoError(); }); @@ -1548,7 +1567,7 @@ CheckedError Parser::ParseNestedFlatbuffer(Value &val, FieldDef *field, nested_parser.enums_ = enums_; nested_parser.opts = opts; nested_parser.uses_flexbuffers_ = uses_flexbuffers_; - + nested_parser.parse_depth_counter_ = parse_depth_counter_; // Parse JSON substring into new flatbuffer builder using nested_parser bool ok = nested_parser.Parse(substring.c_str(), nullptr, nullptr); @@ -1697,6 +1716,9 @@ static inline void SingleValueRepack(Value &e, double val) { #endif CheckedError Parser::ParseFunction(const std::string *name, Value &e) { + ParseDepthGuard depth_guard(this); + ECHECK(depth_guard.Check()); + // Copy name, attribute will be changed on NEXT(). const auto functionname = attribute_; if (!IsFloat(e.type.base_type)) { @@ -1707,7 +1729,7 @@ CheckedError Parser::ParseFunction(const std::string *name, Value &e) { } NEXT(); EXPECT('('); - ECHECK(Recurse([&]() { return ParseSingleValue(name, e, false); })); + ECHECK(ParseSingleValue(name, e, false)); EXPECT(')'); // calculate with double precision double x, y = 0.0; @@ -2678,7 +2700,7 @@ CheckedError Parser::ParseProtoFields(StructDef *struct_def, bool isextend, ECHECK(StartEnum(name, true, &oneof_union)); type = Type(BASE_TYPE_UNION, nullptr, oneof_union); } else { - auto name = "Anonymous" + NumToString(anonymous_counter++); + auto name = "Anonymous" + NumToString(anonymous_counter_++); ECHECK(StartStruct(name, &anonymous_struct)); type = Type(BASE_TYPE_STRUCT, anonymous_struct); } @@ -2851,22 +2873,24 @@ CheckedError Parser::ParseTypeFromProtoType(Type *type) { } CheckedError Parser::SkipAnyJsonValue() { + ParseDepthGuard depth_guard(this); + ECHECK(depth_guard.Check()); + switch (token_) { case '{': { size_t fieldn_outer = 0; - return ParseTableDelimiters( - fieldn_outer, nullptr, - [&](const std::string &, size_t &fieldn, - const StructDef *) -> CheckedError { - ECHECK(Recurse([&]() { return SkipAnyJsonValue(); })); - fieldn++; - return NoError(); - }); + return ParseTableDelimiters(fieldn_outer, nullptr, + [&](const std::string &, size_t &fieldn, + const StructDef *) -> CheckedError { + ECHECK(SkipAnyJsonValue()); + fieldn++; + return NoError(); + }); } case '[': { uoffset_t count = 0; return ParseVectorDelimiters(count, [&](uoffset_t &) -> CheckedError { - return Recurse([&]() { return SkipAnyJsonValue(); }); + return SkipAnyJsonValue(); }); } case kTokenStringConstant: @@ -2882,6 +2906,9 @@ CheckedError Parser::SkipAnyJsonValue() { } CheckedError Parser::ParseFlexBufferValue(flexbuffers::Builder *builder) { + ParseDepthGuard depth_guard(this); + ECHECK(depth_guard.Check()); + switch (token_) { case '{': { auto start = builder->StartMap(); @@ -2943,15 +2970,19 @@ CheckedError Parser::ParseFlexBufferValue(flexbuffers::Builder *builder) { bool Parser::ParseFlexBuffer(const char *source, const char *source_filename, flexbuffers::Builder *builder) { + const auto initial_depth = parse_depth_counter_; + (void)initial_depth; auto ok = !StartParseFile(source, source_filename).Check() && !ParseFlexBufferValue(builder).Check(); if (ok) builder->Finish(); + FLATBUFFERS_ASSERT(initial_depth == parse_depth_counter_); return ok; } bool Parser::Parse(const char *source, const char **include_paths, const char *source_filename) { - FLATBUFFERS_ASSERT(0 == recurse_protection_counter); + const auto initial_depth = parse_depth_counter_; + (void)initial_depth; bool r; if (opts.use_flexbuffers) { @@ -2959,16 +2990,17 @@ bool Parser::Parse(const char *source, const char **include_paths, } else { r = !ParseRoot(source, include_paths, source_filename).Check(); } - FLATBUFFERS_ASSERT(0 == recurse_protection_counter); + FLATBUFFERS_ASSERT(initial_depth == parse_depth_counter_); return r; } bool Parser::ParseJson(const char *json, const char *json_filename) { - FLATBUFFERS_ASSERT(0 == recurse_protection_counter); + const auto initial_depth = parse_depth_counter_; + (void)initial_depth; builder_.Clear(); const auto done = !StartParseFile(json, json_filename).Check() && !DoParseJson().Check(); - FLATBUFFERS_ASSERT(0 == recurse_protection_counter); + FLATBUFFERS_ASSERT(initial_depth == parse_depth_counter_); return done; } diff --git a/tests/fuzzer/CMakeLists.txt b/tests/fuzzer/CMakeLists.txt index a171eb06d..906fbdc16 100644 --- a/tests/fuzzer/CMakeLists.txt +++ b/tests/fuzzer/CMakeLists.txt @@ -10,7 +10,7 @@ option(BUILD_DEBUGGER "Compile a debugger with main() and without libFuzzer" OFF if(NOT DEFINED FLATBUFFERS_MAX_PARSING_DEPTH) # Force checking of RecursionError in the test - set(FLATBUFFERS_MAX_PARSING_DEPTH 8) + set(FLATBUFFERS_MAX_PARSING_DEPTH 24) endif() message(STATUS "FLATBUFFERS_MAX_PARSING_DEPTH: ${FLATBUFFERS_MAX_PARSING_DEPTH}") diff --git a/tests/fuzzer/flatbuffers_monster_fuzzer.cc b/tests/fuzzer/flatbuffers_monster_fuzzer.cc index 2b0504873..2d0bf26ca 100644 --- a/tests/fuzzer/flatbuffers_monster_fuzzer.cc +++ b/tests/fuzzer/flatbuffers_monster_fuzzer.cc @@ -45,7 +45,8 @@ std::string LoadBinarySchema(const char *file_name) { return schemafile; } -flatbuffers::Parser make_parser(const flatbuffers::IDLOptions opts) { +std::string do_test(const flatbuffers::IDLOptions &opts, + const std::string input_json) { // once loaded from disk static const std::string schemafile = LoadBinarySchema("./monster_test.bfbs"); // parse schema first, so we can use it to parse the data after @@ -55,12 +56,7 @@ flatbuffers::Parser make_parser(const flatbuffers::IDLOptions opts) { schemafile.size())); // (re)define parser options parser.opts = opts; - return parser; -} -std::string do_test(const flatbuffers::IDLOptions &opts, - const std::string input_json) { - auto parser = make_parser(opts); std::string jsongen; if (parser.ParseJson(input_json.c_str())) { flatbuffers::Verifier verifier(parser.builder_.GetBufferPointer(),