diff mbox

[pacman-dev] Fix CVE-2016-5434 (DoS/loop and out of boundary read)

Message ID 20170928120200.13270-1-holgersson@posteo.de
State Superseded, archived
Headers show

Commit Message

Nils Freydank Sept. 28, 2017, 12:02 p.m. UTC 
This is a rewrite of Tobias Stoeckmann’s patch from June 2016[1] using
functions instead of macros. (Thanks to Tobias for explanations of his patch.)
A short question on Freenode IRC showed that macros are generally discouraged
and functions should be used.

The patch introduces a static size_t length_check() in libalpm/signing.c.

[1] Original patch:
https://lists.archlinux.org/pipermail/pacman-dev/2016-June/021148.html
CVE request (and assignment):
http://seclists.org/oss-sec/2016/q2/526
---

 Modified as requested and dropped the signing.h from the commit message
 as I fixed that in an earlier revision.

 lib/libalpm/signing.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

Comments

Allan McRae Sept. 29, 2017, 10:16 a.m. UTC | #1 
On 28/09/17 22:02, Nils Freydank wrote:
> @@ -1057,9 +1079,21 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
>  			return -1;
>  		}
>  
> +		if(length_check(len, pos, 4, handle, identifier)) {
> +			return -1;
> +		}
>  		pos = pos + 4;
>  
> +		/* pos got changed above, so an explicit check is necessary
> +		 * check for 2 as that catches another some lines down */
> +		if(length_check(len, pos, 2, handle, identifier)) {
> +			return -1;
> +		}
>  		hlen = (sig[pos] << 8) | sig[pos + 1];
> +

Why is there a double check here?  Sure pos got increased, but there is
not need to check that.  Only the second check before the read is needed.

Or I am missing something completely?

A
Allan McRae Sept. 29, 2017, 10:34 a.m. UTC | #2 
On 28/09/17 22:02, Nils Freydank wrote:
> @@ -1067,22 +1101,38 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
>  
>  		spos = pos;
>  
> +		if(length_check(len, pos, ulen, handle, identifier)) {
> +			return -1;
> +		}

Why is this check needed?  spos starts at pos, and we know pos is good
here.  Every future usage of spos is checked.

>  		while(spos < pos + ulen) {
>  			if(sig[spos] < 192) {
>  				slen = sig[spos];
> +				if(length_check(len + ulen, spos, 1, handle, identifier)) {
> +					return -1;
> +				}

Why is this check needed?  We are not using this value without checking
it in the future.

>  				spos = spos + 1;
>  			} else if(sig[spos] < 255) {
> +				if(length_check(pos + ulen, spos, 2, handle, identifier))
> +				{
> +					return -1;
> +				}
>  				slen = (sig[spos] << 8) | sig[spos + 1];
>  				spos = spos + 2;
>  			} else {
> +				/* check for pos and spos, as spos is still pos */
> +				if(length_check(len, pos, 5, handle, identifier)) {
> +					return -1;
> +				}
>  				slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4];
>  				spos = spos + 5;
>  			}
> 
>  			if(sig[spos] == 16) {
>  				/* issuer key ID */
>  				char key[17];
>  				size_t i;
> +				if(length_check(pos + ulen, spos, 8, handle, identifier)) {
> +					return -1;
> +				}
>  				for (i = 0; i < 8; i++) {
>  					sprintf(&key[i * 2], "%02X", sig[spos + i + 1]);
>  				}
> @@ -1090,12 +1140,16 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
>  				break;
>  			}
>  
> +			if(length_check(pos + ulen + 1, spos, slen, handle, identifier)) {
> +				return -1;
> +			}

Not needed.   We are heading to a while statement with "spos < pos +
ulen", so this is auto handled.

>  			spos = spos + slen;
>  		}
> -
> +		if(length_check( blen, hlen, 8, handle, identifier)) {
> +			return -1;
> +		}
Redundant check, we will check again before accessing the element.

