From 5e7b9ec688d79e7b16ec7064e1d37e8481a31e72 Mon Sep 17 00:00:00 2001 From: Giancarlo Canales Barreto Date: Wed, 10 Jun 2015 21:31:22 +0200 Subject: [PATCH 1/3] Fix buffer overflow (pull request #81) --- CHANGELOG.md | 7 ++++++ src/Internals/QuotedString.cpp | 32 ++++++++++++------------- test/QuotedString_ExtractFrom_Tests.cpp | 10 ++++++++ 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 01c7cc98..d43202a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ Arduino JSON: change log ======================== +v4.5 +---- + +* Fixed buffer overflow when input contains a backslash followed by a terminator (issue #81) + +**Upgrading is recommended** since previous versions contain a potential security risk. + v4.4 ---- diff --git a/src/Internals/QuotedString.cpp b/src/Internals/QuotedString.cpp index 3a90defc..45a6c488 100644 --- a/src/Internals/QuotedString.cpp +++ b/src/Internals/QuotedString.cpp @@ -58,46 +58,44 @@ static char unescapeChar(char c) { static inline bool isQuote(char c) { return c == '\"' || c == '\''; } char *QuotedString::extractFrom(char *input, char **endPtr) { - char firstChar = *input; - - if (!isQuote(firstChar)) { - // must start with a quote - return NULL; - } - - char stopChar = firstChar; // closing quote is the same as opening quote - char *startPtr = input + 1; // skip the quote char *readPtr = startPtr; char *writePtr = startPtr; char c; + char firstChar = *input; + char stopChar = firstChar; // closing quote is the same as opening quote + + if (!isQuote(firstChar)) goto ERROR_OPENING_QUOTE_MISSING; + for (;;) { c = *readPtr++; - if (c == '\0') { - // premature ending - return NULL; - } + if (c == '\0') goto ERROR_CLOSING_QUOTE_MISSING; - if (c == stopChar) { - // closing quote - break; - } + if (c == stopChar) goto SUCCESS; if (c == '\\') { // replace char c = unescapeChar(*readPtr++); + if (c == '\0') goto ERROR_ESCAPE_SEQUENCE_INTERRUPTED; } *writePtr++ = c; } +SUCCESS: // end the string here *writePtr = '\0'; // update end ptr *endPtr = readPtr; + // return pointer to unquoted string return startPtr; + +ERROR_OPENING_QUOTE_MISSING: +ERROR_CLOSING_QUOTE_MISSING: +ERROR_ESCAPE_SEQUENCE_INTERRUPTED: + return NULL; } diff --git a/test/QuotedString_ExtractFrom_Tests.cpp b/test/QuotedString_ExtractFrom_Tests.cpp index 2f6747f3..94d06261 100644 --- a/test/QuotedString_ExtractFrom_Tests.cpp +++ b/test/QuotedString_ExtractFrom_Tests.cpp @@ -16,6 +16,11 @@ class QuotedString_ExtractFrom_Tests : public testing::Test { _result = QuotedString::extractFrom(_jsonString, &_trailing); } + void whenInputIs(const char *json, size_t len) { + memcpy(_jsonString, json, len); + _result = QuotedString::extractFrom(_jsonString, &_trailing); + } + void resultMustBe(const char *expected) { EXPECT_STREQ(expected, _result); } void trailingMustBe(const char *expected) { @@ -134,3 +139,8 @@ TEST_F(QuotedString_ExtractFrom_Tests, AllEscapedCharsTogether) { whenInputIs("\"1\\\"2\\\\3\\/4\\b5\\f6\\n7\\r8\\t9\""); resultMustBe("1\"2\\3/4\b5\f6\n7\r8\t9"); } + +TEST_F(QuotedString_ExtractFrom_Tests, UnterminatedEscapeSequence) { + whenInputIs("\"\\\0\"", 4); + resultMustBe(0); +} From f5b83f9314bc85fe6143e993f528f84d4f27f78a Mon Sep 17 00:00:00 2001 From: Benoit Blanchon Date: Sun, 14 Jun 2015 15:26:33 +0200 Subject: [PATCH 2/3] Added greeting to Giancarlo Canales Barreto --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d43202a1..1246742b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ v4.5 **Upgrading is recommended** since previous versions contain a potential security risk. +Special thanks to [Giancarlo Canales Barreto](https://github.com/gcanalesb) for finding this nasty bug. + v4.4 ---- From 2524a00a9661fcf2a1ad3affea4d1742226fa2e3 Mon Sep 17 00:00:00 2001 From: Benoit Blanchon Date: Sat, 1 Aug 2015 16:31:59 +0200 Subject: [PATCH 3/3] Fixed segmentation fault in `DynamicJsonBuffer` when memory allocation fails (issue #92) --- .clang-format | 3 + .gitattributes | 1 + CHANGELOG.md | 5 + include/ArduinoJson/DynamicJsonBuffer.hpp | 35 +------ .../ArduinoJson/Internals/BlockJsonBuffer.hpp | 93 +++++++++++++++++++ scripts/build-arduino-package.sh | 43 +++++---- src/DynamicJsonBuffer.cpp | 79 ---------------- test/DynamicJsonBuffer_NoMemory_Tests.cpp | 37 ++++++++ 8 files changed, 163 insertions(+), 133 deletions(-) create mode 100644 .clang-format create mode 100644 .gitattributes create mode 100644 include/ArduinoJson/Internals/BlockJsonBuffer.hpp delete mode 100644 src/DynamicJsonBuffer.cpp create mode 100644 test/DynamicJsonBuffer_NoMemory_Tests.cpp diff --git a/.clang-format b/.clang-format new file mode 100644 index 00000000..0f553e88 --- /dev/null +++ b/.clang-format @@ -0,0 +1,3 @@ +# http://clang.llvm.org/docs/ClangFormatStyleOptions.html + +BasedOnStyle: Google \ No newline at end of file diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..526c8a38 --- /dev/null +++ b/.gitattributes @@ -0,0 +1 @@ +*.sh text eol=lf \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 1246742b..deba5c1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,11 @@ Arduino JSON: change log ======================== +v4.6 +---- + +* Fixed segmentation fault in `DynamicJsonBuffer` when memory allocation fails (issue #92) + v4.5 ---- diff --git a/include/ArduinoJson/DynamicJsonBuffer.hpp b/include/ArduinoJson/DynamicJsonBuffer.hpp index 667c52d9..a57f8dff 100644 --- a/include/ArduinoJson/DynamicJsonBuffer.hpp +++ b/include/ArduinoJson/DynamicJsonBuffer.hpp @@ -6,41 +6,12 @@ #pragma once -#include "JsonBuffer.hpp" - -#include +#include "Internals/BlockJsonBuffer.hpp" namespace ArduinoJson { - -// Forward declaration -namespace Internals { -struct DynamicJsonBufferBlock; -} - // Implements a JsonBuffer with dynamic memory allocation. // You are strongly encouraged to consider using StaticJsonBuffer which is much // more suitable for embedded systems. -class DynamicJsonBuffer : public JsonBuffer { - public: - DynamicJsonBuffer(); - ~DynamicJsonBuffer(); - - size_t size() const; - - protected: - virtual void* alloc(size_t bytes); - - private: - typedef Internals::DynamicJsonBufferBlock Block; - - static const size_t FIRST_BLOCK_CAPACITY = 32; - - static Block* createBlock(size_t capacity); - - inline bool canAllocInHead(size_t bytes) const; - inline void* allocInHead(size_t bytes); - inline void addNewBlock(); - - Block* _head; -}; +typedef Internals::BlockJsonBuffer + DynamicJsonBuffer; } diff --git a/include/ArduinoJson/Internals/BlockJsonBuffer.hpp b/include/ArduinoJson/Internals/BlockJsonBuffer.hpp new file mode 100644 index 00000000..a156f659 --- /dev/null +++ b/include/ArduinoJson/Internals/BlockJsonBuffer.hpp @@ -0,0 +1,93 @@ +// Copyright Benoit Blanchon 2014-2015 +// MIT License +// +// Arduino JSON library +// https://github.com/bblanchon/ArduinoJson + +#pragma once + +#include "../JsonBuffer.hpp" + +#include + +namespace ArduinoJson { +namespace Internals { +class DefaultAllocator { + public: + void* allocate(size_t size) { return malloc(size); } + void deallocate(void* pointer) { free(pointer); } +}; + +template +class BlockJsonBuffer : public JsonBuffer { + struct Block; + struct EmptyBlock { + Block* next; + size_t capacity; + size_t size; + }; + struct Block : EmptyBlock { + uint8_t data[1]; + }; + + public: + BlockJsonBuffer() : _head(NULL) {} + + ~BlockJsonBuffer() { + Block* currentBlock = _head; + + while (currentBlock != NULL) { + Block* nextBlock = currentBlock->next; + _allocator.deallocate(currentBlock); + currentBlock = nextBlock; + } + } + + size_t size() const { + size_t total = 0; + for (const Block* b = _head; b; b = b->next) total += b->size; + return total; + } + + protected: + virtual void* alloc(size_t bytes) { + return canAllocInHead(bytes) ? allocInHead(bytes) : allocInNewBlock(bytes); + } + + private: + static const size_t FIRST_BLOCK_CAPACITY = 32; + + bool canAllocInHead(size_t bytes) const { + return _head != NULL && _head->size + bytes <= _head->capacity; + } + + void* allocInHead(size_t bytes) { + void* p = _head->data + _head->size; + _head->size += bytes; + return p; + } + + void* allocInNewBlock(size_t bytes) { + size_t capacity = FIRST_BLOCK_CAPACITY; + if (_head != NULL) capacity = _head->capacity * 2; + if (bytes > capacity) capacity = bytes; + if (!addNewBlock(capacity)) return NULL; + return allocInHead(bytes); + } + + bool addNewBlock(size_t capacity) { + size_t size = sizeof(EmptyBlock) + capacity; + Block* block = static_cast(_allocator.allocate(size)); + if (block == NULL) return false; + block->capacity = capacity; + block->size = 0; + block->next = _head; + _head = block; + return true; + } + + Block* _head; + TAllocator _allocator; +}; +} +} diff --git a/scripts/build-arduino-package.sh b/scripts/build-arduino-package.sh index f91481a4..dd05fb1c 100755 --- a/scripts/build-arduino-package.sh +++ b/scripts/build-arduino-package.sh @@ -1,23 +1,22 @@ -#!/bin/bash - -ZIP="C:\Program Files\7-Zip\7z.exe" -TAG=$(git describe) -OUTPUT="ArduinoJson-$TAG.zip" - -cd ../.. - -# remove existing file -rm -f $OUTPUT - -# create zip -"$ZIP" a $OUTPUT \ - ArduinoJson/CHANGELOG.md \ - ArduinoJson/examples \ - ArduinoJson/include \ - ArduinoJson/keywords.txt \ - ArduinoJson/LICENSE.md \ - ArduinoJson/README.md \ - ArduinoJson/src \ - ArduinoJson/ArduinoJson.h \ - ArduinoJson/ArduinoJson.cpp \ +#!/bin/bash + +TAG=$(git describe) +OUTPUT="ArduinoJson-$TAG.zip" + +cd ../.. + +# remove existing file +rm -f $OUTPUT + +# create zip +7z a $OUTPUT \ + ArduinoJson/CHANGELOG.md \ + ArduinoJson/examples \ + ArduinoJson/include \ + ArduinoJson/keywords.txt \ + ArduinoJson/LICENSE.md \ + ArduinoJson/README.md \ + ArduinoJson/src \ + ArduinoJson/ArduinoJson.h \ + ArduinoJson/ArduinoJson.cpp \ -x!ArduinoJson/src/CMakeLists.txt \ No newline at end of file diff --git a/src/DynamicJsonBuffer.cpp b/src/DynamicJsonBuffer.cpp deleted file mode 100644 index f0fb9393..00000000 --- a/src/DynamicJsonBuffer.cpp +++ /dev/null @@ -1,79 +0,0 @@ -// Copyright Benoit Blanchon 2014-2015 -// MIT License -// -// Arduino JSON library -// https://github.com/bblanchon/ArduinoJson - -#include "../include/ArduinoJson/DynamicJsonBuffer.hpp" - -namespace ArduinoJson { -namespace Internals { -struct DynamicJsonBufferBlockWithoutData { - DynamicJsonBufferBlock* next; - size_t capacity; - size_t size; -}; - - -struct DynamicJsonBufferBlock : DynamicJsonBufferBlockWithoutData { - uint8_t data[1]; -}; -} -} - -using namespace ArduinoJson; -using namespace ArduinoJson::Internals; - -DynamicJsonBuffer::DynamicJsonBuffer() { - _head = createBlock(FIRST_BLOCK_CAPACITY); -} - -DynamicJsonBuffer::~DynamicJsonBuffer() { - Block* currentBlock = _head; - - while (currentBlock != NULL) { - Block* nextBlock = currentBlock->next; - free(currentBlock); - currentBlock = nextBlock; - } -} - -size_t DynamicJsonBuffer::size() const { - size_t total = 0; - - for (const Block* b = _head; b != NULL; b = b->next) { - total += b->size; - } - - return total; -} - -void* DynamicJsonBuffer::alloc(size_t bytes) { - if (!canAllocInHead(bytes)) addNewBlock(); - return allocInHead(bytes); -} - -bool DynamicJsonBuffer::canAllocInHead(size_t bytes) const { - return _head->size + bytes <= _head->capacity; -} - -void* DynamicJsonBuffer::allocInHead(size_t bytes) { - void* p = _head->data + _head->size; - _head->size += bytes; - return p; -} - -void DynamicJsonBuffer::addNewBlock() { - Block* block = createBlock(_head->capacity * 2); - block->next = _head; - _head = block; -} - -DynamicJsonBuffer::Block* DynamicJsonBuffer::createBlock(size_t capacity) { - size_t blkSize = sizeof(DynamicJsonBufferBlockWithoutData) + capacity; - Block* block = static_cast(malloc(blkSize)); - block->capacity = capacity; - block->size = 0; - block->next = NULL; - return block; -} diff --git a/test/DynamicJsonBuffer_NoMemory_Tests.cpp b/test/DynamicJsonBuffer_NoMemory_Tests.cpp new file mode 100644 index 00000000..cecfc5a5 --- /dev/null +++ b/test/DynamicJsonBuffer_NoMemory_Tests.cpp @@ -0,0 +1,37 @@ +// Copyright Benoit Blanchon 2014-2015 +// MIT License +// +// Arduino JSON library +// https://github.com/bblanchon/ArduinoJson + +#include +#include + +class DynamicJsonBuffer_NoMemory_Tests : public ::testing::Test { + class NoMemoryAllocator { + public: + void* allocate(size_t) { return NULL; } + void deallocate(void*) {} + }; + + protected: + Internals::BlockJsonBuffer _jsonBuffer; +}; + +TEST_F(DynamicJsonBuffer_NoMemory_Tests, CreateArray) { + ASSERT_FALSE(_jsonBuffer.createArray().success()); +} + +TEST_F(DynamicJsonBuffer_NoMemory_Tests, CreateObject) { + ASSERT_FALSE(_jsonBuffer.createObject().success()); +} + +TEST_F(DynamicJsonBuffer_NoMemory_Tests, ParseArray) { + char json[] = "[]"; + ASSERT_FALSE(_jsonBuffer.parseArray(json).success()); +} + +TEST_F(DynamicJsonBuffer_NoMemory_Tests, ParseObject) { + char json[] = "{}"; + ASSERT_FALSE(_jsonBuffer.parseObject(json).success()); +}