From 5c7757d0a5f8aa480347cea7c3c92e2ebcc5dcde Mon Sep 17 00:00:00 2001 From: Karel Miko Date: Tue, 28 Jul 2026 00:10:29 +0200 Subject: [PATCH 1/2] various DER related fixes/improvements --- src/headers/tomcrypt_private.h | 4 ++-- src/misc/pem/pem_read.c | 2 +- .../der/custom_type/der_decode_custom_type.c | 9 +++++++-- .../der_decode_generalizedtime.c | 18 +++++++++++++----- src/pk/asn1/der/integer/der_decode_integer.c | 5 +++++ .../asn1/der/sequence/der_flexi_sequence_cmp.c | 7 +++++++ .../short_integer/der_decode_short_integer.c | 5 +++++ src/pk/asn1/der/utctime/der_decode_utctime.c | 10 +++++++++- src/pk/pkcs1/pkcs_1_v1_5_decode.c | 4 ++-- 9 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/headers/tomcrypt_private.h b/src/headers/tomcrypt_private.h index a823e9b4e..d45679b38 100644 --- a/src/headers/tomcrypt_private.h +++ b/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 { diff --git a/src/misc/pem/pem_read.c b/src/misc/pem/pem_read.c index bbc61cb0d..f364e43a8 100644 --- a/src/misc/pem/pem_read.c +++ b/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) diff --git a/src/pk/asn1/der/custom_type/der_decode_custom_type.c b/src/pk/asn1/der/custom_type/der_decode_custom_type.c index 60f5054c7..07f8f2d4b 100644 --- a/src/pk/asn1/der/custom_type/der_decode_custom_type.c +++ b/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; diff --git a/src/pk/asn1/der/generalizedtime/der_decode_generalizedtime.c b/src/pk/asn1/der/generalizedtime/der_decode_generalizedtime.c index b9c7d491c..ef9d888c4 100644 --- a/src/pk/asn1/der/generalizedtime/der_decode_generalizedtime.c +++ b/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; } diff --git a/src/pk/asn1/der/integer/der_decode_integer.c b/src/pk/asn1/der/integer/der_decode_integer.c index e2555b92a..36a2fa1f5 100644 --- a/src/pk/asn1/der/integer/der_decode_integer.c +++ b/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; } diff --git a/src/pk/asn1/der/sequence/der_flexi_sequence_cmp.c b/src/pk/asn1/der/sequence/der_flexi_sequence_cmp.c index 535483f59..2acb7d9ac 100644 --- a/src/pk/asn1/der/sequence/der_flexi_sequence_cmp.c +++ b/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; } diff --git a/src/pk/asn1/der/short_integer/der_decode_short_integer.c b/src/pk/asn1/der/short_integer/der_decode_short_integer.c index e00702d15..fc01fd15c 100644 --- a/src/pk/asn1/der/short_integer/der_decode_short_integer.c +++ b/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; } diff --git a/src/pk/asn1/der/utctime/der_decode_utctime.c b/src/pk/asn1/der/utctime/der_decode_utctime.c index 4c78c97f6..2f1beaabe 100644 --- a/src/pk/asn1/der/utctime/der_decode_utctime.c +++ b/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; } diff --git a/src/pk/pkcs1/pkcs_1_v1_5_decode.c b/src/pk/pkcs1/pkcs_1_v1_5_decode.c index 950749a62..f52b8c013 100644 --- a/src/pk/pkcs1/pkcs_1_v1_5_decode.c +++ b/src/pk/pkcs1/pkcs_1_v1_5_decode.c @@ -37,9 +37,9 @@ int ltc_pkcs_1_v1_5_decode(const unsigned char *msg, modulus_len = (modulus_bitlen >> 3) + (modulus_bitlen & 7 ? 1 : 0); - /* test message size */ + /* test message size - the encoded message is exactly as long as the modulus (RFC 8017 7.2.2, 8.2.2) and the code below reads modulus_len octets */ - if ((msglen > modulus_len) || (modulus_len < 11)) { + if ((msglen != modulus_len) || (modulus_len < 11)) { return CRYPT_PK_INVALID_SIZE; } From 209df8ae1abf3f32098a0b2690dc8ac286ff8815 Mon Sep 17 00:00:00 2001 From: Karel Miko Date: Tue, 28 Jul 2026 16:33:09 +0200 Subject: [PATCH 2/2] hardening pkcs8_get_children against uninitialized output pointers --- src/misc/pem/pem_pkcs.c | 2 +- src/pk/asn1/pkcs8/pkcs8_get.c | 13 ++++++++++++- src/pk/dh/dh_import_pkcs8.c | 2 +- src/pk/dsa/dsa_import_pkcs8.c | 2 +- src/pk/ec25519/ec25519_import_pkcs8.c | 2 +- src/pk/ec448/ec448_import_pkcs8.c | 2 +- src/pk/ecc/ecc_import_pkcs8.c | 2 +- src/pk/rsa/rsa_import_pkcs8.c | 2 +- 8 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/misc/pem/pem_pkcs.c b/src/misc/pem/pem_pkcs.c index 6a6d4e457..5f016e0be 100644 --- a/src/misc/pem/pem_pkcs.c +++ b/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; diff --git a/src/pk/asn1/pkcs8/pkcs8_get.c b/src/pk/asn1/pkcs8/pkcs8_get.c index 015d1e7f1..9dc34ab39 100644 --- a/src/pk/asn1/pkcs8/pkcs8_get.c +++ b/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,6 +40,9 @@ 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; } @@ -42,6 +50,9 @@ int pkcs8_get_children(const ltc_asn1_list *decoded_list, enum ltc_oid_id *pka, 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); } diff --git a/src/pk/dh/dh_import_pkcs8.c b/src/pk/dh/dh_import_pkcs8.c index d1fd792bb..2962bb1d0 100644 --- a/src/pk/dh/dh_import_pkcs8.c +++ b/src/pk/dh/dh_import_pkcs8.c @@ -61,7 +61,7 @@ int dh_import_pkcs8(const unsigned char *in, unsigned long inlen, { int err; ltc_asn1_list *l = NULL; - ltc_asn1_list *alg_id, *priv_key; + ltc_asn1_list *alg_id = NULL, *priv_key = NULL; enum ltc_oid_id pka; LTC_ARGCHK(in != NULL); diff --git a/src/pk/dsa/dsa_import_pkcs8.c b/src/pk/dsa/dsa_import_pkcs8.c index b3bc9c953..974631ead 100644 --- a/src/pk/dsa/dsa_import_pkcs8.c +++ b/src/pk/dsa/dsa_import_pkcs8.c @@ -62,7 +62,7 @@ int dsa_import_pkcs8(const unsigned char *in, unsigned long inlen, { int err; ltc_asn1_list *l = NULL; - ltc_asn1_list *alg_id, *priv_key; + ltc_asn1_list *alg_id = NULL, *priv_key = NULL; enum ltc_oid_id pka; LTC_ARGCHK(in != NULL); diff --git a/src/pk/ec25519/ec25519_import_pkcs8.c b/src/pk/ec25519/ec25519_import_pkcs8.c index 4a7f9fb9f..9f9a6bcb8 100644 --- a/src/pk/ec25519/ec25519_import_pkcs8.c +++ b/src/pk/ec25519/ec25519_import_pkcs8.c @@ -60,7 +60,7 @@ int ec25519_import_pkcs8(const unsigned char *in, unsigned long inlen, { int err; ltc_asn1_list *l = NULL; - ltc_asn1_list *alg_id, *priv_key; + ltc_asn1_list *alg_id = NULL, *priv_key = NULL; enum ltc_oid_id pka; LTC_ARGCHK(in != NULL); diff --git a/src/pk/ec448/ec448_import_pkcs8.c b/src/pk/ec448/ec448_import_pkcs8.c index 0de083472..37ffba3b9 100644 --- a/src/pk/ec448/ec448_import_pkcs8.c +++ b/src/pk/ec448/ec448_import_pkcs8.c @@ -60,7 +60,7 @@ int ec448_import_pkcs8(const unsigned char *in, unsigned long inlen, { int err; ltc_asn1_list *l = NULL; - ltc_asn1_list *alg_id, *priv_key; + ltc_asn1_list *alg_id = NULL, *priv_key = NULL; enum ltc_oid_id pka; LTC_ARGCHK(in != NULL); diff --git a/src/pk/ecc/ecc_import_pkcs8.c b/src/pk/ecc/ecc_import_pkcs8.c index 9df67890f..ad32fa07d 100644 --- a/src/pk/ecc/ecc_import_pkcs8.c +++ b/src/pk/ecc/ecc_import_pkcs8.c @@ -140,7 +140,7 @@ int ecc_import_pkcs8(const unsigned char *in, unsigned long inlen, { int err; ltc_asn1_list *l = NULL; - ltc_asn1_list *alg_id, *priv_key; + ltc_asn1_list *alg_id = NULL, *priv_key = NULL; enum ltc_oid_id pka; LTC_ARGCHK(key != NULL); diff --git a/src/pk/rsa/rsa_import_pkcs8.c b/src/pk/rsa/rsa_import_pkcs8.c index 0a4570c03..406772e0f 100644 --- a/src/pk/rsa/rsa_import_pkcs8.c +++ b/src/pk/rsa/rsa_import_pkcs8.c @@ -44,7 +44,7 @@ int rsa_import_pkcs8(const unsigned char *in, unsigned long inlen, { int err; ltc_asn1_list *l = NULL; - ltc_asn1_list *alg_id, *priv_key; + ltc_asn1_list *alg_id = NULL, *priv_key = NULL; enum ltc_oid_id pka; LTC_ARGCHK(in != NULL);