>  		pos = pos + (blen - hlen - 8);>  	}
> -
>  	return 0;
>  }
>
diff mbox

Patch

diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c
index 95cb3280..33438140 100644
--- a/lib/libalpm/signing.c
+++ b/lib/libalpm/signing.c
@@ -986,6 +986,19 @@  int SYMEXPORT alpm_siglist_cleanup(alpm_siglist_t *siglist)
 	return 0;
 }
 
+/* Check to avoid out of boundary reads */
+static size_t length_check(size_t length, size_t position, size_t a,
+		alpm_handle_t *handle, const char *identifier)
+{
+	if( a == 0 || length - position <= a) {
+		_alpm_log(handle, ALPM_LOG_ERROR,
+		_("%s: signature format error"), identifier);
+		return -1;
+	} else {
+		return 0;
+	}
+}
+
 /**
  * Extract the Issuer Key ID from a signature
  * @param sig PGP signature
@@ -1022,16 +1035,25 @@  int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 
 		switch(sig[pos] & 0x03) {
 			case 0:
+				if(length_check(len, pos, 2, handle, identifier) != 0) {
+					return -1;
+				}
 				blen = sig[pos + 1];
 				pos = pos + 2;
 				break;
 
 			case 1:
+				if(length_check(len, pos, 3, handle, identifier)) {
+					return -1;
+				}
 				blen = (sig[pos + 1] << 8) | sig[pos + 2];
 				pos = pos + 3;
 				break;
 
 			case 2:
+				if(length_check(len, pos, 5, handle, identifier)) {
+					return -1;
+				}
 				blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4];
 				pos = pos + 5;
 				break;
@@ -1057,9 +1079,21 @@  int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 			return -1;
 		}
 
+		if(length_check(len, pos, 4, handle, identifier)) {
+			return -1;
+		}
 		pos = pos + 4;
 
+		/* pos got changed above, so an explicit check is necessary
+		 * check for 2 as that catches another some lines down */
+		if(length_check(len, pos, 2, handle, identifier)) {
+			return -1;
+		}
 		hlen = (sig[pos] << 8) | sig[pos + 1];
+
+		if(length_check(len, pos, hlen + 2, handle, identifier)) {
+			return -1;
+		}
 		pos = pos + hlen + 2;
 
 		ulen = (sig[pos] << 8) | sig[pos + 1];
@@ -1067,22 +1101,38 @@  int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 
 		spos = pos;
 
+		if(length_check(len, pos, ulen, handle, identifier)) {
+			return -1;
+		}
 		while(spos < pos + ulen) {
 			if(sig[spos] < 192) {
 				slen = sig[spos];
+				if(length_check(len + ulen, spos, 1, handle, identifier)) {
+					return -1;
+				}
 				spos = spos + 1;
 			} else if(sig[spos] < 255) {
+				if(length_check(pos + ulen, spos, 2, handle, identifier))
+				{
+					return -1;
+				}
 				slen = (sig[spos] << 8) | sig[spos + 1];
 				spos = spos + 2;
 			} else {
+				/* check for pos and spos, as spos is still pos */
+				if(length_check(len, pos, 5, handle, identifier)) {
+					return -1;
+				}
 				slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4];
 				spos = spos + 5;
 			}
-
 			if(sig[spos] == 16) {
 				/* issuer key ID */
 				char key[17];
 				size_t i;
+				if(length_check(pos + ulen, spos, 8, handle, identifier)) {
+					return -1;
+				}
 				for (i = 0; i < 8; i++) {
 					sprintf(&key[i * 2], "%02X", sig[spos + i + 1]);
 				}
@@ -1090,12 +1140,16 @@  int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 				break;
 			}
 
+			if(length_check(pos + ulen + 1, spos, slen, handle, identifier)) {
+				return -1;
+			}
 			spos = spos + slen;
 		}
-
+		if(length_check( blen, hlen, 8, handle, identifier)) {
+			return -1;
+		}
 		pos = pos + (blen - hlen - 8);
 	}
-
 	return 0;
 }