diff --git a/src/vmime/utility/encoder/uuEncoder.cpp b/src/vmime/utility/encoder/uuEncoder.cpp index 24dcdc8b..87d6f703 100644 --- a/src/vmime/utility/encoder/uuEncoder.cpp +++ b/src/vmime/utility/encoder/uuEncoder.cpp @@ -179,8 +179,12 @@ size_t uuEncoder::decode( break; } - const size_t outLength = UUDECODE(lengthChar); - const size_t inLength = std::min((outLength * 4) / 3, static_cast (64)); + // A line holds at most 45 bytes of data. Clamp the decoded length so + // that the encoded line (4 characters per group of 3 bytes, the last + // group possibly being incomplete) always fits in 'inBuffer'. + const size_t outLength = + std::min(static_cast (UUDECODE(lengthChar)), static_cast (48)); + const size_t inLength = ((outLength + 2) / 3) * 4; size_t inPos = 0; switch (lengthChar) { @@ -207,12 +211,14 @@ size_t uuEncoder::decode( inTotal += 5; - byte_t c = 0; + // The space we just read may already be the end of the line + // ("begin\n", with no mode nor filename) + byte_t c = inBuffer[4]; size_t count = 0; byte_t buffer[512]; - while (count < sizeof(buffer) - 1 && in.read(&c, 1) == 1) { + while (c != '\n' && count < sizeof(buffer) - 1 && in.read(&c, 1) == 1) { if (c == '\n') { break; @@ -243,7 +249,7 @@ size_t uuEncoder::decode( while (*p && parserHelpers::isSpace(*p)) ++p; - byte_t* modeStart = buffer; + byte_t* modeStart = p; while (*p && !parserHelpers::isSpace(*p)) ++p; @@ -251,7 +257,7 @@ size_t uuEncoder::decode( while (*p && parserHelpers::isSpace(*p)) ++p; - byte_t* filenameStart = buffer; + byte_t* filenameStart = p; while (*p && !(*p == '\r' || *p == '\n')) ++p; @@ -289,6 +295,14 @@ size_t uuEncoder::decode( } + // Looking for a "begin"/"end" marker above may have consumed more bytes + // than this line can hold: a 'b' line length means 4 encoded characters, + // but the "begin" probe reads 5 bytes before giving up. Stop decoding + // here, as 'inLength - inPos' would otherwise underflow. + if (inPos > inLength) { + break; + } + // Read encoded data if (in.read(inBuffer + inPos, inLength - inPos) != inLength - inPos) { // Premature end of data @@ -305,7 +319,7 @@ size_t uuEncoder::decode( const byte_t c3 = inBuffer[i + 2]; const byte_t c4 = inBuffer[i + 3]; - const size_t n = std::min(inLength - i, static_cast (3)); + const size_t n = std::min(outLength - j, static_cast (3)); if (n >= 3) { outBuffer[j + 2] = static_cast (UUDECODE(c3) << 6 | UUDECODE(c4)); diff --git a/src/vmime/utility/inputStreamStringAdapter.cpp b/src/vmime/utility/inputStreamStringAdapter.cpp index bf09a9b8..c79744a0 100644 --- a/src/vmime/utility/inputStreamStringAdapter.cpp +++ b/src/vmime/utility/inputStreamStringAdapter.cpp @@ -64,11 +64,11 @@ void inputStreamStringAdapter::reset() { size_t inputStreamStringAdapter::read(byte_t* const data, const size_t count) { - if (m_pos + count >= m_end) { + const size_t remaining = m_end - m_pos; - const size_t remaining = m_end - m_pos; + if (remaining <= count) { - std::copy(m_buffer.begin() + m_pos, m_buffer.end(), data); + std::copy(m_buffer.begin() + m_pos, m_buffer.begin() + m_end, data); m_pos = m_end; return remaining; @@ -85,9 +85,10 @@ size_t inputStreamStringAdapter::read(byte_t* const data, const size_t count) { size_t inputStreamStringAdapter::skip(const size_t count) { - if (m_pos + count >= m_end) { + const size_t remaining = m_end - m_pos; + + if (remaining <= count) { - const size_t remaining = m_end - m_pos; m_pos = m_end; return remaining; diff --git a/src/vmime/utility/seekableInputStreamRegionAdapter.cpp b/src/vmime/utility/seekableInputStreamRegionAdapter.cpp index 32723661..1ddb2f36 100644 --- a/src/vmime/utility/seekableInputStreamRegionAdapter.cpp +++ b/src/vmime/utility/seekableInputStreamRegionAdapter.cpp @@ -59,9 +59,10 @@ size_t seekableInputStreamRegionAdapter::read(byte_t* const data, const size_t c size_t readBytes = 0; - if (m_position + count >= m_length) { + const size_t remaining = m_length - m_position; + + if (remaining <= count) { - const size_t remaining = m_length - m_position; readBytes = m_stream->read(data, remaining); } else { @@ -77,9 +78,10 @@ size_t seekableInputStreamRegionAdapter::read(byte_t* const data, const size_t c size_t seekableInputStreamRegionAdapter::skip(const size_t count) { - if (m_position + count >= m_length) { + const size_t remaining = m_length - m_position; + + if (remaining <= count) { - const size_t remaining = m_length - m_position; m_position += remaining; return remaining; diff --git a/tests/utility/encoder/uuEncoderTest.cpp b/tests/utility/encoder/uuEncoderTest.cpp new file mode 100644 index 00000000..84e4bec3 --- /dev/null +++ b/tests/utility/encoder/uuEncoderTest.cpp @@ -0,0 +1,162 @@ +// +// VMime library (http://www.vmime.org) +// Copyright (C) 2002 Vincent Richard +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License as +// published by the Free Software Foundation; either version 3 of +// the License, or (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +// General Public License for more details. +// +// You should have received a copy of the GNU General Public License along +// with this program; if not, write to the Free Software Foundation, Inc., +// 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +// +// Linking this library statically or dynamically with other modules is making +// a combined work based on this library. Thus, the terms and conditions of +// the GNU General Public License cover the whole combination. +// + +#include "tests/testUtils.hpp" + +#include "encoderTestUtils.hpp" + + +VMIME_TEST_SUITE_BEGIN(uuEncoderTest) + + VMIME_TEST_LIST_BEGIN + VMIME_TEST(testEncodeDecode) + VMIME_TEST(testDecodeBeginMarker) + VMIME_TEST(testDecodeBeginMarkerWithoutModeAndFilename) + VMIME_TEST(testDecodeInvalidBeginMarker) + VMIME_TEST(testDecodeInvalidEndMarker) + VMIME_TEST(testDecodeOversizedLineLength) + VMIME_TEST(testDecodeTruncatedMarkers) + VMIME_TEST_LIST_END + + + void testEncodeDecode() { + + // Include lengths which are not a multiple of 3, so that the last group + // on a line is incomplete, and lengths which span several lines + static const unsigned int lengths[] = { 0, 1, 2, 3, 4, 44, 45, 46, 47, 100, 256 }; + + for (unsigned int i = 0 ; i < sizeof(lengths) / sizeof(lengths[0]) ; ++i) { + + vmime::string data; + + for (unsigned int j = 0 ; j < lengths[i] ; ++j) { + data += static_cast (j & 0xff); + } + + std::ostringstream oss; + oss << "length " << lengths[i]; + + VASSERT_EQ(oss.str(), data, decode("uuencode", encode("uuencode", data))); + } + } + + void testDecodeBeginMarker() { + + // A well-formed prelude is skipped, and its mode and filename are + // exposed in the encoder results + vmime::shared_ptr enc = getEncoder("uuencode"); + + vmime::utility::inputStreamStringAdapter in("begin 644 test.txt\r\nend\r\n"); + + std::ostringstream oss; + vmime::utility::outputStreamAdapter out(oss); + + enc->decode(in, out); + + const vmime::utility::encoder::encoder& cenc = *enc; + + VASSERT_EQ("data", "", oss.str()); + VASSERT_EQ("mode", "644", cenc.getResults().getProperty ("mode", "")); + VASSERT_EQ("filename", "test.txt", cenc.getResults().getProperty ("filename", "")); + } + + void testDecodeBeginMarkerWithoutModeAndFilename() { + + // With LF line endings, the space following "begin" is the end of the + // line itself: the next line holds data, not the mode and filename + vmime::shared_ptr enc = getEncoder("uuencode"); + + vmime::utility::inputStreamStringAdapter in("begin\n#86)C\nend\n"); + + std::ostringstream oss; + vmime::utility::outputStreamAdapter out(oss); + + enc->decode(in, out); + + const vmime::utility::encoder::encoder& cenc = *enc; + + VASSERT_EQ("data", "abc", oss.str()); + VASSERT_EQ("mode", "644", cenc.getResults().getProperty ("mode", "")); + VASSERT_EQ("filename", "untitled", cenc.getResults().getProperty ("filename", "")); + } + + /** A 'b' line length which does not introduce a "begin" line must not make + * the per-line read count underflow. + * + * 'b' decodes to a line length of 2, so the line holds 4 encoded characters, + * but the "begin" probe reads 5 bytes before giving up. 'inLength - inPos' + * then used to wrap around as an unsigned value, and the resulting + * near-SIZE_MAX read count drove a write past the decoder's 64-byte stack + * buffer (CWE-191/CWE-787). + */ + void testDecodeInvalidBeginMarker() { + + vmime::string data = "b"; + data += "ZZZZZ"; // not "egin ", so the probe leaves inPos == 5 + data.append(512, 'A'); // bytes the oversized read would have run over + + VASSERT_EQ("decoded", "", decode("uuencode", data)); + } + + /** Same shape, on the "end" marker. 'e' decodes to a line length of 5, ie. + * 8 encoded characters, and the probe only reads 3 of them, so this path + * does not underflow, but it must stay consistent. + */ + void testDecodeInvalidEndMarker() { + + // 3 characters are consumed by the probe, then the 5 remaining ones + const vmime::string data = "eZZZAAAAA\r\n"; + + VASSERT_EQ("decoded size", 5, decode("uuencode", data).length()); + } + + /** A line length larger than what the decoder's buffers can hold must not + * make it write out bytes it never decoded. + * + * '_' decodes to a line length of 63, but the decoding loop fills at most + * 48 bytes of the output buffer, so 15 bytes of uninitialized stack used + * to be copied into the decoded output (CWE-908). + */ + void testDecodeOversizedLineLength() { + + vmime::string data = "_"; + data.append(64, 'A'); + + // 16 groups of 4 encoded bytes, each decoding to 3 bytes + vmime::string expected; + + for (int i = 0 ; i < 16 ; ++i) { + expected += "\x86\x18\x61"; + } + + VASSERT_EQ("decoded", expected, decode("uuencode", data)); + } + + void testDecodeTruncatedMarkers() { + + VASSERT_EQ("b", "", decode("uuencode", "b")); + VASSERT_EQ("e", "", decode("uuencode", "e")); + VASSERT_EQ("begin", "", decode("uuencode", "begin 644 test.txt\r\n")); + } + +VMIME_TEST_SUITE_END diff --git a/tests/utility/seekableInputStreamRegionAdapterTest.cpp b/tests/utility/seekableInputStreamRegionAdapterTest.cpp index b099cd77..8ca87209 100644 --- a/tests/utility/seekableInputStreamRegionAdapterTest.cpp +++ b/tests/utility/seekableInputStreamRegionAdapterTest.cpp @@ -40,6 +40,8 @@ VMIME_TEST_SUITE_BEGIN(seekableInputStreamRegionAdapterTest) VMIME_TEST(testSkip) VMIME_TEST(testReset) VMIME_TEST(testOwnPosition) + VMIME_TEST(testReadCountOverflow) + VMIME_TEST(testSkipCountOverflow) VMIME_TEST_LIST_END @@ -173,4 +175,54 @@ VMIME_TEST_SUITE_BEGIN(seekableInputStreamRegionAdapterTest) VASSERT_EQ("Pos", 21, ustream->getPosition()); } + void testReadCountOverflow() { + + // A count which overflows when added to the current position must be + // handled as a request for all the remaining bytes, both by the region + // adapter and by the underlying stream + const vmime::size_t count = static_cast (-3); + + vmime::shared_ptr ustream; + vmime::shared_ptr stream = createStream(&ustream); + + stream->seek(5); + + vmime::byte_t buffer[100]; + std::fill(vmime::begin(buffer), vmime::end(buffer), 0); + + VASSERT_EQ("Read 1", 6, stream->read(buffer, count)); + VASSERT_EQ("Pos 1", 11, stream->getPosition()); + VASSERT_TRUE("EOF 1", stream->eof()); + VASSERT_EQ("Buffer 1", "BUFFER", vmime::utility::stringUtils::makeStringFromBytes(buffer, 6)); + + ustream->seek(10); + + std::fill(vmime::begin(buffer), vmime::end(buffer), 0); + + VASSERT_EQ("Read 2", 11, ustream->read(buffer, count)); + VASSERT_EQ("Pos 2", 21, ustream->getPosition()); + VASSERT_TRUE("EOF 2", ustream->eof()); + VASSERT_EQ("Buffer 2", "TEST BUFFER", vmime::utility::stringUtils::makeStringFromBytes(buffer, 11)); + } + + void testSkipCountOverflow() { + + const vmime::size_t count = static_cast (-3); + + vmime::shared_ptr ustream; + vmime::shared_ptr stream = createStream(&ustream); + + stream->seek(5); + + VASSERT_EQ("Skip 1", 6, stream->skip(count)); + VASSERT_EQ("Pos 1", 11, stream->getPosition()); + VASSERT_TRUE("EOF 1", stream->eof()); + + ustream->seek(10); + + VASSERT_EQ("Skip 2", 11, ustream->skip(count)); + VASSERT_EQ("Pos 2", 21, ustream->getPosition()); + VASSERT_TRUE("EOF 2", ustream->eof()); + } + VMIME_TEST_SUITE_END