diff mbox series

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

Message ID 27360402.O0bhZDOYKm@pygoscelis
State Superseded, archived
Headers show
Series [pacman-dev,1/1] Fix CVE-2016-5434 (DoS/loop and out of boundary read) | expand

Commit Message

Nils Freydank Aug. 29, 2017, 7:53 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 size_t length_check() and applies it to 
libalpm/signing.c and signing.h.

Feedback is very appreciated.


[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

Comments

Nils Freydank Sept. 5, 2017, 4:04 p.m. UTC | #1 
This is an update to fix style issues (indentation, newlines etc.) that were addressed on IRC.

Original message:
>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 size_t length_check() and applies it to 
>libalpm/signing.c and signing.h.
>
>Feedback is very appreciated.
>
>
>[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

diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c
index 95cb3280..3a907bb8 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 */
+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,18 +1035,41 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 
 		switch(sig[pos] & 0x03) {
 			case 0:
-				blen = sig[pos + 1];
-				pos = pos + 2;
+				if(length_check(len, pos, 1, handle, identifier)) {
+					return -1;
+				} else {
+					blen = sig[pos + 1];
+				}
+				if(length_check(len, pos, 2, handle, identifier)) {
+					return -1;
+				} else {
+					pos = pos + 2;
+				}
 				break;
-
 			case 1:
-				blen = (sig[pos + 1] << 8) | sig[pos + 2];
-				pos = pos + 3;
+				if(length_check(len, pos, 2, handle, identifier)) {
+					return -1;
+				} else {
+					blen = (sig[pos + 1] << 8) | sig[pos + 2];
+				}
+				if(length_check(len, pos, 3, handle, identifier)) {
+					return -1;
+				} else {
+					pos = pos + 3;
+				}
 				break;
 
 			case 2:
-				blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4];
-				pos = pos + 5;
+				if(length_check(len, pos, 4, handle, identifier)) {
+					return -1;
+				} else {
+					blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4];
+				}
+				if(length_check(len, pos, 5, handle, identifier)) {
+					return -1;
+				} else {
+					pos = pos + 5;
+				}
 				break;
 
 			case 3:
@@ -1057,43 +1093,99 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
 			return -1;
 		}
 
-		pos = pos + 4;
+		if(length_check(len, pos, 4, handle, identifier)) {
+			return -1;
+		} else {
+			pos = pos + 4;
+		}
 
-		hlen = (sig[pos] << 8) | sig[pos + 1];
-		pos = pos + hlen + 2;
+		if(length_check(len, pos, 1, handle, identifier)) {
+			return -1;
+		} else {
+			hlen = (sig[pos] << 8) | sig[pos + 1];
+		}
 
-		ulen = (sig[pos] << 8) | sig[pos + 1];
-		pos = pos + 2;
+		if(length_check(len, pos, 2, handle, identifier)) {
+			return -1;
+		} else {
+			pos = pos + hlen + 2;
+		}
 
-		spos = pos;
+		if(length_check(len, pos, 1, handle, identifier)) {
+			return -1;
+		} else {
+			ulen = (sig[pos] << 8) | sig[pos + 1];
+		}
 
-		while(spos < pos + ulen) {
-			if(sig[spos] < 192) {
-				slen = sig[spos];
-				spos = spos + 1;
-			} else if(sig[spos] < 255) {
-				slen = (sig[spos] << 8) | sig[spos + 1];
-				spos = spos + 2;
-			} else {
-				slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4];
-				spos = spos + 5;
-			}
+		if(length_check(len, pos, 2, handle, identifier)) {
+			return -1;
+		} else {
+			pos = pos + 2;
+		}
 
