From a0e02cb3ead79376db6d1f51d3d6219de5d8a0c9 Mon Sep 17 00:00:00 2001 From: Charles Munson Date: Wed, 23 Mar 2016 14:07:52 +0100 Subject: [PATCH 1/2] Bugfix: Possible Out of Bounds Write in StreamNameIO and BlockNameIO #15 Issue #15, the encodeName functions fail to verify buffer length can store encoded filenames For good measure and interface consistency, also check decodeName fnc --- encfs/BlockNameIO.cpp | 8 ++++++-- encfs/BlockNameIO.h | 4 ++-- encfs/NameIO.cpp | 31 +++++++++++++++++-------------- encfs/NameIO.h | 28 +++++++++++++++++++--------- encfs/NullNameIO.cpp | 8 ++++++-- encfs/NullNameIO.h | 6 ++++-- encfs/StreamNameIO.cpp | 7 +++++-- encfs/StreamNameIO.h | 4 ++-- 8 files changed, 61 insertions(+), 35 deletions(-) diff --git a/encfs/BlockNameIO.cpp b/encfs/BlockNameIO.cpp index 863d68c..075ae76 100644 --- a/encfs/BlockNameIO.cpp +++ b/encfs/BlockNameIO.cpp @@ -135,14 +135,17 @@ int BlockNameIO::maxDecodedNameLen(int encodedNameLen) const { } int BlockNameIO::encodeName(const char *plaintextName, int length, uint64_t *iv, - char *encodedName) const { + char *encodedName, int bufferLength) const { + // copy the data into the encoding buffer.. + rAssert(length <= (bufferLength - 2)); memcpy(encodedName + 2, plaintextName, length); // Pad encryption buffer to block boundary.. int padding = _bs - length % _bs; if (padding == 0) padding = _bs; // padding a full extra block! + rAssert(padding <= (bufferLength - length - 2)); memset(encodedName + length + 2, (unsigned char)padding, padding); // store the IV before it is modified by the MAC call. @@ -182,7 +185,7 @@ int BlockNameIO::encodeName(const char *plaintextName, int length, uint64_t *iv, } int BlockNameIO::decodeName(const char *encodedName, int length, uint64_t *iv, - char *plaintextName) const { + char *plaintextName, int bufferLength) const { int decLen256 = _caseSensitive ? B32ToB256Bytes(length) : B64ToB256Bytes(length); int decodedStreamLen = decLen256 - 2; @@ -225,6 +228,7 @@ int BlockNameIO::decodeName(const char *encodedName, int length, uint64_t *iv, } // copy out the result.. + rAssert(finalSize < bufferLength); memcpy(plaintextName, tmpBuf + 2, finalSize); plaintextName[finalSize] = '\0'; diff --git a/encfs/BlockNameIO.h b/encfs/BlockNameIO.h index 3f51257..fd2f481 100644 --- a/encfs/BlockNameIO.h +++ b/encfs/BlockNameIO.h @@ -55,9 +55,9 @@ class BlockNameIO : public NameIO { protected: virtual int encodeName(const char *plaintextName, int length, uint64_t *iv, - char *encodedName) const; + char *encodedName, int bufferLength) const; virtual int decodeName(const char *encodedName, int length, uint64_t *iv, - char *plaintextName) const; + char *plaintextName, int bufferLength) const; private: int _interface; diff --git a/encfs/NameIO.cpp b/encfs/NameIO.cpp index 682c089..ebf8c23 100644 --- a/encfs/NameIO.cpp +++ b/encfs/NameIO.cpp @@ -142,7 +142,7 @@ bool NameIO::getReverseEncryption() const { return reverseEncryption; } std::string NameIO::recodePath(const char *path, int (NameIO::*_length)(int) const, int (NameIO::*_code)(const char *, int, - uint64_t *, char *) const, + uint64_t *, char *, int) const, uint64_t *iv) const { string output; @@ -166,11 +166,12 @@ std::string NameIO::recodePath(const char *path, // figure out buffer sizes int approxLen = (this->*_length)(len); if (approxLen <= 0) throw ERROR("Filename too small to decode"); + int bufSize = 0; - BUFFER_INIT(codeBuf, 32, (unsigned int)approxLen + 1) - + BUFFER_INIT_S(codeBuf, 32, (unsigned int)approxLen + 1, bufSize) + // code the name - int codedLen = (this->*_code)(path, len, iv, codeBuf); + int codedLen = (this->*_code)(path, len, iv, codeBuf, bufSize); rAssert(codedLen <= approxLen); rAssert(codeBuf[codedLen] == '\0'); path += len; @@ -217,21 +218,22 @@ std::string NameIO::decodePath(const char *path, uint64_t *iv) const { return getReverseEncryption() ? _encodePath(path, iv) : _decodePath(path, iv); } -int NameIO::encodeName(const char *input, int length, char *output) const { - return encodeName(input, length, (uint64_t *)0, output); +int NameIO::encodeName(const char *input, int length, char *output, int bufferLength) const { + return encodeName(input, length, (uint64_t *)0, output, bufferLength); } -int NameIO::decodeName(const char *input, int length, char *output) const { - return decodeName(input, length, (uint64_t *)0, output); +int NameIO::decodeName(const char *input, int length, char *output, int bufferLength) const { + return decodeName(input, length, (uint64_t *)0, output, bufferLength); } std::string NameIO::_encodeName(const char *plaintextName, int length) const { int approxLen = maxEncodedNameLen(length); + int bufSize = 0; - BUFFER_INIT(codeBuf, 32, (unsigned int)approxLen + 1) - + BUFFER_INIT_S(codeBuf, 32, (unsigned int)approxLen + 1, bufSize) + // code the name - int codedLen = encodeName(plaintextName, length, 0, codeBuf); + int codedLen = encodeName(plaintextName, length, 0, codeBuf, bufSize); rAssert(codedLen <= approxLen); rAssert(codeBuf[codedLen] == '\0'); @@ -245,11 +247,12 @@ std::string NameIO::_encodeName(const char *plaintextName, int length) const { std::string NameIO::_decodeName(const char *encodedName, int length) const { int approxLen = maxDecodedNameLen(length); + int bufSize = 0; - BUFFER_INIT(codeBuf, 32, (unsigned int)approxLen + 1) - + BUFFER_INIT_S(codeBuf, 32, (unsigned int)approxLen + 1, bufSize) + // code the name - int codedLen = decodeName(encodedName, length, 0, codeBuf); + int codedLen = decodeName(encodedName, length, 0, codeBuf, bufSize); rAssert(codedLen <= approxLen); rAssert(codeBuf[codedLen] == '\0'); diff --git a/encfs/NameIO.h b/encfs/NameIO.h index 916c2c2..7f7ecb3 100644 --- a/encfs/NameIO.h +++ b/encfs/NameIO.h @@ -83,19 +83,19 @@ class NameIO { protected: virtual int encodeName(const char *plaintextName, int length, - char *encodedName) const; + char *encodedName, int bufferLength) const; virtual int decodeName(const char *encodedName, int length, - char *plaintextName) const; + char *plaintextName, int bufferLength) const; virtual int encodeName(const char *plaintextName, int length, uint64_t *iv, - char *encodedName) const = 0; + char *encodedName, int bufferLength) const = 0; virtual int decodeName(const char *encodedName, int length, uint64_t *iv, - char *plaintextName) const = 0; + char *plaintextName, int bufferLength) const = 0; private: std::string recodePath(const char *path, int (NameIO::*codingLen)(int) const, int (NameIO::*codingFunc)(const char *, int, - uint64_t *, char *) const, + uint64_t *, char *, int) const, uint64_t *iv) const; std::string _encodePath(const char *plaintextPath, uint64_t *iv) const; @@ -108,11 +108,11 @@ class NameIO { }; /* - Helper macros for creating temporary buffers with an optimization that - below a given size (OptimizedSize) is allocated on the stack, and when a - larger size is requested it is allocated on the heap. + Helper macros for creating temporary buffers with an optimization that + below a given size (OptimizedSize) is allocated on the stack, and when a + larger size is requested it is allocated on the heap. - BUFFER_RESET should be called for the same name as BUFFER_INIT + BUFFER_RESET should be called for the same name as BUFFER_INIT */ #define BUFFER_INIT(Name, OptimizedSize, Size) \ char Name##_Raw[OptimizedSize]; \ @@ -120,6 +120,16 @@ class NameIO { if (sizeof(Name##_Raw) < Size) Name = new char[Size]; \ memset(Name, 0, Size); +#define BUFFER_INIT_S(Name, OptimizedSize, Size, BufSize) \ + char Name##_Raw[OptimizedSize]; \ + BufSize = sizeof(Name##_Raw); \ + char *Name = Name##_Raw; \ + if (sizeof(Name##_Raw) < Size) { \ + Name = new char[Size]; \ + BufSize = Size; \ + } \ + memset(Name, 0, Size); + #define BUFFER_RESET(Name) \ do { \ if (Name != Name##_Raw) { \ diff --git a/encfs/NullNameIO.cpp b/encfs/NullNameIO.cpp index ec18a4f..fada7bf 100644 --- a/encfs/NullNameIO.cpp +++ b/encfs/NullNameIO.cpp @@ -56,16 +56,20 @@ int NullNameIO::maxDecodedNameLen(int encodedNameLen) const { } int NullNameIO::encodeName(const char *plaintextName, int length, uint64_t *iv, - char *encodedName) const { + char *encodedName, int bufferLength) const { (void)iv; + + rAssert(length <= bufferLength); memcpy(encodedName, plaintextName, length); return length; } int NullNameIO::decodeName(const char *encodedName, int length, uint64_t *iv, - char *plaintextName) const { + char *plaintextName, int bufferLength) const { (void)iv; + + rAssert(length <= bufferLength); memcpy(plaintextName, encodedName, length); return length; diff --git a/encfs/NullNameIO.h b/encfs/NullNameIO.h index 5591e18..ab86f9a 100644 --- a/encfs/NullNameIO.h +++ b/encfs/NullNameIO.h @@ -23,6 +23,8 @@ #include +#include "rlog/Error.h" +#include "rlog/rlog.h" #include "Interface.h" #include "NameIO.h" @@ -44,9 +46,9 @@ class NullNameIO : public NameIO { protected: virtual int encodeName(const char *plaintextName, int length, uint64_t *iv, - char *encodedName) const; + char *encodedName, int bufferLength) const; virtual int decodeName(const char *encodedName, int length, uint64_t *iv, - char *plaintextName) const; + char *plaintextName, int bufferLength) const; private: }; diff --git a/encfs/StreamNameIO.cpp b/encfs/StreamNameIO.cpp index c111198..a14b00b 100644 --- a/encfs/StreamNameIO.cpp +++ b/encfs/StreamNameIO.cpp @@ -90,7 +90,7 @@ int StreamNameIO::maxDecodedNameLen(int encodedStreamLen) const { } int StreamNameIO::encodeName(const char *plaintextName, int length, - uint64_t *iv, char *encodedName) const { + uint64_t *iv, char *encodedName, int bufferLength) const { uint64_t tmpIV = 0; if (iv && _interface >= 2) tmpIV = *iv; @@ -104,11 +104,13 @@ int StreamNameIO::encodeName(const char *plaintextName, int length, encodedName[0] = (mac >> 8) & 0xff; encodedName[1] = (mac)&0xff; encodeBegin = (unsigned char *)encodedName + 2; + rAssert(length <= (bufferLength - 2)); } else { // encfs 0.x stored checksums at the end. encodedName[length] = (mac >> 8) & 0xff; encodedName[length + 1] = (mac)&0xff; encodeBegin = (unsigned char *)encodedName; + rAssert(length <= bufferLength); } // stream encode the plaintext bytes @@ -126,10 +128,11 @@ int StreamNameIO::encodeName(const char *plaintextName, int length, } int StreamNameIO::decodeName(const char *encodedName, int length, uint64_t *iv, - char *plaintextName) const { + char *plaintextName, int bufferLength) const { rAssert(length > 2); int decLen256 = B64ToB256Bytes(length); int decodedStreamLen = decLen256 - 2; + rAssert(decodedStreamLen <= bufferLength); if (decodedStreamLen <= 0) throw ERROR("Filename too small to decode"); diff --git a/encfs/StreamNameIO.h b/encfs/StreamNameIO.h index 723fc6d..32a5327 100644 --- a/encfs/StreamNameIO.h +++ b/encfs/StreamNameIO.h @@ -48,9 +48,9 @@ class StreamNameIO : public NameIO { protected: virtual int encodeName(const char *plaintextName, int length, uint64_t *iv, - char *encodedName) const; + char *encodedName, int bufferLength) const; virtual int decodeName(const char *encodedName, int length, uint64_t *iv, - char *plaintextName) const; + char *plaintextName, int bufferLength) const; private: int _interface; From 6db69c2b0ac0821673e8f68ead0d1076938c3d1b Mon Sep 17 00:00:00 2001 From: Charles Munson Date: Thu, 24 Mar 2016 16:34:52 +0100 Subject: [PATCH 2/2] Consolidate rAssert statements in BlockNameIO Whitespace cleanup StreamNameIO rAssert statements should be the same --- encfs/BlockNameIO.cpp | 9 ++++----- encfs/NameIO.cpp | 8 ++++---- encfs/NameIO.h | 8 ++++---- encfs/NullNameIO.cpp | 2 +- encfs/StreamNameIO.cpp | 3 +-- 5 files changed, 14 insertions(+), 16 deletions(-) diff --git a/encfs/BlockNameIO.cpp b/encfs/BlockNameIO.cpp index 075ae76..ce189a9 100644 --- a/encfs/BlockNameIO.cpp +++ b/encfs/BlockNameIO.cpp @@ -137,17 +137,16 @@ int BlockNameIO::maxDecodedNameLen(int encodedNameLen) const { int BlockNameIO::encodeName(const char *plaintextName, int length, uint64_t *iv, char *encodedName, int bufferLength) const { - // copy the data into the encoding buffer.. - rAssert(length <= (bufferLength - 2)); - memcpy(encodedName + 2, plaintextName, length); - // Pad encryption buffer to block boundary.. int padding = _bs - length % _bs; if (padding == 0) padding = _bs; // padding a full extra block! - rAssert(padding <= (bufferLength - length - 2)); + rAssert(bufferLength >= length + 2 + padding); memset(encodedName + length + 2, (unsigned char)padding, padding); + // copy the data into the encoding buffer.. + memcpy(encodedName + 2, plaintextName, length); + // store the IV before it is modified by the MAC call. uint64_t tmpIV = 0; if (iv && _interface >= 3) tmpIV = *iv; diff --git a/encfs/NameIO.cpp b/encfs/NameIO.cpp index ebf8c23..a7d8611 100644 --- a/encfs/NameIO.cpp +++ b/encfs/NameIO.cpp @@ -166,10 +166,10 @@ std::string NameIO::recodePath(const char *path, // figure out buffer sizes int approxLen = (this->*_length)(len); if (approxLen <= 0) throw ERROR("Filename too small to decode"); - int bufSize = 0; + int bufSize = 0; BUFFER_INIT_S(codeBuf, 32, (unsigned int)approxLen + 1, bufSize) - + // code the name int codedLen = (this->*_code)(path, len, iv, codeBuf, bufSize); rAssert(codedLen <= approxLen); @@ -231,7 +231,7 @@ std::string NameIO::_encodeName(const char *plaintextName, int length) const { int bufSize = 0; BUFFER_INIT_S(codeBuf, 32, (unsigned int)approxLen + 1, bufSize) - + // code the name int codedLen = encodeName(plaintextName, length, 0, codeBuf, bufSize); rAssert(codedLen <= approxLen); @@ -250,7 +250,7 @@ std::string NameIO::_decodeName(const char *encodedName, int length) const { int bufSize = 0; BUFFER_INIT_S(codeBuf, 32, (unsigned int)approxLen + 1, bufSize) - + // code the name int codedLen = decodeName(encodedName, length, 0, codeBuf, bufSize); rAssert(codedLen <= approxLen); diff --git a/encfs/NameIO.h b/encfs/NameIO.h index 7f7ecb3..726325f 100644 --- a/encfs/NameIO.h +++ b/encfs/NameIO.h @@ -108,11 +108,11 @@ class NameIO { }; /* - Helper macros for creating temporary buffers with an optimization that - below a given size (OptimizedSize) is allocated on the stack, and when a - larger size is requested it is allocated on the heap. + Helper macros for creating temporary buffers with an optimization that + below a given size (OptimizedSize) is allocated on the stack, and when a + larger size is requested it is allocated on the heap. - BUFFER_RESET should be called for the same name as BUFFER_INIT + BUFFER_RESET should be called for the same name as BUFFER_INIT */ #define BUFFER_INIT(Name, OptimizedSize, Size) \ char Name##_Raw[OptimizedSize]; \ diff --git a/encfs/NullNameIO.cpp b/encfs/NullNameIO.cpp index fada7bf..1dbcece 100644 --- a/encfs/NullNameIO.cpp +++ b/encfs/NullNameIO.cpp @@ -58,7 +58,7 @@ int NullNameIO::maxDecodedNameLen(int encodedNameLen) const { int NullNameIO::encodeName(const char *plaintextName, int length, uint64_t *iv, char *encodedName, int bufferLength) const { (void)iv; - + rAssert(length <= bufferLength); memcpy(encodedName, plaintextName, length); diff --git a/encfs/StreamNameIO.cpp b/encfs/StreamNameIO.cpp index a14b00b..17efe36 100644 --- a/encfs/StreamNameIO.cpp +++ b/encfs/StreamNameIO.cpp @@ -99,18 +99,17 @@ int StreamNameIO::encodeName(const char *plaintextName, int length, // add on checksum bytes unsigned char *encodeBegin; + rAssert(bufferLength >= length + 2); if (_interface >= 1) { // current versions store the checksum at the beginning encodedName[0] = (mac >> 8) & 0xff; encodedName[1] = (mac)&0xff; encodeBegin = (unsigned char *)encodedName + 2; - rAssert(length <= (bufferLength - 2)); } else { // encfs 0.x stored checksums at the end. encodedName[length] = (mac >> 8) & 0xff; encodedName[length + 1] = (mac)&0xff; encodeBegin = (unsigned char *)encodedName; - rAssert(length <= bufferLength); } // stream encode the plaintext bytes