From ec38d8bd7e8a66645e75e3d6c8b9dadb5dd85ec7 Mon Sep 17 00:00:00 2001 From: Jonathan McDowell Date: Thu, 28 Nov 2013 13:30:06 -0800 Subject: [PATCH] Check that signature data lengths do not exceed the available data Some signatures have been witnessed on the keyserver network where the hashed data lengths reported exceed the size of the overall packet. It is assumed these are the results of some corruption somewhere, so if they're detected don't try to parse any further (and drop them if we're doing signature checking). --- decodekey.c | 48 +++++++++++++++++++++++++++++++++++------------- decodekey.h | 9 +++++++-- sigcheck.c | 13 ++++++++++++- 3 files changed, 54 insertions(+), 16 deletions(-) diff --git a/decodekey.c b/decodekey.c index 57a1e66..522ad90 100644 --- a/decodekey.c +++ b/decodekey.c @@ -34,6 +34,8 @@ /* * parse_subpackets - Parse the subpackets of a Type 4 signature. * @data: The subpacket data. + * @len: The amount of data available to read. + * @parselen: The amount of data that was actually parsed. * @keyid: A pointer to where we should return the keyid. * @creationtime: A pointer to where we should return the creation time. * @@ -42,7 +44,8 @@ * processed. If the value of any piece of data is not desired a NULL * can be passed instead of a pointer to a storage area for that value. */ -int parse_subpackets(unsigned char *data, uint64_t *keyid, time_t *creation) +onak_status_t parse_subpackets(unsigned char *data, size_t len, + size_t *parselen, uint64_t *keyid, time_t *creation) { int offset = 0; int length = 0; @@ -50,8 +53,20 @@ int parse_subpackets(unsigned char *data, uint64_t *keyid, time_t *creation) log_assert(data != NULL); + /* Make sure we actually have the 2 byte length field */ + if (len < 2) { + return ONAK_E_INVALID_PKT; + } + length = (data[0] << 8) + data[1] + 2; + /* If the length is off the end of the data available, it's bogus */ + if (len < length) { + return ONAK_E_INVALID_PKT; + } + + *parselen = length; + offset = 2; while (offset < length) { packetlen = data[offset++]; @@ -152,7 +167,7 @@ int parse_subpackets(unsigned char *data, uint64_t *keyid, time_t *creation) offset += packetlen; } - return length; + return ONAK_E_OK; } /** @@ -187,10 +202,12 @@ struct ll *keysigs(struct ll *curll, * key or pulls the data directly from v2/3. NULL can be passed for any * values which aren't cared about. */ -void sig_info(struct openpgp_packet *packet, uint64_t *keyid, time_t *creation) +onak_status_t sig_info(struct openpgp_packet *packet, uint64_t *keyid, + time_t *creation) { - int length = 0; - + size_t length = 0; + onak_status_t res; + if (packet != NULL) { switch (packet->data[0]) { case 2: @@ -223,20 +240,25 @@ void sig_info(struct openpgp_packet *packet, uint64_t *keyid, time_t *creation) } break; case 4: - length = parse_subpackets(&packet->data[4], - keyid, creation); - parse_subpackets(&packet->data[length + 4], - keyid, creation); - /* - * Don't bother to look at the unsigned packets. - */ + res = parse_subpackets(&packet->data[4], + packet->length - 4, + &length, keyid, creation); + if (res != ONAK_E_OK) { + return res; + } + res = parse_subpackets(&packet->data[length + 4], + packet->length - (4 + length), + &length, keyid, creation); + if (res != ONAK_E_OK) { + return res; + } break; default: break; } } - return; + return ONAK_E_OK; } /** diff --git a/decodekey.h b/decodekey.h index 8d6306a..befc9e5 100644 --- a/decodekey.h +++ b/decodekey.h @@ -24,6 +24,7 @@ #include #include "keystructs.h" #include "ll.h" +#include "onak.h" /** * keysigs - Return the sigs on a given OpenPGP signature packet list. @@ -46,7 +47,8 @@ struct ll *keysigs(struct ll *curll, * key or pulls the data directly from v2/3. NULL can be passed for any * values which aren't cared about. */ -void sig_info(struct openpgp_packet *packet, uint64_t *keyid, time_t *creation); +onak_status_t sig_info(struct openpgp_packet *packet, uint64_t *keyid, + time_t *creation); /** * sig_keyid - Return the keyid for a given OpenPGP signature packet. @@ -79,6 +81,8 @@ struct openpgp_fingerprint *keysubkeys(struct openpgp_publickey *key); /** * parse_subpackets - Parse the subpackets of a Type 4 signature. * @data: The subpacket data. + * @len: The amount of data available to read. + * @parselen: The amount of data that was actually parsed. * @keyid: A pointer to where we should return the keyid. * @creationtime: A pointer to where we should return the creation time. * @@ -87,6 +91,7 @@ struct openpgp_fingerprint *keysubkeys(struct openpgp_publickey *key); * processed. If the value of any piece of data is not desired a NULL * can be passed instead of a pointer to a storage area for that value. */ -int parse_subpackets(unsigned char *data, uint64_t *keyid, time_t *creation); +onak_status_t parse_subpackets(unsigned char *data, size_t len, + size_t *parselen, uint64_t *keyid, time_t *creation); #endif diff --git a/sigcheck.c b/sigcheck.c index 3ee8d39..d0ca3d1 100644 --- a/sigcheck.c +++ b/sigcheck.c @@ -70,6 +70,7 @@ int check_packet_sighash(struct openpgp_publickey *key, size_t hashlen[8]; int chunks, i; uint64_t keyid; + onak_status_t res; keyheader[0] = 0x99; keyheader[1] = key->publickey->length >> 8; @@ -114,7 +115,13 @@ int check_packet_sighash(struct openpgp_publickey *key, size_t len; keyid = 0; - len = parse_subpackets(&sig->data[4], &keyid, NULL); + res = parse_subpackets(&sig->data[4], + sig->length - 4, &len, + &keyid, NULL); + if (res != ONAK_E_OK) { + /* If it parses badly, reject it */ + return 0; + } if (keyid == 0 && /* No unhashed data */ sig->data[4 + len] == 0 && @@ -163,6 +170,10 @@ int check_packet_sighash(struct openpgp_publickey *key, hashdata[chunks] = sig->data; hashlen[chunks] = siglen = (sig->data[4] << 8) + sig->data[5] + 6;; + if (siglen > sig->length) { + /* Signature data exceed packet length, bogus */ + return 0; + } chunks++; v4trailer[0] = 4; -- 2.39.5