CVE-2019-19221: Out-of-bounds Read (CWE-125)
value: In Libarchive 3.4.0, archive_wstring_append_from_mbs in archive_string.c has an out-of-bounds read because of an incorrect mbrtowc or mbtowc call. For example, bsdtar crashes via a crafted archive.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 5.5
Published: 2019-11-21T23:15Z
Repository: https://github.com/libarchive/libarchive
Language: C
Files changed: libarchive/archive_string.c
Commit date: 2019-11-21 03:08:40 +0100
Commit message: Bugfix and optimize archive_wstring_append_from_mbs()

The cal to mbrtowc() or mbtowc() should read up to mbs_length
bytes and not wcs_length. This avoids out-of-bounds reads.

mbrtowc() and mbtowc() return (size_t)-1 wit errno EILSEQ when
they encounter an invalid multibyte character and (size_t)-2 when
they they encounter an incomplete multibyte character. As we return
failure and all our callers error out it makes no sense to continue
parsing mbs.

As we allocate `len` wchars at the beginning and each wchar has
at least one byte, there will never be need to grow the buffer,
so the code can be left out. On the other hand, we are always
allocatng more memory than we need.

As long as wcs_length == mbs_length == len we can omit wcs_length.
We keep the old code commented if we decide to save memory and
use autoexpanding wcs_length in the future.

Fixes #1276

Diff: diff --git a/libarchive/archive_string.c b/libarchive/archive_string.c
index 979a418b67..bd39c96f10 100644
--- a/libarchive/archive_string.c
+++ b/libarchive/archive_string.c
@@ -589,55 +589,61 @@ archive_wstring_append_from_mbs(struct archive_wstring *dest,
 	int ret_val = 0;
 	/*
 	 * No single byte will be more than one wide character,
 	 * so this length estimate will always be big enough.
 	 */
-	size_t wcs_length = len;
+	// size_t wcs_length = len;
 	size_t mbs_length = len;
 	const char *mbs = p;
 	wchar_t *wcs;
 #if HAVE_MBRTOWC
 	mbstate_t shift_state;
 
 	memset(&shift_state, 0, sizeof(shift_state));
 #endif
-	if (NULL == archive_wstring_ensure(dest, dest->length + wcs_length + 1))
+	/*
+	 * As we decided to have wcs_length == mbs_length == len
+	 * we can use len here instead of wcs_length
+	 */
+	if (NULL == archive_wstring_ensure(dest, dest->length + len + 1))
 		return (-1);
 	wcs = dest->s + dest->length;
 	/*
 	 * We cannot use mbsrtowcs/mbstowcs here because those may convert
 	 * extra MBS when strlen(p) > len and one wide character consists of
 	 * multi bytes.
 	 */
 	while (*mbs && mbs_length > 0) {
+		/*
+		 * The buffer we allocated is always big enough.
+		 * Keep this code path in a comment if we decide to choose
+		 * smaller wcs_length in the future
+		 */
+/*
 		if (wcs_length == 0) {
 			dest->length = wcs - dest->s;
 			dest->s[dest->length] = L'\0';
 			wcs_length = mbs_length;
 			if (NULL == archive_wstring_ensure(dest,
 			    dest->length + wcs_length + 1))
 				return (-1);
 			wcs = dest->s + dest->length;
 		}
+*/
 #if HAVE_MBRTOWC
-		r = mbrtowc(wcs, mbs, wcs_length, &shift_state);
+		r = mbrtowc(wcs, mbs, mbs_length, &shift_state);
 #else
-		r = mbtowc(wcs, mbs, wcs_length);
+		r = mbtowc(wcs, mbs, mbs_length);
 #endif
 		if (r == (size_t)-1 || r == (size_t)-2) {
 			ret_val = -1;
-			if (errno == EILSEQ) {
-				++mbs;
-				--mbs_length;
-				continue;
-			} else
-				break;
+			break;
 		}
 		if (r == 0 || r > mbs_length)
 			break;
 		wcs++;
-		wcs_length--;
+		// wcs_length--;
 		mbs += r;
 		mbs_length -= r;
 	}
 	dest->length = wcs - dest->s;
 	dest->s[dest->length] = L'\0';

Vulnerable code: 	size_t wcs_length = len;
	if (NULL == archive_wstring_ensure(dest, dest->length + wcs_length + 1))
		r = mbrtowc(wcs, mbs, wcs_length, &shift_state);
		r = mbtowc(wcs, mbs, wcs_length);
			if (errno == EILSEQ) {
				++mbs;
				--mbs_length;
				continue;
			} else
				break;
		wcs_length--;
Fixed code: 	// size_t wcs_length = len;
	/*
	 * As we decided to have wcs_length == mbs_length == len
	 * we can use len here instead of wcs_length
	 */
	if (NULL == archive_wstring_ensure(dest, dest->length + len + 1))
		/*
		 * The buffer we allocated is always big enough.
		 * Keep this code path in a comment if we decide to choose
		 * smaller wcs_length in the future
		 */
/*
*/
		r = mbrtowc(wcs, mbs, mbs_length, &shift_state);
		r = mbtowc(wcs, mbs, mbs_length);
			break;
		// wcs_length--;