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

[Allan McRae]

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;
>  }
>