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

On 16.09.2017 22:21, Nils Freydank wrote:

[...]

(Feedback is very appreciated / this is an updated patch.)

This line shouldn't be part of the commit. You can add it after the "---" marker (see below) when you use `git send-email --annotate` or if you save the patch with `git format-patch` and then edit the file before sending with send-email. You can also include a list of changes from the previous version there.

[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 ---

This marker here ^

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

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;

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

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

Not needed. We are heading to a while statement with "spos < pos + ulen", so this is auto handled. -> actually this one is the one that is needed to fix the loop -> other checks are dropped as asked for

lib/libalpm/signing.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c index 95cb3280..51b11df6 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; @@ -1059,7 +1081,16 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, 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]; @@ -1072,30 +1103,39 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, slen = sig[spos]; 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]); } *keys = alpm_list_add(*keys, strdup(key)); break; } - + if(length_check(pos + ulen + 1, spos, slen, handle, identifier)) { + return -1; + } spos = spos + slen; } - pos = pos + (blen - hlen - 8); } - return 0; } -- 2.14.2