The parser in alpm_extract_keyid does not properly check boundaries, which makes it vulnerable to malicious input for out ouf bondary reads. I did not attach a proof of concept for this one, because it is very system-specific if you see a segmentation fault or not. If in doubt, you won't notice it. ;) It is also possible to trigger an endless loop with a malicious signature file on 32 bit systems. A proof of concept for this can be triggered through this crafted file (SigLevel must be properly set for local files): $ uname -m i686 $ PKG=package-1.0.tar.xz $ touch $PKG $ echo "iQEcBAABCAAGBQJXTxJiAAr/////+wA=" | base64 -d - > $PKG.sig $ sudo pacman -U $PKG _ This proof of concept uses the ability to overflow position increments. By setting lengths to specific values ((size_t)-5 in this example), the position incrementation effectively ends up at its old position. This patch does not use GCC-specific overflow checking routines, because I am not sure if that's applicable for pacman's design goals. Signed-off-by: Tobias Stoeckmann --- Feedback is very appreciated. :) --- lib/libalpm/signing.c | 46 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c index 0267158..7dc7694 100644 --- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -982,6 +982,21 @@ int SYMEXPORT alpm_siglist_cleanup(alpm_siglist_t *siglist) return 0; } +#define LENGTH_CHECK(l, p, a) \ + do { \ + if ((a) == 0 || (l) - (p) <= (a)) { \ + _alpm_log(handle, ALPM_LOG_ERROR, \ + _("%s: signature format error"), identifier); \ + return -1; \ + } \ + } while(0) + +#define SAFE_ADD(l, p, a) \ + do { \ + LENGTH_CHECK((l), p, (a)); \ + p = p + (a); \ + } while(0) + /** * Extract the Issuer Key ID from a signature * @param sig PGP signature @@ -1018,18 +1033,21 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, switch(sig[pos] & 0x03) { case 0: + LENGTH_CHECK(len, pos, 1); blen = sig[pos + 1]; - pos = pos + 2; + SAFE_ADD(len, pos, 2); break; case 1: + LENGTH_CHECK(len, pos, 2); blen = (sig[pos + 1] << 8) | sig[pos + 2]; - pos = pos + 3; + SAFE_ADD(len, pos, 3); break; case 2: + LENGTH_CHECK(len, pos, 4); blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4]; - pos = pos + 5; + SAFE_ADD(len, pos, 5); break; case 3: @@ -1046,6 +1064,7 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, return -1; } + LENGTH_CHECK(len, pos, 1); if(sig[pos + 1] != 0x00) { /* not a signature of a binary document */ _alpm_log(handle, ALPM_LOG_ERROR, @@ -1053,32 +1072,38 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, return -1; } - pos = pos + 4; + SAFE_ADD(len, pos, 4); + LENGTH_CHECK(len, pos, 1); hlen = (sig[pos] << 8) | sig[pos + 1]; - pos = pos + hlen + 2; + SAFE_ADD(len, pos, hlen + 2); + LENGTH_CHECK(len, pos, 1); ulen = (sig[pos] << 8) | sig[pos + 1]; - pos = pos + 2; + SAFE_ADD(len, pos, 2); spos = pos; + LENGTH_CHECK(len, pos, ulen); while(spos < pos + ulen) { if(sig[spos] < 192) { slen = sig[spos]; spos = spos + 1; } else if(sig[spos] < 255) { + LENGTH_CHECK(pos + ulen, spos, 1); slen = (sig[spos] << 8) | sig[spos + 1]; - spos = spos + 2; + SAFE_ADD(pos + ulen, spos, 2); } else { + LENGTH_CHECK(pos + ulen, spos, 4); slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4]; - spos = spos + 5; + SAFE_ADD(pos + ulen, spos, 5); } if(sig[spos] == 16) { /* issuer key ID */ char key[17]; size_t i; + LENGTH_CHECK(pos + ulen, spos, 8); for (i = 0; i < 8; i++) { sprintf(&key[i * 2], "%02X", sig[spos + i + 1]); } @@ -1086,10 +1111,11 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, break; } - spos = spos + slen; + SAFE_ADD(pos + ulen + 1, spos, slen); } - pos = pos + (blen - hlen - 8); + LENGTH_CHECK(blen, hlen, 8); + SAFE_ADD(len + 1, pos, blen - hlen - 8); } return 0; -- 2.8.3