[pacman-dev] [PATCH] Fix OOB read and endless loop in signature parser.

[Tobias Stoeckmann]

The parser in alpm_extract_keyid does not properly check boundaries,
which makes it vulnerable to malicious input for out ouf bondary reads.
I did not attach a proof of concept for this one, because it is very
system-specific if you see a segmentation fault or not. If in doubt,
you won't notice it. ;)

It is also possible to trigger an endless loop with a malicious
signature file on 32 bit systems. A proof of concept for this can
be triggered through this crafted file (SigLevel must be properly set
for local files):

$ uname -m
i686
$ PKG=package-1.0.tar.xz
$ touch $PKG
$ echo "iQEcBAABCAAGBQJXTxJiAAr/////+wA=" | base64 -d - > $PKG.sig
$ sudo pacman -U $PKG
_

This proof of concept uses the ability to overflow position increments.
By setting lengths to specific values ((size_t)-5 in this example), the
position incrementation effectively ends up at its old position.

This patch does not use GCC-specific overflow checking routines,
because I am not sure if that's applicable for pacman's design goals.

Signed-off-by: Tobias Stoeckmann <tobias at stoeckmann.org>
---
Feedback is very appreciated. :)
---
 lib/libalpm/signing.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c
index 0267158..7dc7694 100644
--- a/lib/libalpm/signing.c
+++ b/lib/libalpm/signing.c
@@ -982,6 +982,21 @@ int SYMEXPORT alpm_siglist_cleanup(alpm_siglist_t *siglist)
 	return 0;
 }
 
+#define LENGTH_CHECK(l, p, a)								\
+	do {										\
+		if ((a) == 0 || (l) - (p) <= (a)) {					\
+			_alpm_log(handle, ALPM_LOG_ERROR,				\
+					_("%s: signature format error"), identifier);	\
+			return -1;							\
+		}									\
+	} while(0)
+
+#define SAFE_ADD(l, p, a)								\
+	do {										\
+		LENGTH_CHECK((l), p, (a));						\
+		p = p + (a);								\
+	} while(0)
+
 /**
  * Extract the Issuer Key ID from a signature
  * @param sig PGP signature
@@ -1018,18 +1033,21 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 
 		switch(sig[pos] & 0x03) {
 			case 0:
+				LENGTH_CHECK(len, pos, 1);
 				blen = sig[pos + 1];
-				pos = pos + 2;
+				SAFE_ADD(len, pos, 2);
 				break;
 
 			case 1:
+				LENGTH_CHECK(len, pos, 2);
 				blen = (sig[pos + 1] << 8) | sig[pos + 2];
-				pos = pos + 3;
+				SAFE_ADD(len, pos, 3);
 				break;
 
 			case 2:
+				LENGTH_CHECK(len, pos, 4);
 				blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4];
-				pos = pos + 5;
+				SAFE_ADD(len, pos, 5);
 				break;
 
 			case 3:
@@ -1046,6 +1064,7 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 			return -1;
 		}
 
+		LENGTH_CHECK(len, pos, 1);
 		if(sig[pos + 1] != 0x00) {
 			/* not a signature of a binary document */
 			_alpm_log(handle, ALPM_LOG_ERROR,
@@ -1053,32 +1072,38 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 			return -1;
 		}
 
-		pos = pos + 4;
+		SAFE_ADD(len, pos, 4);
 
+		LENGTH_CHECK(len, pos, 1);
 		hlen = (sig[pos] << 8) | sig[pos + 1];
-		pos = pos + hlen + 2;
+		SAFE_ADD(len, pos, hlen + 2);
 
+		LENGTH_CHECK(len, pos, 1);
 		ulen = (sig[pos] << 8) | sig[pos + 1];
-		pos = pos + 2;
+		SAFE_ADD(len, pos, 2);
 
 		spos = pos;
 
+		LENGTH_CHECK(len, pos, ulen);
 		while(spos < pos + ulen) {
 			if(sig[spos] < 192) {
 				slen = sig[spos];
 				spos = spos + 1;
 			} else if(sig[spos] < 255) {
+				LENGTH_CHECK(pos + ulen, spos, 1);
 				slen = (sig[spos] << 8) | sig[spos + 1];
-				spos = spos + 2;
+				SAFE_ADD(pos + ulen, spos, 2);
 			} else {
+				LENGTH_CHECK(pos + ulen, spos, 4);
 				slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4];
-				spos = spos + 5;
+				SAFE_ADD(pos + ulen, spos, 5);
 			}
 
 			if(sig[spos] == 16) {
 				/* issuer key ID */
 				char key[17];
 				size_t i;
+				LENGTH_CHECK(pos + ulen, spos, 8);
 				for (i = 0; i < 8; i++) {
 					sprintf(&key[i * 2], "%02X", sig[spos + i + 1]);
 				}
@@ -1086,10 +1111,11 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 				break;
 			}
 
-			spos = spos + slen;
+			SAFE_ADD(pos + ulen + 1, spos, slen);
 		}
 
-		pos = pos + (blen - hlen - 8);
+		LENGTH_CHECK(blen, hlen, 8);
+		SAFE_ADD(len + 1, pos, blen - hlen - 8);
 	}
 
 	return 0;
-- 
2.8.3