Merge pull request #783 from libtom/pr/der-misc-issues

various DER related fixes/improvements

Author: karel-m

src/headers/tomcrypt_private.h

@@ -372,8 +372,8 @@ struct bufp {
    char *start, *work, *end;
 };
 
-#define SET_BUFP(n, d, l) n.start = (char*)d, n.work = (char*)d, n.end = (char*)d + l + 1
-#define UPDATE_BUFP(n, d, w, l) n.start = (char*)d, n.work = (char*)d + w, n.end = (char*)d + l + 1
+#define SET_BUFP(n, d, l) n.start = (char*)d, n.work = (char*)d, n.end = (char*)d + l
+#define UPDATE_BUFP(n, d, w, l) n.start = (char*)d, n.work = (char*)d + w, n.end = (char*)d + l
 
 struct get_char;
 struct get_char_api {

src/misc/pem/pem_pkcs.c

@@ -73,7 +73,7 @@ static int s_import_pkcs8(unsigned char *asn1_cert, unsigned long asn1_len, ltc_
 {
    int err;
    enum ltc_oid_id oid_id;
-   ltc_asn1_list *alg_id, *priv_key;
+   ltc_asn1_list *alg_id = NULL, *priv_key = NULL;
    ltc_asn1_list *p8_asn1 = NULL;
    if ((err = pkcs8_decode_flexi(asn1_cert, asn1_len, pw_ctx, &p8_asn1)) != CRYPT_OK) {
       goto cleanup;

src/misc/pem/pem_read.c

@@ -21,7 +21,7 @@ static LTC_INLINE unsigned long s_bufp_alloc_len(struct bufp *buf)
 {
    if (buf->start == NULL || buf->end == NULL)
       return 0;
-   return buf->end - buf->start - 1;
+   return buf->end - buf->start;
 }
 
 static LTC_INLINE unsigned long s_bufp_used_len(struct bufp *buf)

src/pk/asn1/der/custom_type/der_decode_custom_type.c

@@ -341,8 +341,8 @@ int der_decode_custom_type_ex(const unsigned char *in,   unsigned long  inlen,
 
            case LTC_ASN1_SETOF:
            case LTC_ASN1_SEQUENCE:
-               /* detect if we have the right type */
-               if ((type == LTC_ASN1_SETOF && (in[x] & 0x3F) != 0x31) || (type == LTC_ASN1_SEQUENCE && (in[x] & 0x3F) != 0x30)) {
+               /* detect if we have the right type, the inlen test keeps in[x] inside the packet */
+               if ((inlen == 0) || (type == LTC_ASN1_SETOF && (in[x] & 0x3F) != 0x31) || (type == LTC_ASN1_SEQUENCE && (in[x] & 0x3F) != 0x30)) {
                   err = CRYPT_INVALID_PACKET;
                   goto LBL_ERR;
                }
@@ -390,6 +390,11 @@ int der_decode_custom_type_ex(const unsigned char *in,   unsigned long  inlen,
                err = CRYPT_INVALID_ARG;
                goto LBL_ERR;
        }
+       /* z is the canonical length of the decoded value, not always the octets consumed; without this test inlen wraps and the next element is read OOB */
+       if (z > inlen) {
+          err = CRYPT_INVALID_PACKET;
+          goto LBL_ERR;
+       }
        x           += z;
        inlen       -= z;
        list[i].used = 1;

src/pk/asn1/der/generalizedtime/der_decode_generalizedtime.c

@@ -31,12 +31,14 @@ static LTC_INLINE int s_char_to_int(unsigned char x)
 #endif
 
 #define DECODE_V(y, max) do {\
+   if (x + 2 > declen) return CRYPT_INVALID_PACKET; \
    y  = s_char_to_int(buf[x])*10 + s_char_to_int(buf[x+1]); \
    if (y >= max) return CRYPT_INVALID_PACKET;           \
    x += 2; \
 } while(0)
 
 #define DECODE_V4(y, max) do {\
+   if (x + 4 > declen) return CRYPT_INVALID_PACKET; \
    y  = s_char_to_int(buf[x])*1000 + s_char_to_int(buf[x+1])*100 + s_char_to_int(buf[x+2])*10 + s_char_to_int(buf[x+3]); \
    if (y >= max) return CRYPT_INVALID_PACKET; \
    x += 4; \
@@ -52,8 +54,8 @@ static LTC_INLINE int s_char_to_int(unsigned char x)
 int der_decode_generalizedtime(const unsigned char *in, unsigned long *inlen,
                                ltc_generalizedtime *out)
 {
-   unsigned char buf[32];
-   unsigned long x;
+   unsigned char buf[32] = { 0 }; /* initialize as all zeroes */
+   unsigned long x, declen;
    int           y;
 
    LTC_ARGCHK(in    != NULL);
@@ -78,9 +80,10 @@ int der_decode_generalizedtime(const unsigned char *in, unsigned long *inlen,
        }
        buf[x] = y;
    }
+   declen = x; /* octets decoded into buf - the parsing below must not read past them */
    *inlen = 2 + x;
 
-   if (x < 15) {
+   if (declen < 15) {
       return CRYPT_INVALID_PACKET;
    }
 
@@ -107,14 +110,16 @@ YYYYMMDDhhmmss.fs-hh'mm'
     out->fs = 0;
 
     /* now is it Z or . */
+    if (x >= declen) {
+       return CRYPT_INVALID_PACKET;
+    }
     if (buf[x] == 'Z') {
        return CRYPT_OK;
     }
     if (buf[x] == '.') {
        x++;
-       while (buf[x] >= '0' && buf[x] <= '9') {
+       while (x < declen && buf[x] >= '0' && buf[x] <= '9') {
           unsigned fs = out->fs;
-          if (x >= sizeof(buf)) return CRYPT_INVALID_PACKET;
           out->fs *= 10;
           out->fs += s_char_to_int(buf[x]);
           if (fs > out->fs) return CRYPT_OVERFLOW;
@@ -123,6 +128,9 @@ YYYYMMDDhhmmss.fs-hh'mm'
     }
 
     /* now is it Z, +, - */
+    if (x >= declen) {
+       return CRYPT_INVALID_PACKET;
+    }
     if (buf[x] == 'Z') {
        return CRYPT_OK;
     }

src/pk/asn1/der/integer/der_decode_integer.c

@@ -43,6 +43,11 @@ int der_decode_integer(const unsigned char *in, unsigned long inlen, void *num)
    }
    x += inlen;
 
+   /* an INTEGER holds at least one content octet, so in[x] below stays inside the packet */
+   if (y == 0) {
+      return CRYPT_INVALID_PACKET;
+   }
+
    if ((err = ltc_mp_read_unsigned_bin(num, (unsigned char *)in + x, y)) != CRYPT_OK) {
       return err;
    }

src/pk/asn1/der/sequence/der_flexi_sequence_cmp.c

@@ -41,6 +41,13 @@ int der_flexi_sequence_cmp(const ltc_asn1_list *flexi, der_flexi_check *check)
       cur = cur->next;
       check++;
    }
+   /* the loop above also ends when the decoded list runs out early; returning CRYPT_OK then would leave the caller's output pointers unassigned */
+   while (check->t != LTC_ASN1_EOL) {
+      if (!check->optional) {
+         return CRYPT_INVALID_PACKET;
+      }
+      check++;
+   }
    return CRYPT_OK;
 }
 

src/pk/asn1/der/short_integer/der_decode_short_integer.c

@@ -42,6 +42,11 @@ int der_decode_short_integer(const unsigned char *in, unsigned long inlen, unsig
       return CRYPT_INVALID_PACKET;
    }
 
+   /* an INTEGER holds at least one content octet */
+   if (len == 0) {
+      return CRYPT_INVALID_PACKET;
+   }
+
    if (len > sizeof(unsigned long)) {
       return CRYPT_OVERFLOW;
    }

src/pk/asn1/der/utctime/der_decode_utctime.c

@@ -30,6 +30,7 @@ static LTC_INLINE int s_char_to_int(unsigned char x)
 #endif
 
 #define DECODE_V(y, max) \
+   if (x + 2 > declen) return CRYPT_INVALID_PACKET;     \
    y  = s_char_to_int(buf[x])*10 + s_char_to_int(buf[x+1]); \
    if (y >= max) return CRYPT_INVALID_PACKET;           \
    x += 2;
@@ -45,7 +46,7 @@ int der_decode_utctime(const unsigned char *in, unsigned long *inlen,
                              ltc_utctime   *out)
 {
    unsigned char buf[32] = { 0 }; /* initialize as all zeroes */
-   unsigned long x;
+   unsigned long x, declen;
    int           y;
 
    LTC_ARGCHK(in    != NULL);
@@ -65,6 +66,7 @@ int der_decode_utctime(const unsigned char *in, unsigned long *inlen,
        }
        buf[x] = y;
    }
+   declen = x; /* octets decoded into buf - the parsing below must not read past them */
    *inlen = 2 + x;
 
 
@@ -90,6 +92,9 @@ YYMMDDhhmmss-hh'mm'
     out->off_dir = out->off_hh = out->off_mm = out->ss = 0;
 
     /* now is it Z, +, - or 0-9 */
+    if (x >= declen) {
+       return CRYPT_INVALID_PACKET;
+    }
     if (buf[x] == 'Z') {
        return CRYPT_OK;
     }
@@ -104,6 +109,9 @@ YYMMDDhhmmss-hh'mm'
     DECODE_V(out->ss, 60);
 
     /* now is it Z, +, - */
+    if (x >= declen) {
+       return CRYPT_INVALID_PACKET;
+    }
     if (buf[x] == 'Z') {
        return CRYPT_OK;
     }

src/pk/asn1/pkcs8/pkcs8_get.c

@@ -14,11 +14,16 @@ int pkcs8_get_children(const ltc_asn1_list *decoded_list, enum ltc_oid_id *pka,
    int err;
    unsigned long n;
    der_flexi_check flexi_should[4];
-   ltc_asn1_list *seq_l, *version;
+   ltc_asn1_list *seq_l = NULL, *priv_l = NULL, *version = NULL;
 
    LTC_ARGCHK(ltc_mp.name != NULL);
 
    if (alg_id == NULL) alg_id = &seq_l;
+   if (priv_key == NULL) priv_key = &priv_l;
+
+   /* der_flexi_sequence_cmp() writes only matched outputs, so unmatched ones stay NULL */
+   *alg_id = NULL;
+   *priv_key = NULL;
 
    /* Setup for basic structure */
    n=0;
@@ -35,13 +40,19 @@ int pkcs8_get_children(const ltc_asn1_list *decoded_list, enum ltc_oid_id *pka,
           * we get an 'input too long' error but the rest is already decoded and can be
           * handled the same as for version 0
           */
+         if (version == NULL) {
+            return CRYPT_INVALID_PACKET;
+         }
          if (ltc_mp_cmp_d(version->data, 0) != LTC_MP_EQ && ltc_mp_cmp_d(version->data, 1) != LTC_MP_EQ) {
             return CRYPT_INVALID_PACKET;
          }
          break;
       default:
          return err;
    }
+   if ((*alg_id == NULL) || ((*alg_id)->child == NULL) || (*priv_key == NULL)) {
+      return CRYPT_INVALID_PACKET;
+   }
    return pk_get_oid_from_asn1((*alg_id)->child, pka);
 }