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: */
>
