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

diff --git a/decodekey.c b/decodekey.c
index 57a1e660754b1a7adf79284d028f248d23416b6d..522ad901383dd6a217b0ef5ed2320a93e3d51f8b 100644 (file)
--- 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 8d6306aeb2ac144086fb960a0122a23dcd87b05e..befc9e5805344b1da0dc505222d924e57e51ad83 100644 (file)
--- a/decodekey.h
+++ b/decodekey.h
@@ -24,6 +24,7 @@
 #include <time.h>
 #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 3ee8d3959e0e2db27e1c8c264d4b631cdec22896..d0ca3d1965e8a8c8fa3f2254d0b8f5ea3f8364ef 100644 (file)
--- 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;