uuEncoder: fix out-of-bounds write and several decoding bugs

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 <size_t>(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 <size_t>(UUDECODE(lengthChar)), static_cast <size_t>(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,15 +249,15 @@ size_t uuEncoder::decode(
 
 						while (*p && parserHelpers::isSpace(*p)) ++p;
 
-						byte_t* modeStart = buffer;
+						byte_t* modeStart = p;
 
 						while (*p && !parserHelpers::isSpace(*p)) ++p;
 
 						getResults()["mode"] = string(modeStart, p);
 
 						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 <size_t>(3));
+			const size_t n = std::min(outLength - j, static_cast <size_t>(3));
 
 			if (n >= 3) {
 				outBuffer[j + 2] = static_cast <byte_t>(UUDECODE(c3) << 6 | UUDECODE(c4));

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;

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;
 

tests/utility/encoder/uuEncoderTest.cpp

@@ -0,0 +1,162 @@
+//
+// VMime library (http://www.vmime.org)
+// Copyright (C) 2002 Vincent Richard <vincent@vmime.org>
+//
+// 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 <char>(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 <vmime::utility::encoder::encoder> 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 <vmime::string>("mode", ""));
+		VASSERT_EQ("filename", "test.txt", cenc.getResults().getProperty <vmime::string>("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 <vmime::utility::encoder::encoder> 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 <vmime::string>("mode", ""));
+		VASSERT_EQ("filename", "untitled", cenc.getResults().getProperty <vmime::string>("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

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 <vmime::size_t>(-3);
+
+		vmime::shared_ptr <seekableInputStream> ustream;
+		vmime::shared_ptr <seekableInputStreamRegionAdapter> 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 <vmime::size_t>(-3);
+
+		vmime::shared_ptr <seekableInputStream> ustream;
+		vmime::shared_ptr <seekableInputStreamRegionAdapter> 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