From ea5afc63166207127428d4587bed2a425ffb1b9b Mon Sep 17 00:00:00 2001 From: Vincent Richard Date: Thu, 22 Oct 2026 09:41:33 +0200 Subject: [PATCH] uuEncoder: fix out-of-bounds write and several decoding bugs Memory safety: - A 'b' line length which does not introduce a "begin" line made the decoder read 5 bytes while probing for the marker, more than such a line holds. The per-line read count 'inLength - inPos' then underflowed, and the resulting near-SIZE_MAX count drove a write past the decoder's 64-byte stack buffer. - A line length larger than 48 made the decoder write out up to 15 bytes of its output buffer which it had not decoded, leaking uninitialized stack memory into the decoded data. Hardening: - inputStreamStringAdapter and seekableInputStreamRegionAdapter now compare the requested count with the number of remaining bytes, instead of testing 'pos + count >= end': the sum may overflow and let a huge count through to the copy. - inputStreamStringAdapter no longer copies past the end of its range when it covers only a part of the string. Decoding: - The number of encoded characters on a line was computed as (length * 4) / 3, while the encoder writes 4 characters for each group of 3 bytes, including an incomplete last group. Any line whose length is not a multiple of 3 (usually the last one) was only partly read, and the decoder lost sync with its input. - The mode and filename of the "begin" line were both extracted from the start of the line, so the filename included the mode. - On a "begin\n" line with no mode nor filename, the following line, which holds data, was taken as the mode and filename. Fixes #362. Reported by Nguyen Dinh Hai (@Haind03). --- src/vmime/utility/encoder/uuEncoder.cpp | 28 ++- .../utility/inputStreamStringAdapter.cpp | 11 +- .../seekableInputStreamRegionAdapter.cpp | 10 +- tests/utility/encoder/uuEncoderTest.cpp | 162 ++++++++++++++++++ .../seekableInputStreamRegionAdapterTest.cpp | 52 ++++++ 5 files changed, 247 insertions(+), 16 deletions(-) create mode 100644 tests/utility/encoder/uuEncoderTest.cpp 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