-			if(sig[spos] == 16) {
-				/* issuer key ID */
-				char key[17];
-				size_t i;
-				for (i = 0; i < 8; i++) {
-					sprintf(&key[i * 2], "%02X", sig[spos + i + 1]);
+		spos = pos;
+
+		if(length_check(len, pos, ulen, handle, identifier)) {
+			return -1;
+		} else {
+			while(spos < pos + ulen) {
+				if(sig[spos] < 192) {
+					slen = sig[spos];
+					if(length_check(len + ulen, spos, 1, handle, identifier)) {
+						return -1;
+					} else {
+						spos = spos + 1;
+					}
+				} else if(sig[spos] < 255) {
+					if(length_check(pos + ulen, spos, 1, handle, identifier))
+					{
+						return -1;
+					} else {
+						slen = (sig[spos] << 8) | sig[spos + 1];
+					}
+					if(length_check(pos + ulen, spos, 2, handle, identifier)) {
+						return -1;
+					} else {
+						spos = spos + 2;
+					}
+				} else {
+					if(length_check(len, pos, 4, handle, identifier)) {
+						return -1;
+					} else {
+						slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4];
+					}
+					if(length_check(len, spos, 5, handle, identifier)) {
+						return -1;
+					} else {
+						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;
+					} else {
+						for (i = 0; i < 8; i++) {
+							sprintf(&key[i * 2], "%02X", sig[spos + i + 1]);
+						}
+					}
+					*keys = alpm_list_add(*keys, strdup(key));
+					break;
+				}
+				if(length_check(pos + ulen + 1, spos, slen, handle, identifier)) {
+					return -1;
+				} else {
+					spos = spos + slen;
 				}
-				*keys = alpm_list_add(*keys, strdup(key));
-				break;
 			}
-
-			spos = spos + slen;
+			if(length_check( blen, hlen, 8, handle, identifier )) {
+				return -1;
+			} else {
+				pos = pos + (blen - hlen - 8);
+			}
 		}
-
-		pos = pos + (blen - hlen - 8);
 	}
 
 	return 0;
diff --git a/lib/libalpm/signing.h b/lib/libalpm/signing.h
index 3507f584..a67bd900 100644
--- a/lib/libalpm/signing.h
+++ b/lib/libalpm/signing.h
@@ -36,4 +36,6 @@ int _alpm_key_import(alpm_handle_t *handle, const char *fpr);
 
 #endif /* ALPM_SIGNING_H */
 
+size_t length_check(size_t length, size_t position, size_t a,
+		alpm_handle_t *handle, const char *identifier);
 /* vim: set noet: */
