From 4a99af92dca9ceab1b22cc7462e3920183be69ec Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Tue, 12 May 2015 10:41:45 +0900 Subject: [PATCH] Implement the recursion limit in the CBOR decoder API It's very easy for a short packet to contain a very high number of nested arrays (a packet with N bytes can be N-1 nested arrays), which could cause stack overflow problems in devices using cbor_value_map_find_value, so ensure that we won't crash. The actual value needs to be chosen by the implementor depending on the stack size of their devices and the expected usage. Signed-off-by: Thiago Macieira --- TODO | 1 - src/cbor.h | 1 + src/cborerrorstrings.c | 3 ++ src/cborparser.c | 54 +++++++++++++++++----------- tests/parser/parser.pro | 1 + tests/parser/tst_parser.cpp | 72 +++++++++++++++++++++++++++++++++++++ 6 files changed, 110 insertions(+), 22 deletions(-) diff --git a/TODO b/TODO index 107ccc0..e9103ee 100644 --- a/TODO +++ b/TODO @@ -23,4 +23,3 @@ * (unlikely) Add API for checking the pairing of a tag and the tagged type * Write tests for error conditions * Fuzzy-test the decoder -* Add recursion limit to recursive functions (advance, map_find) diff --git a/src/cbor.h b/src/cbor.h index 4d1ae4b..7202feb 100644 --- a/src/cbor.h +++ b/src/cbor.h @@ -117,6 +117,7 @@ typedef enum CborError { /* internal implementation errors */ CborErrorDataTooLarge = 1024, + CborErrorNestingTooDeep, CborErrorInternalError = ~0U } CborError; diff --git a/src/cborerrorstrings.c b/src/cborerrorstrings.c index 25e6a58..9091c18 100644 --- a/src/cborerrorstrings.c +++ b/src/cborerrorstrings.c @@ -85,6 +85,9 @@ const char *cbor_error_string(CborError error) case CborErrorDataTooLarge: return _("internal error: data too large"); + case CborErrorNestingTooDeep: + return _("internal error: too many nested containers found in recursive function"); + case CborErrorInternalError: return _("internal error"); } diff --git a/src/cborparser.c b/src/cborparser.c index 03bb554..315f9ea 100644 --- a/src/cborparser.c +++ b/src/cborparser.c @@ -32,6 +32,10 @@ #include #include +#ifndef CBOR_PARSER_MAX_RECURSIONS +# define CBOR_PARSER_MAX_RECURSIONS 1024 +#endif + /** * \typedef CborValue * This type contains one value parsed from the CBOR stream. @@ -307,6 +311,34 @@ CborError cbor_value_advance_fixed(CborValue *it) return advance_internal(it); } +static CborError advance_recursive(CborValue *it, int nestingLevel) +{ + if (is_fixed_type(it->type)) + return advance_internal(it); + + if (!cbor_value_is_container(it)) { + size_t len = SIZE_MAX; + return cbor_value_copy_string(it, NULL, &len, it); + } + + // map or array + if (nestingLevel == CBOR_PARSER_MAX_RECURSIONS) + return CborErrorNestingTooDeep; + + CborError err; + CborValue recursed; + err = cbor_value_enter_container(it, &recursed); + if (err) + return err; + while (!cbor_value_at_end(&recursed)) { + err = advance_recursive(&recursed, nestingLevel + 1); + if (err) + return err; + } + return cbor_value_leave_container(it, &recursed); +} + + /** * Advances the CBOR value \a it by one element, skipping over containers. * Unlike cbor_value_advance_fixed(), this function can be called on a CBOR @@ -323,26 +355,7 @@ CborError cbor_value_advance(CborValue *it) assert(it->type != CborInvalidType); if (!it->remaining) return CborErrorAdvancePastEOF; - if (is_fixed_type(it->type)) - return advance_internal(it); - - if (!cbor_value_is_container(it)) { - size_t len = SIZE_MAX; - return cbor_value_copy_string(it, NULL, &len, it); - } - - // map or array - CborError err; - CborValue recursed; - err = cbor_value_enter_container(it, &recursed); - if (err) - return err; - while (!cbor_value_at_end(&recursed)) { - err = cbor_value_advance(&recursed); - if (err) - return err; - } - return cbor_value_leave_container(it, &recursed); + return advance_recursive(it, 0); } /** @@ -361,7 +374,6 @@ CborError cbor_value_skip_tag(CborValue *it) return CborNoError; } - /** * \fn bool cbor_value_is_container(const CborValue *it) * diff --git a/tests/parser/parser.pro b/tests/parser/parser.pro index c219b80..bce38a3 100644 --- a/tests/parser/parser.pro +++ b/tests/parser/parser.pro @@ -2,5 +2,6 @@ SOURCES += tst_parser.cpp CONFIG += testcase parallel_test c++11 QT = core testlib +DEFINES += CBOR_PARSER_MAX_RECURSIONS=16 include(../../src/src.pri) diff --git a/tests/parser/tst_parser.cpp b/tests/parser/tst_parser.cpp index 57e4642..0749bc8 100644 --- a/tests/parser/tst_parser.cpp +++ b/tests/parser/tst_parser.cpp @@ -76,6 +76,8 @@ private slots: void resumeParsing(); void endPointer_data(); void endPointer(); + void recursionLimit_data(); + void recursionLimit(); }; char toHexUpper(unsigned n) @@ -1316,5 +1318,75 @@ void tst_Parser::endPointer() QCOMPARE(int(first.ptr - data.constBegin()), offset); } +void tst_Parser::recursionLimit_data() +{ + static const int recursions = CBOR_PARSER_MAX_RECURSIONS + 2; + QTest::addColumn("data"); + + QTest::newRow("array") << QByteArray(recursions, '\x81') + '\x20'; + QTest::newRow("_array") << QByteArray(recursions, '\x9f') + '\x20' + QByteArray(recursions, '\xff'); + + QByteArray data; + for (int i = 0; i < recursions; ++i) + data += "\xa1\x65Hello"; + data += '\2'; + QTest::newRow("map-recursive-values") << data; + + data.clear(); + for (int i = 0; i < recursions; ++i) + data += "\xbf\x65World"; + data += '\2'; + for (int i = 0; i < recursions; ++i) + data += "\xff"; + QTest::newRow("_map-recursive-values") << data; + + data = QByteArray(recursions, '\xa1'); + data += '\2'; + for (int i = 0; i < recursions; ++i) + data += "\x7f\x64quux\xff"; + QTest::newRow("map-recursive-keys") << data; + + data = QByteArray(recursions, '\xbf'); + data += '\2'; + for (int i = 0; i < recursions; ++i) + data += "\1\xff"; + QTest::newRow("_map-recursive-keys") << data; + + data.clear(); + for (int i = 0; i < recursions / 2; ++i) + data += "\x81\xa1\1"; + data += '\2'; + QTest::newRow("mixed") << data; +} + +void tst_Parser::recursionLimit() +{ + QFETCH(QByteArray, data); + + CborParser parser; + CborValue first; + CborError err = cbor_parser_init(data.constData(), data.length(), 0, &parser, &first); + QVERIFY2(!err, QByteArray("Got error \"") + cbor_error_string(err) + "\""); + + // check that it is valid: + CborValue it = first; + { + QString dummy; + err = parseOne(&it, &dummy); + QVERIFY2(!err, QByteArray("Got error \"") + cbor_error_string(err) + "\""); + } + + it = first; + err = cbor_value_advance(&it); + QCOMPARE(int(err), int(CborErrorNestingTooDeep)); + + it = first; + if (cbor_value_is_map(&it)) { + CborValue dummy; + err = cbor_value_map_find_value(&it, "foo", &dummy); + QCOMPARE(int(err), int(CborErrorNestingTooDeep)); + } +} + QTEST_MAIN(tst_Parser) #include "tst_parser.moc"