Allan McRae Sept. 12, 2017, 7:01 a.m. UTC | #2 
On 06/09/17 02:04, Nils Freydank wrote:
> This is an update to fix style issues (indentation, newlines etc.) that were addressed on IRC.
> 
> Original message:
>> 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 size_t length_check() and applies it to 
>> libalpm/signing.c and signing.h.
>>
>> Feedback is very appreciated.
>>
>>
>> [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
> 

Please send as a git patch.

> diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c
> index 95cb3280..3a907bb8 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 */

This function is not used outside this file, so scope it there.  If
needed elsewhere, it can be moved to util.c.

static size_t ...

> +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,18 +1035,41 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
>  
>  		switch(sig[pos] & 0x03) {
>  			case 0:
> -				blen = sig[pos + 1];
> -				pos = pos + 2;

Too much going on here....


> +				if(length_check(len, pos, 1, handle, identifier)) {
> +					return -1;
> +				} else {
> +					blen = sig[pos + 1];
> +				}
> +				if(length_check(len, pos, 2, handle, identifier)) {
> +					return -1;
> +				} else {
> +					pos = pos + 2;
> +				}

Replace this block with :

if(length_check(len, pos, 2, handle, identifier) != 0) {
	return -1;
}
blen = sig[pos + 1];
pos = pos + 2;

Note the explicit test of != 0 in the if statement.  This combining of
tests can be done throughout the patch.


>  				break;
> -
>  			case 1:
> -				blen = (sig[pos + 1] << 8) | sig[pos + 2];
> -				pos = pos + 3;
> +				if(length_check(len, pos, 2, handle, identifier)) {
> +					return -1;
> +				} else {
> +					blen = (sig[pos + 1] << 8) | sig[pos + 2];
> +				}
> +				if(length_check(len, pos, 3, handle, identifier)) {
> +					return -1;
> +				} else {
> +					pos = pos + 3;
> +				}
>  				break;
>  
>  			case 2:
> -				blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4];
> -				pos = pos + 5;
> +				if(length_check(len, pos, 4, handle, identifier)) {
> +					return -1;
> +				} else {
> +					blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4];
> +				}
> +				if(length_check(len, pos, 5, handle, identifier)) {
> +					return -1;
> +				} else {
> +					pos = pos + 5;
> +				}
>  				break;
>  
>  			case 3:
> @@ -1057,43 +1093,99 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier,
>  			return -1;
>  		}
>  
> -		pos = pos + 4;
> +		if(length_check(len, pos, 4, handle, identifier)) {
> +			return -1;
> +		} else {
> +			pos = pos + 4;
> +		}
>  
> -		hlen = (sig[pos] << 8) | sig[pos + 1];
> -		pos = pos + hlen + 2;
> +		if(length_check(len, pos, 1, handle, identifier)) {
> +			return -1;
> +		} else {
> +			hlen = (sig[pos] << 8) | sig[pos + 1];
> +		}
>  
> -		ulen = (sig[pos] << 8) | sig[pos + 1];
> -		pos = pos + 2;
> +		if(length_check(len, pos, 2, handle, identifier)) {
> +			return -1;
> +		} else {
> +			pos = pos + hlen + 2;
> +		}
>  
> -		spos = pos;
> +		if(length_check(len, pos, 1, handle, identifier)) {
> +			return -1;
> +		} else {
> +			ulen = (sig[pos] << 8) | sig[pos + 1];
> +		}
>  
> -		while(spos < pos + ulen) {
> -			if(sig[spos] < 192) {
> -				slen = sig[spos];
> -				spos = spos + 1;
> -			} else if(sig[spos] < 255) {
> -				slen = (sig[spos] << 8) | sig[spos + 1];
> -				spos = spos + 2;
> -			} else {
> -				slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4];
> -				spos = spos + 5;
> -			}
> +		if(length_check(len, pos, 2, handle, identifier)) {
> +			return -1;
> +		} else {
> +			pos = pos + 2;
> +		}
>  
> -			if(sig[spos] == 16) {
> -				/* issuer key ID */
> -				char key[17];
> -				size_t i;
> -				for (i = 0; i < 8; i++) {
> -					sprintf(&key[i * 2], "%02X", sig[spos + i + 1]);
> +		spos = pos;
> +
> +		if(length_check(len, pos, ulen, handle, identifier)) {
> +			return -1;
> +		} else {
> +			while(spos < pos + ulen) {
> +				if(sig[spos] < 192) {
> +					slen = sig[spos];
> +					if(length_check(len + ulen, spos, 1, handle, identifier)) {
> +						return -1;
> +					} else {
> +						spos = spos + 1;
> +					}
> +				} else if(sig[spos] < 255) {
> +					if(length_check(pos + ulen, spos, 1, handle, identifier))
> +					{
> +						return -1;
> +					} else {
> +						slen = (sig[spos] << 8) | sig[spos + 1];
> +					}
> +					if(length_check(pos + ulen, spos, 2, handle, identifier)) {
> +						return -1;
> +					} else {
> +						spos = spos + 2;
> +					}
> +				} else {
> +					if(length_check(len, pos, 4, handle, identifier)) {
> +						return -1;
> +					} else {
> +						slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4];
> +					}
> +					if(length_check(len, spos, 5, handle, identifier)) {
> +						return -1;
> +					} else {
> +						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;
> +					} else {
> +						for (i = 0; i < 8; i++) {
> +							sprintf(&key[i * 2], "%02X", sig[spos + i + 1]);
> +						}
> +					}
> +					*keys = alpm_list_add(*keys, strdup(key));
> +					break;
> +				}
> +				if(length_check(pos + ulen + 1, spos, slen, handle, identifier)) {
> +					return -1;
> +				} else {
> +					spos = spos + slen;
>  				}
> -				*keys = alpm_list_add(*keys, strdup(key));
> -				break;
>  			}
> -
> -			spos = spos + slen;
> +			if(length_check( blen, hlen, 8, handle, identifier )) {
> +				return -1;
> +			} else {
> +				pos = pos + (blen - hlen - 8);
> +			}
>  		}
> -
> -		pos = pos + (blen - hlen - 8);
>  	}
>  
>  	return 0;



Remove the header addition:

> diff --git a/lib/libalpm/signing.h b/lib/libalpm/signing.h
> index 3507f584..a67bd900 100644
> --- a/lib/libalpm/signing.h
> +++ b/lib/libalpm/signing.h
> @@ -36,4 +36,6 @@ int _alpm_key_import(alpm_handle_t *handle, const char *fpr);
>  
>  #endif /* ALPM_SIGNING_H */
>  
> +size_t length_check(size_t length, size_t position, size_t a,
> +		alpm_handle_t *handle, const char *identifier);
>  /* vim: set noet: */
>
diff mbox series

Patch

diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c
index 95cb3280..f7a9c9a8 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 */
+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
@@ -1021,19 +1034,22 @@  int SYMEXPORT alpm_extract_keyid(alpm_handle_t 
*handle, const char *identifier,
 		}
 
 		switch(sig[pos] & 0x03) {
-			case 0:
-				blen = sig[pos + 1];
-				pos = pos + 2;
+			case 0: if( length_check(len, pos, 1, handle, identifier) ){ return 
-1; }
+				else blen = sig[pos + 1];
+				if( length_check(len, pos, 2, handle, identifier) ){ return -1; }
+				else pos = pos + 2;
 				break;
-
 			case 1:
-				blen = (sig[pos + 1] << 8) | sig[pos + 2];
-				pos = pos + 3;
+				if( length_check(len, pos, 2, handle, identifier) ){ return -1; }
+				else blen = (sig[pos + 1] << 8) | sig[pos + 2];
+				if( length_check(len, pos, 3, handle, identifier) ){ return -1; }
+				else pos = pos + 3;
 				break;
 
-			case 2:
-				blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] 
<< 8) | sig[pos + 4];
-				pos = pos + 5;
+			case 2:	if( length_check(len, pos, 4, handle, identifier) ){ return 
-1; }
+				else blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos 
+ 3] << 8) | sig[pos + 4];
+				if( length_check(len, pos, 5, handle, identifier) ){ return -1; }
+				else pos = pos + 5;
 				break;
 
 			case 3:
@@ -1057,25 +1073,38 @@  int SYMEXPORT alpm_extract_keyid(alpm_handle_t 
*handle, const char *identifier,
 			return -1;
 		}
 
-		pos = pos + 4;
+		if( length_check(len, pos, 4, handle, identifier) ){ return -1; }
+		else pos = pos + 4;
 
-		hlen = (sig[pos] << 8) | sig[pos + 1];
-		pos = pos + hlen + 2;
+		if( length_check(len, pos, 1, handle, identifier) ){ return -1; }
+		else hlen = (sig[pos] << 8) | sig[pos + 1];
 
-		ulen = (sig[pos] << 8) | sig[pos + 1];
-		pos = pos + 2;
+		if( length_check(len, pos, 2, handle, identifier) ){ return -1; }
+		else pos = pos + hlen + 2;
+
+		if( length_check(len, pos, 1, handle, identifier) ){ return -1; }
+		else ulen = (sig[pos] << 8) | sig[pos + 1];
+
+		if( length_check(len, pos, 2, handle, identifier) ){ return -1; }
+		else pos = pos + 2;
 
 		spos = pos;
 
+		if( length_check(len, pos, ulen, handle, identifier) ){ return -1; }
+		else
 		while(spos < pos + ulen) {
 			if(sig[spos] < 192) {
 				slen = sig[spos];
-				spos = spos + 1;
+				if( length_check(len + ulen, spos, 1, handle, identifier) ){ 
return -1; }
+				else spos = spos + 1;
 			} else if(sig[spos] < 255) {
-				slen = (sig[spos] << 8) | sig[spos + 1];
-				spos = spos + 2;
+				if( length_check(pos + ulen, spos, 1, handle, identifier) ){ 
return -1; }
+				else slen = (sig[spos] << 8) | sig[spos + 1];
+				if( length_check(pos + ulen, spos, 2, handle, identifier) ){ 
return -1; }
+				else spos = spos + 2;
 			} else {
-				slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 
3] << 8) | sig[spos + 4];
+				if( length_check(len, pos, 4, handle, identifier) ){ return -1; }
+				else slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | 
(sig[spos + 3] << 8) | sig[spos + 4];
 				spos = spos + 5;
 			}
 
@@ -1083,17 +1112,20 @@  int SYMEXPORT alpm_extract_keyid(alpm_handle_t 
*handle, const char *identifier,
 				/* issuer key ID */
 				char key[17];
 				size_t i;
+				if( length_check(pos + ulen, spos, 8, handle, identifier) ){return 
-1; }
+				else{
 				for (i = 0; i < 8; i++) {
 					sprintf(&key[i * 2], "%02X", sig[spos + i + 1]);
 				}
+				}
 				*keys = alpm_list_add(*keys, strdup(key));
 				break;
 			}
-
-			spos = spos + slen;
+			if( length_check(pos + ulen + 1, spos, slen, handle, identifier) )
{return -1; }
+			else spos = spos + slen;
 		}
-
-		pos = pos + (blen - hlen - 8);
+		if( length_check( blen, hlen, 8, handle, identifier ) ){return -1;}
+		else pos = pos + (blen - hlen - 8);
 	}
 
 	return 0;
diff --git a/lib/libalpm/signing.h b/lib/libalpm/signing.h
index 3507f584..a67bd900 100644
--- a/lib/libalpm/signing.h
+++ b/lib/libalpm/signing.h
@@ -36,4 +36,6 @@  int _alpm_key_import(alpm_handle_t *handle, const char 
*fpr);
 
 #endif /* ALPM_SIGNING_H */
 
+size_t length_check(size_t length, size_t position, size_t a,
+		alpm_handle_t *handle, const char *identifier);