diff --git a/src/crl.c b/src/crl.c index bc3c24e2f8d..6ad6bff590f 100644 --- a/src/crl.c +++ b/src/crl.c @@ -2621,6 +2621,13 @@ int wolfSSL_X509_CRL_add_revoked(WOLFSSL_X509_CRL* crl, } { + /* The serial is the bare value octets - no DER tag or length. That is + * what wolfSSL_X509_CRL_get_REVOKED() and + * wolfSSL_X509_CRL_add_revoked_cert() produce, and the only layout + * this function has ever accepted, so it is read without consulting + * dataIsRaw. Reading a DER INTEGER here instead would misread a bare + * value that happens to form one: the serial 02 02 00 00, for + * instance, is a valid DER encoding of zero. */ const byte* serial = rev->serialNumber->data; int serialSz = rev->serialNumber->length; int i; @@ -2764,6 +2771,8 @@ int wolfSSL_X509_CRL_add_revoked_cert(WOLFSSL_X509_CRL* crl, XMEMCPY(serialInt->data, cert->serial, cert->serialSz); serialInt->length = cert->serialSz; + /* The certificate's serial is the value, with no DER tag or length. */ + serialInt->dataIsRaw = 1; XMEMSET(&revoked, 0, sizeof(revoked)); revoked.serialNumber = serialInt; diff --git a/src/ocsp.c b/src/ocsp.c index 2d49b0dcbd2..0499060c48f 100644 --- a/src/ocsp.c +++ b/src/ocsp.c @@ -2138,12 +2138,15 @@ int wolfSSL_OCSP_id_get0_info(WOLFSSL_ASN1_STRING **name, XMEMCPY(&ser->data[i], cid->status->serial, (size_t)cid->status->serialSz); ser->length = cid->status->serialSz; + /* Qt and HAProxy read data/length as the value octets. */ + ser->dataIsRaw = 1; #else ser->data[i++] = ASN_INTEGER; i += SetLength(cid->status->serialSz, ser->data + i); XMEMCPY(&ser->data[i], cid->status->serial, (size_t)cid->status->serialSz); ser->length = i + cid->status->serialSz; + ser->dataIsRaw = 0; #endif cid->status->serialInt = ser; diff --git a/src/pk.c b/src/pk.c index 8b91ff04268..c79fef2928c 100644 --- a/src/pk.c +++ b/src/pk.c @@ -7492,7 +7492,12 @@ int pkcs8_encrypt(WOLFSSL_EVP_PKEY* pkey, } else #endif /* WOLFSSL_HAVE_MLDSA */ - { + if ((pkey->pkey.ptr == NULL) || (pkey->pkey_sz <= 0) || + (pkey->pkcs8HeaderSz >= pkey->pkey_sz)) { + /* The PKCS#8 header size must lie inside the DER encoding. */ + ret = BAD_FUNC_ARG; + } + else { /* Encrypt private into buffer. */ ret = TraditionalEnc( (byte*)pkey->pkey.ptr + pkey->pkcs8HeaderSz, @@ -7639,6 +7644,11 @@ int pkcs8_encode(WOLFSSL_EVP_PKEY* pkey, byte* key, word32* keySz) ret = NOT_COMPILED_IN; } + /* The PKCS#8 header size must lie inside the DER encoding. */ + if ((ret >= 0) && ((pkey->pkey.ptr == NULL) || (pkey->pkey_sz <= 0) || + (pkey->pkcs8HeaderSz >= pkey->pkey_sz))) { + ret = BAD_FUNC_ARG; + } if (ret >= 0) { /* Encode private key in PKCS#8 format. */ ret = wc_CreatePKCS8Key(key, keySz, (byte*)pkey->pkey.ptr + diff --git a/src/pk_ec.c b/src/pk_ec.c index 05459162bd1..60c54aca93b 100644 --- a/src/pk_ec.c +++ b/src/pk_ec.c @@ -1858,11 +1858,14 @@ int wolfSSL_EC_POINT_is_on_curve(const WOLFSSL_EC_GROUP *group, #if !defined(WOLFSSL_SP_MATH) && !defined(WOLF_CRYPTO_CB_ONLY_ECC) /* Convert Jacobian ordinates to affine. + * + * Note the return convention: zero is success, the opposite of most functions + * here. Both callers treat a non-zero return as an error. * * @param [in] group EC group. - * @param [in] point EC point to get coordinates from. - * @return 1 on success. - * @return 0 on error. + * @param [in, out] point EC point to convert. + * @return 0 on success. + * @return 1 on error. */ int ec_point_convert_to_affine(const WOLFSSL_EC_GROUP *group, WOLFSSL_EC_POINT *point) @@ -1881,8 +1884,16 @@ int ec_point_convert_to_affine(const WOLFSSL_EC_GROUP *group, mp_digit mp = 0; WC_DECLARE_VAR(modulus, mp_int, 1, 0); + /* Curve parameters are only available for curves in ecc_sets. */ + if ((group->curve_idx < 0) || (!wc_ecc_is_valid_idx(group->curve_idx))) { + WOLFSSL_MSG("ec_point_convert_to_affine curve index error"); + err = 1; + } + /* Allocate memory for curve's prime modulus. */ - WC_ALLOC_VAR_EX(modulus, mp_int, 1, NULL, DYNAMIC_TYPE_BIGINT, err=1); + if (!err) { + WC_ALLOC_VAR_EX(modulus, mp_int, 1, NULL, DYNAMIC_TYPE_BIGINT, err=1); + } /* Initialize the MP integer. */ if ((!err) && (mp_init(modulus) != MP_OKAY)) { WOLFSSL_MSG("mp_init failed"); @@ -2250,6 +2261,12 @@ int wolfSSL_EC_POINT_add(const WOLFSSL_EC_GROUP* group, WOLFSSL_EC_POINT* r, WOLFSSL_MSG("wolfSSL_EC_POINT_add error"); ret = 0; } + /* Curve parameters are only available for curves in ecc_sets. */ + if ((ret == 1) && ((group->curve_idx < 0) || + (!wc_ecc_is_valid_idx(group->curve_idx)))) { + WOLFSSL_MSG("wolfSSL_EC_POINT_add Bad group idx"); + ret = 0; + } /* Ensure the internal objects of the EC points are setup. */ if ((ret == 1) && ((ec_point_setup(r) != 1) || (ec_point_setup(p1) != 1) || @@ -2515,6 +2532,12 @@ int wolfSSL_EC_POINT_mul(const WOLFSSL_EC_GROUP *group, WOLFSSL_EC_POINT *r, WOLFSSL_MSG("wolfSSL_EC_POINT_mul NULL error"); ret = 0; } + /* Curve parameters are only available for curves in ecc_sets. */ + if ((ret == 1) && ((group->curve_idx < 0) || + (!wc_ecc_is_valid_idx(group->curve_idx)))) { + WOLFSSL_MSG("wolfSSL_EC_POINT_mul Bad group idx"); + ret = 0; + } /* Ensure the internal representation of the EC point q is setup. */ if ((ret == 1) && (q != NULL) && (ec_point_setup(q) != 1)) { @@ -2638,6 +2661,12 @@ int wolfSSL_EC_POINT_invert(const WOLFSSL_EC_GROUP *group, if ((group == NULL) || (point == NULL) || (point->internal == NULL)) { ret = 0; } + /* Curve parameters are only available for curves in ecc_sets. */ + if ((ret == 1) && ((group->curve_idx < 0) || + (!wc_ecc_is_valid_idx(group->curve_idx)))) { + WOLFSSL_MSG("wolfSSL_EC_POINT_invert Bad group idx"); + ret = 0; + } /* Ensure internal representation of point is setup. */ if ((ret == 1) && (ec_point_setup(point) != 1)) { @@ -2720,6 +2749,11 @@ static int ec_point_cmp_jacobian(const WOLFSSL_EC_GROUP* group, (mod == NULL)) { ret = WOLFSSL_FATAL_ERROR; } + /* Curve parameters are only available for curves in ecc_sets. */ + if ((ret == 0) && ((group->curve_idx < 0) || + (!wc_ecc_is_valid_idx(group->curve_idx)))) { + ret = WOLFSSL_FATAL_ERROR; + } /* Get the modulus for the curve. */ if ((ret == 0) && (BN_hex2bn(&mod, ecc_sets[group->curve_idx].prime) != 1)) { diff --git a/src/ssl_asn1.c b/src/ssl_asn1.c index 695735a4632..cdd4d8cbbd8 100644 --- a/src/ssl_asn1.c +++ b/src/ssl_asn1.c @@ -32,6 +32,76 @@ #endif #else +#ifndef NO_ASN +#if defined(OPENSSL_EXTRA) || defined(WOLFSSL_WPAS_SMALL) || \ + defined(OPENSSL_EXTRA_X509_SMALL) +/* Locate the value octets of an ASN.1 INTEGER object. + * + * Placed outside the OPENSSL_EXTRA_NO_ASN1 and OPENSSL_EXTRA guards that cover + * the rest of this file, because X509_set_serialNumber() is built whenever + * OPENSSL_EXTRA_X509_SMALL is defined and does not need the ASN1 object APIs. + * This has to be available whenever any consumer of it is. + * + * Which layout a->data uses is recorded in a->dataIsRaw by whoever filled the + * object in, so the value octets are never inspected to work it out. That + * matters because a value may legitimately be byte-identical to a DER INTEGER + * encoding - a CRL revoked serial of 02 01 05, say - and guessing truncates + * it. + * + * An empty object is the value zero whichever layout is claimed. + * + * @param [in] a ASN.1 INTEGER object. Must not be NULL. + * @param [out] idx Index of the first value octet in a->data. + * @param [out] len Number of value octets. + * @return 1 on success. + * @return 0 when data and length are not consistent with each other, or the + * object claims a DER layout that its data does not have. + */ +int wolfssl_asn1_integer_value(const WOLFSSL_ASN1_INTEGER* a, word32* idx, + int* len) +{ + int ret = 1; + + /* Both layouts read a->length bytes of a->data, so the length has to lie + * inside the buffer. */ + if ((a->data == NULL) || (a->length < 0) || + ((word32)a->length > a->dataMax)) { + ret = 0; + } + /* No data: the value is zero, which encodes as a single zero octet. */ + else if (a->length == 0) { + *idx = 0; + *len = 0; + } + else if (a->dataIsRaw) { + /* Value octets on their own. */ + *idx = 0; + *len = a->length; + } + else { + word32 i = 1; + int l = 0; + + /* A DER INTEGER whose tag and length span exactly a->length. The + * object says it is DER, so malformed data is an error rather than a + * cue to treat it as a value. */ + if ((a->data[0] != ASN_INTEGER) || + (GetLength(a->data, &i, &l, (word32)a->length) < 0) || + (i + (word32)l != (word32)a->length)) { + WOLFSSL_MSG("ASN.1 INTEGER claims DER but data is not"); + ret = 0; + } + else { + *idx = i; + *len = l; + } + } + + return ret; +} +#endif +#endif /* !NO_ASN */ + /******************************************************************************* * ASN1_item APIs ******************************************************************************/ @@ -1021,6 +1091,9 @@ WOLFSSL_ASN1_INTEGER* wolfSSL_ASN1_INTEGER_new(void) a->dataMax = WOLFSSL_ASN1_INTEGER_MAX; /* No value set - no data. */ a->length = 0; + /* dataIsRaw is left zero by the XMEMSET above: a DER layout, which is + * what every producer in this file writes. The two that write bare + * value octets set it. */ } return a; @@ -1050,21 +1123,14 @@ void wolfSSL_ASN1_INTEGER_free(WOLFSSL_ASN1_INTEGER* in) */ int wolfSSL_ASN1_INTEGER_get_length(const WOLFSSL_ASN1_INTEGER* ai) { - if (ai == NULL || ai->data == NULL || ai->length <= 0) { + word32 idx = 0; + int len = 0; + + if ((ai == NULL) || (wolfssl_asn1_integer_value(ai, &idx, &len) != 1)) { return 0; } - if (ai->data[0] == ASN_INTEGER) { - word32 idx = 1; - int len = 0; - if (GetLength(ai->data, &idx, &len, (word32)ai->length) >= 0 && - idx + (word32)len == (word32)ai->length) { - return len; - } - } - /* WOLFSSL_QT / WOLFSSL_HAPROXY format: raw bytes without DER header, - * or data that coincidentally starts with 0x02 but whose header+value - * boundaries do not span exactly ai->length. */ - return ai->length; + + return len; } /* Get a pointer to the raw integer value bytes, skipping the DER tag/length @@ -1077,21 +1143,15 @@ int wolfSSL_ASN1_INTEGER_get_length(const WOLFSSL_ASN1_INTEGER* ai) */ const unsigned char* wolfSSL_ASN1_INTEGER_get0_data(const WOLFSSL_ASN1_INTEGER* ai) { - if (ai == NULL || ai->data == NULL || ai->length <= 0) { + word32 idx = 0; + int len = 0; + + if ((ai == NULL) || (ai->length <= 0) || + (wolfssl_asn1_integer_value(ai, &idx, &len) != 1)) { return NULL; } - if (ai->data[0] == ASN_INTEGER) { - word32 idx = 1; - int len = 0; - if (GetLength(ai->data, &idx, &len, (word32)ai->length) >= 0 && - idx + (word32)len == (word32)ai->length) { - return ai->data + idx; - } - } - /* WOLFSSL_QT / WOLFSSL_HAPROXY format: raw bytes without DER header, - * or data that coincidentally starts with 0x02 but whose header+value - * boundaries do not span exactly ai->length. */ - return ai->data; + + return ai->data + idx; } #if defined(OPENSSL_EXTRA) @@ -1120,6 +1180,8 @@ static void wolfssl_asn1_integer_reset_data(WOLFSSL_ASN1_INTEGER* a) a->length = 0; /* No data, not negative. */ a->negative = 0; + /* Back to the default layout along with the data. */ + a->dataIsRaw = 0; /* Set type to positive INTEGER. */ a->type = WOLFSSL_V_ASN1_INTEGER; } @@ -1221,6 +1283,7 @@ static WOLFSSL_ASN1_INTEGER* wolfssl_asn1_integer_new_buf( } XMEMCPY(a->data + i, val, len); a->length = (int)(len + i); + a->dataIsRaw = 0; a->type = WOLFSSL_V_ASN1_INTEGER; return a; @@ -1246,8 +1309,9 @@ WOLFSSL_ASN1_INTEGER* wolfSSL_ASN1_INTEGER_dup(const WOLFSSL_ASN1_INTEGER* src) } /* Check for object to copy into. */ if (dst != NULL) { - /* Copy simple fields. */ - dst->length = src->length; + /* Copy simple fields. The copy has the same layout as the + * original. */ + dst->dataIsRaw = src->dataIsRaw; dst->negative = src->negative; dst->type = src->type; @@ -1262,6 +1326,12 @@ WOLFSSL_ASN1_INTEGER* wolfSSL_ASN1_INTEGER_dup(const WOLFSSL_ASN1_INTEGER* src) else { XMEMCPY(dst->data, src->data, (size_t)src->length); } + if (dst != NULL) { + /* Set the length last: wolfssl_asn1_integer_require_len() clears + * it when told not to keep the old data, which would leave a + * heap-backed copy looking empty. */ + dst->length = src->length; + } } return dst; @@ -1270,6 +1340,49 @@ WOLFSSL_ASN1_INTEGER* wolfSSL_ASN1_INTEGER_dup(const WOLFSSL_ASN1_INTEGER* src) #if defined(OPENSSL_EXTRA) +/* Numeric magnitude of the value octets of an ASN.1 INTEGER. + * + * Used only by wolfSSL_ASN1_INTEGER_cmp(), to compare an encoded value against + * an object that keeps its value in ->length and so has no value octets to + * compare byte for byte. + * + * @param [in] data Buffer holding the value octets. + * @param [in] idx Index of the first value octet in data. + * @param [in] len Number of value octets. + * @param [out] mag Magnitude of the value. Only set on success. + * @return 1 when the magnitude is returned in mag. + * @return 0 when the value needs more octets than mag holds, and so is + * greater than any value an int ->length can carry. + */ +static int wolfssl_asn1_integer_magnitude(const unsigned char* data, + word32 idx, int len, word32* mag) +{ + int ret = 1; + + /* Leading zeroes are not part of the magnitude: a DER value carries one + * whenever its top bit is set. */ + while ((len > 0) && (data[idx] == 0)) { + idx++; + len--; + } + + if (len > (int)sizeof(*mag)) { + ret = 0; + } + else { + word32 m = 0; + + while (len > 0) { + m = (m << 8) | data[idx]; + idx++; + len--; + } + *mag = m; + } + + return ret; +} + /* Compare values in two ASN.1 INTEGER objects. * * @param [in] a First ASN.1 INTEGER object. @@ -1300,17 +1413,104 @@ int wolfSSL_ASN1_INTEGER_cmp(const WOLFSSL_ASN1_INTEGER* a, ret = 1; } else { - /* Check for difference in length. */ - if (a->length != b->length) { - ret = a->length - b->length; + word32 aIdx = 0; + word32 bIdx = 0; + int aLen = 0; + int bLen = 0; + /* Compare the values, not the encodings: the two objects may record + * different layouts, so a->data and b->data are not comparable + * directly. */ + int aOk = wolfssl_asn1_integer_value(a, &aIdx, &aLen); + int bOk = wolfssl_asn1_integer_value(b, &bIdx, &bLen); + + if ((aOk != 1) || (bOk != 1)) { + /* At least one of them holds no value in the layout it records, so + * that side has no value octets to compare. + * + * That is not just leniency towards malformed input: the library + * itself produces such objects for a basic constraints + * pathLenConstraint. X509_get_ext() and X509_get_ext_d2i() put the + * path length in ->length and leave ->data zeroed (x509.c), a + * convention the public pathlen fields of WOLFSSL_ASN1_OBJECT and + * WOLFSSL_BASIC_CONSTRAINTS expose to applications, so it cannot + * be changed here. + * + * Compare numeric values: ->length is the value on that side, and + * the other side, when it does hold an encoded value, is decoded + * to a number so the two are comparable. Taking ->length from both + * sides instead would compare a value against an octet count - a + * path length of 3 would come out equal to both + * ASN1_INTEGER_set(, 1) and ASN1_INTEGER_set(, 127), which are + * three octets of DER each and unequal to one another. + * + * ->data is deliberately not read on a side that holds no value: + * there ->length is a value, not a byte count, and so is not + * bounded by the buffer. */ + word32 aVal = 0; + word32 bVal = 0; + int aWide = 0; + int bWide = 0; + + if (aOk == 1) { + aWide = (wolfssl_asn1_integer_magnitude(a->data, aIdx, aLen, + &aVal) != 1); + } + else if (a->length > 0) { + aVal = (word32)a->length; + } + if (bOk == 1) { + bWide = (wolfssl_asn1_integer_magnitude(b->data, bIdx, bLen, + &bVal) != 1); + } + else if (b->length > 0) { + bVal = (word32)b->length; + } + + /* Magnitudes of 2 rather than 1: negating a 1 for the both + * negative case below would produce WOLFSSL_FATAL_ERROR, which is + * this function's NULL result, as noted above. + * + * Only a side holding an encoded value can be too wide to decode, + * and only one side can be holding one here, so the two cannot be + * wide at once. */ + if (aWide != bWide) { + ret = aWide ? 2 : -2; + } + else if (aVal != bVal) { + ret = (aVal > bVal) ? 2 : -2; + } + if (a->negative) { + ret = -ret; + } } else { - /* Compare data given they are the same length. */ - ret = XMEMCMP(a->data, b->data, (size_t)a->length); - } - /* Reverse comparison result when both negative. */ - if (a->negative) { - ret = -ret; + /* Discard leading zeroes so the two magnitudes are comparable. + * A DER value carries a 0x00 pad whenever its top bit is set, so + * 128 is 00 80 there but 80 as bare value octets, and zero is 00 + * as a DER value but may be empty as a bare one. Neither pair + * differs in value. OpenSSL stores the magnitude without the pad, + * so this is also what it compares. */ + while ((aLen > 0) && (a->data[aIdx] == 0)) { + aIdx++; + aLen--; + } + while ((bLen > 0) && (b->data[bIdx] == 0)) { + bIdx++; + bLen--; + } + /* Check for difference in length. Both are normalized, so the + * shorter magnitude is the smaller number. */ + if (aLen != bLen) { + ret = aLen - bLen; + } + else if (aLen > 0) { + /* Compare values given they are the same length. */ + ret = XMEMCMP(a->data + aIdx, b->data + bIdx, (size_t)aLen); + } + /* Reverse comparison result when both negative. */ + if (a->negative) { + ret = -ret; + } } } @@ -1480,6 +1680,8 @@ WOLFSSL_ASN1_INTEGER* wolfSSL_d2i_ASN1_INTEGER(WOLFSSL_ASN1_INTEGER** a, /* Copy DER encoding and length. */ XMEMCPY(ret->data, *in, (size_t)(idx + (word32)len)); ret->length = (int)idx + len; + /* Decoded a DER INTEGER into data, header included. */ + ret->dataIsRaw = 0; /* Do 2's complement if number is negative. */ if (wolfssl_asn1_int_twos_compl(ret->data, ret->length, &ret->negative) != 0) { @@ -1634,6 +1836,8 @@ int wolfSSL_a2i_ASN1_INTEGER(WOLFSSL_BIO *bio, WOLFSSL_ASN1_INTEGER *asn1, SetASNInt(asn1->length, asn1->data[idx], asn1->data); /* Update length of data. */ asn1->length += idx; + /* The header was just written in, so data is a DER INTEGER. */ + asn1->dataIsRaw = 0; } return ret; @@ -1760,7 +1964,8 @@ static void wolfssl_asn1_integer_pad(unsigned char* data, int len, /* Convert ASN.1 INTEGER object into content octets. * - * TODO: compatibility with OpenSSL? OpenSSL assumes data not DER encoded. + * Data may be DER encoded (tag, length, value) or, as in OpenSSL, the raw + * value bytes. In both cases a->length bounds the bytes read from a->data. * * When pp points to a buffer, on success pp will point to after the encoded * data. @@ -1768,7 +1973,8 @@ static void wolfssl_asn1_integer_pad(unsigned char* data, int len, * @param [in] a ASN.1 INTEGER object. * @param [in, out] pp Pointer to buffer. May be NULL. Cannot point to NULL. * @return Length of encoding on success. - * @return 0 when a is NULL, pp points to NULL or DER length encoding invalid. + * @return 0 when a is NULL, pp points to NULL, or a's data and length are + * not consistent with each other. */ int wolfSSL_i2c_ASN1_INTEGER(WOLFSSL_ASN1_INTEGER *a, unsigned char **pp) { @@ -1785,8 +1991,8 @@ int wolfSSL_i2c_ASN1_INTEGER(WOLFSSL_ASN1_INTEGER *a, unsigned char **pp) err = 1; } - /* Get length from DER encoding. */ - if ((!err) && (GetLength_ex(a->data, &idx, &len, a->dataMax, 0) < 0)) { + /* Locate the value octets using the layout the object records. */ + if ((!err) && (wolfssl_asn1_integer_value(a, &idx, &len) != 1)) { err = 1; } @@ -1842,16 +2048,12 @@ WOLFSSL_BIGNUM *wolfSSL_ASN1_INTEGER_to_BN(const WOLFSSL_ASN1_INTEGER *ai, } if (!err) { - /* Get the length of ASN.1 INTEGER number. */ - if ((ai->data[0] != ASN_INTEGER) || (GetLength(ai->data, &idx, &len, - (word32)ai->length) <= 0)) { - #if defined(WOLFSSL_QT) || defined(WOLFSSL_HAPROXY) - idx = 0; - len = ai->length; - #else - WOLFSSL_MSG("Data in WOLFSSL_ASN1_INTEGER not DER encoded"); + /* Locate the value octets using the layout the object records. This + * used to require a DER layout outside WOLFSSL_QT/WOLFSSL_HAPROXY + * builds, which made every bare-value object - X509_CRL_get_REVOKED() + * serial numbers, in any build - fail to convert. */ + if (wolfssl_asn1_integer_value(ai, &idx, &len) != 1) { err = 1; - #endif } } if (!err) { @@ -1953,6 +2155,8 @@ WOLFSSL_ASN1_INTEGER* wolfSSL_BN_to_ASN1_INTEGER(const WOLFSSL_BIGNUM *bn, /* Set length to encoded length. */ a->length = idx + len; + /* SetASNInt() wrote the header, so data is a DER INTEGER. */ + a->dataIsRaw = 0; } if (err) { @@ -1985,6 +2189,12 @@ long wolfSSL_ASN1_INTEGER_get(const WOLFSSL_ASN1_INTEGER* a) if (a == NULL) { ret = 0; } + /* An object with no value octets has no value to return. OpenSSL reports + * -1 from ASN1_INTEGER_get() here, while ASN1_INTEGER_to_BN() on the same + * object yields zero, so this is checked rather than left to to_BN(). */ + else if (a->length == 0) { + ret = WOLFSSL_FATAL_ERROR; + } if (ret > 0) { /* Create a big number from the DER encoding. */ @@ -2064,6 +2274,8 @@ int wolfSSL_ASN1_INTEGER_set(WOLFSSL_ASN1_INTEGER *a, long v) a->data[i++] = pad + j; /* Set length of DER encoding. +2 for tag and length */ a->length = 2 + pad + j; + /* Tag and length were written above. */ + a->dataIsRaw = 0; /* Add pad byte if required. */ if (pad == 1) { diff --git a/src/ssl_tsp.c b/src/ssl_tsp.c index d006fb1b889..64b24664473 100644 --- a/src/ssl_tsp.c +++ b/src/ssl_tsp.c @@ -202,17 +202,19 @@ struct WOLFSSL_TS_VERIFY_CTX { static int ts_asn1_integer_get(const WOLFSSL_ASN1_INTEGER* a, const unsigned char** data, word32* len) { - word32 idx = 1; + word32 idx = 0; int length = 0; - if ((a == NULL) || (a->length < 3) || (a->data[0] != ASN_INTEGER)) + if (a == NULL) return 0; /* The number is an unsigned big-endian value. The data holds the * magnitude; the sign is carried separately - reject a negative value * rather than use its magnitude as if positive. */ if (a->negative || (a->type == WOLFSSL_V_ASN1_NEG_INTEGER)) return 0; - if (GetLength(a->data, &idx, &length, (word32)a->length) < 0) + /* Take the value octets whichever layout the object records, instead of + * requiring a DER one. */ + if ((wolfssl_asn1_integer_value(a, &idx, &length) != 1) || (length <= 0)) return 0; *data = a->data + idx; diff --git a/src/x509.c b/src/x509.c index 62159e99aac..950607c9664 100644 --- a/src/x509.c +++ b/src/x509.c @@ -11878,6 +11878,8 @@ static WOLFSSL_X509_REVOKED* RevokedCertToRevoked(RevokedCert* rc, int seq) serial->length = rc->serialSz; serial->dataMax = rc->serialSz; serial->isDynamic = 1; + /* Value octets only - no DER tag or length. */ + serial->dataIsRaw = 1; } rev->serialNumber = serial; @@ -12048,11 +12050,14 @@ WOLFSSL_ASN1_INTEGER* wolfSSL_X509_get_serialNumber(WOLFSSL_X509* x509) #if defined(WOLFSSL_QT) || defined(WOLFSSL_HAPROXY) XMEMCPY(&a->data[i], x509->serial, x509->serialSz); a->length = x509->serialSz; + /* Qt and HAProxy read data/length as the value octets. */ + a->dataIsRaw = 1; #else a->data[i++] = ASN_INTEGER; i += SetLength(x509->serialSz, a->data + i); XMEMCPY(&a->data[i], x509->serial, x509->serialSz); a->length = x509->serialSz + 2; + a->dataIsRaw = 0; #endif x509->serialNumber = a; @@ -13101,11 +13106,23 @@ static int CertFromX509(Cert* cert, WOLFSSL_X509* x509) * this define the function will error out below */ #ifdef OPENSSL_EXTRA if (x509->serialSz == 0 && x509->serialNumber != NULL && - /* Check if the buffer contains more than just the - * ASN tag and length */ - x509->serialNumber->length > 2) { - if (wolfSSL_X509_set_serialNumber(x509, x509->serialNumber) - != WOLFSSL_SUCCESS) { + (x509->serialNumber->length > 0)) { + word32 serialIdx = 0; + int serialLen = 0; + + /* Ask the object for its value octets rather than testing + * length > 2, which assumed a DER layout. A malformed object is an + * error: skipping it would leave wc_MakeCert() to generate a random + * serial, silently signing a certificate the caller did not ask for. + * An empty object - no value to copy - still skips. */ + if (wolfssl_asn1_integer_value(x509->serialNumber, &serialIdx, + &serialLen) != 1) { + WOLFSSL_MSG("Serial number is not a valid ASN.1 INTEGER"); + return WOLFSSL_FAILURE; + } + if ((serialLen > 0) && + (wolfSSL_X509_set_serialNumber(x509, x509->serialNumber) + != WOLFSSL_SUCCESS)) { WOLFSSL_MSG("Failed to set serial number"); return WOLFSSL_FAILURE; } @@ -17563,19 +17580,24 @@ int wolfSSL_X509_set1_notBefore(WOLFSSL_X509* x509, const WOLFSSL_ASN1_TIME *t) int wolfSSL_X509_set_serialNumber(WOLFSSL_X509* x509, WOLFSSL_ASN1_INTEGER* s) { + word32 idx = 0; + int len = 0; + WOLFSSL_ENTER("wolfSSL_X509_set_serialNumber"); if (x509 == NULL || s == NULL || s->data == NULL || s->length >= EXTERNAL_SERIAL_SIZE) return WOLFSSL_FAILURE; - /* WOLFSSL_ASN1_INTEGER has type | size | data - * Sanity check that the data is actually in ASN format */ - if (s->length < 3 || s->data[0] != ASN_INTEGER || - s->data[1] != s->length - 2) { + /* Take the value octets whichever layout the object records, rather than + * requiring a DER one - X509_CRL_get_REVOKED() serials and, under + * WOLFSSL_QT/WOLFSSL_HAPROXY, X509_get_serialNumber() hand back bare + * values and used to be rejected here. */ + if ((wolfssl_asn1_integer_value(s, &idx, &len) != 1) || (len <= 0) || + (len >= EXTERNAL_SERIAL_SIZE)) { return WOLFSSL_FAILURE; } - XMEMCPY(x509->serial, s->data + 2, s->length - 2); - x509->serialSz = s->length - 2; + XMEMCPY(x509->serial, s->data + idx, (size_t)len); + x509->serialSz = len; x509->serial[x509->serialSz] = 0; return WOLFSSL_SUCCESS; diff --git a/tests/api.c b/tests/api.c index 182c5ab7996..4336612f9b8 100644 --- a/tests/api.c +++ b/tests/api.c @@ -30869,23 +30869,32 @@ static int generate_crl_test(const char* keyFile, const char* certFile, static const int numSerials = (int)(sizeof(serialsToRevokeBytes) / sizeof(serialsToRevokeBytes[0])); + /* dataIsRaw = 0: bare value octets, which is what + * wolfSSL_X509_CRL_add_revoked() reads. Every member is named, including + * the ones whose value is the default, because a designated initializer + * that omits one is an error under -Werror=missing-field-initializers in + * C++ builds - the C front end does not warn there, so a missing member + * only shows up in the C++ CI job. */ WOLFSSL_ASN1_INTEGER serialsToRevoke[3] = { { .intData = {0}, .negative = 0, .data = serialsToRevokeBytes[0], .dataMax = sizeof(serialsToRevokeBytes[0]), .isDynamic = 0, + .dataIsRaw = 0, .length = sizeof(serialsToRevokeBytes[0]), .type = 0 }, { .intData = {0}, .negative = 0, .data = serialsToRevokeBytes[1], .dataMax = sizeof(serialsToRevokeBytes[1]), .isDynamic = 0, + .dataIsRaw = 0, .length = sizeof(serialsToRevokeBytes[1]), .type = 0 }, { .intData = {0}, .negative = 0, .data = serialsToRevokeBytes[2], .dataMax = sizeof(serialsToRevokeBytes[2]), .isDynamic = 0, + .dataIsRaw = 0, .length = sizeof(serialsToRevokeBytes[2]), .type = 0 } }; @@ -31174,6 +31183,8 @@ static int test_wolfSSL_X509_CRL_sign_large(void) ExpectNotNull(revoked.serialNumber); revoked.reason = CRL_REASON_NONE; if (revoked.serialNumber != NULL) { + /* The bare value octets, with dataIsRaw left clear: the layout this + * API accepted before the flag existed. It must still work. */ revoked.serialNumber->data = serial; revoked.serialNumber->length = (int)sizeof(serial); } diff --git a/tests/api/test_evp_pkey.c b/tests/api/test_evp_pkey.c index 632af9eae4d..3d75df8dd75 100644 --- a/tests/api/test_evp_pkey.c +++ b/tests/api/test_evp_pkey.c @@ -1688,6 +1688,408 @@ int test_wolfSSL_EVP_PKEY_set1_shrinking_der(void) return EXPECT_RESULT(); } +/* Guards shared by the two tests below. The oversized parameters SEQUENCE the + * first one builds is only part of the PKCS#8 ASN.1 template when WC_RSA_PSS + * is defined, and that is what makes a header size this large reachable. */ +#if defined(OPENSSL_EXTRA) && defined(HAVE_PKCS8) && \ + defined(WOLFSSL_ASN_TEMPLATE) && defined(WC_RSA_PSS) && \ + !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && !defined(NO_ASN) && \ + !defined(NO_PWDBASED) && \ + (defined(HAVE_ECC) || (!defined(NO_RSA) && defined(WOLFSSL_KEY_TO_DER))) +#define TEST_EVP_PKEY_OVERSIZED_PKCS8 + +/* Number of bytes of tag and length that der_put_hdr() writes for len. */ +static word32 der_hdr_sz(word32 len) +{ + return (len < 0x80) ? 2 : ((len <= 0xFF) ? 3 : 4); +} + +/* Write a DER tag and definite length. Returns the number of bytes written. */ +static word32 der_put_hdr(byte* out, byte tag, word32 len) +{ + word32 i = 0; + + out[i++] = tag; + if (len < 0x80) { + out[i++] = (byte)len; + } + else if (len <= 0xFF) { + out[i++] = 0x81; + out[i++] = (byte)len; + } + else { + out[i++] = 0x82; + out[i++] = (byte)(len >> 8); + out[i++] = (byte)len; + } + return i; +} + +/* Wrap a traditional private key in a PKCS#8 PrivateKeyInfo whose + * AlgorithmIdentifier ends in a padSz byte parameters SEQUENCE. + * + * alg/algSz is the encoded content of the AlgorithmIdentifier that precedes + * the parameters: the algorithm OID, the curve OID for ECC, the NULL for RSA. + * ToTraditionalInline_ex() skips the parameters without looking inside, so + * padSz is what the decoder records as pkcs8HeaderSz. The buffer is allocated + * with XMALLOC and its length returned through outSz. + */ +static byte* der_wrap_pkcs8_padded(const byte* alg, word32 algSz, + const byte* inner, word32 innerSz, word32 padSz, word32* outSz) +{ + byte* out; + word32 paramSz = der_hdr_sz(padSz) + padSz; + word32 algSeqSz = der_hdr_sz(algSz + paramSz) + algSz + paramSz; + word32 keySz = der_hdr_sz(innerSz) + innerSz; + /* 3 bytes for the version INTEGER. */ + word32 bodySz = 3 + algSeqSz + keySz; + word32 i = 0; + + out = (byte*)XMALLOC(der_hdr_sz(bodySz) + bodySz, NULL, + DYNAMIC_TYPE_TMP_BUFFER); + if (out == NULL) { + return NULL; + } + + i += der_put_hdr(out + i, 0x30, bodySz); /* PrivateKeyInfo */ + out[i++] = 0x02; out[i++] = 0x01; out[i++] = 0x00; /* version v1 */ + i += der_put_hdr(out + i, 0x30, algSz + paramSz); /* algorithm */ + XMEMCPY(out + i, alg, algSz); + i += algSz; + i += der_put_hdr(out + i, 0x30, padSz); /* parameters */ + XMEMSET(out + i, 0, padSz); + i += padSz; + i += der_put_hdr(out + i, 0x04, innerSz); /* privateKey */ + XMEMCPY(out + i, inner, innerSz); + i += innerSz; + + *outSz = i; + return out; +} + +/* Decode der into an EVP_PKEY and hand the key object it yields to a second, + * empty EVP_PKEY. That second one, returned through fresh, is the object under + * test: taking the key over runs the producer that re-encodes it, + * ECC_populate_EVP_PKEY() or PopulateRSAEvpPkeyDer(). + * + * srcHdrSz gets the header size the decoder recorded for der, so the caller + * can confirm a crafted wrapper really did inflate it. */ +static int evp_pkey_take_over_key(int keyType, const byte* der, long derSz, + WOLFSSL_EVP_PKEY** fresh, word16* srcHdrSz) +{ + EXPECT_DECLS; + const unsigned char* in = der; + WOLFSSL_EVP_PKEY* src = NULL; + + *fresh = NULL; + *srcHdrSz = 0; + ExpectNotNull(src = wolfSSL_d2i_PrivateKey(keyType, NULL, &in, derSz)); + if (src != NULL) { + *srcHdrSz = src->pkcs8HeaderSz; + } + ExpectNotNull(*fresh = wolfSSL_EVP_PKEY_new()); +#ifdef HAVE_ECC + if (keyType == EVP_PKEY_EC) { + WOLFSSL_EC_KEY* ec = NULL; + + ExpectNotNull(ec = wolfSSL_EVP_PKEY_get1_EC_KEY(src)); + ExpectIntEQ(wolfSSL_EVP_PKEY_set1_EC_KEY(*fresh, ec), + WOLFSSL_SUCCESS); + wolfSSL_EC_KEY_free(ec); + } +#endif +#if !defined(NO_RSA) && defined(WOLFSSL_KEY_TO_DER) + if (keyType == EVP_PKEY_RSA) { + WOLFSSL_RSA* rsa = NULL; + + ExpectNotNull(rsa = wolfSSL_EVP_PKEY_get1_RSA(src)); + ExpectIntEQ(wolfSSL_EVP_PKEY_set1_RSA(*fresh, rsa), WOLFSSL_SUCCESS); + wolfSSL_RSA_free(rsa); + } +#endif + wolfSSL_EVP_PKEY_free(src); + return EXPECT_RESULT(); +} + +/* bad came from the crafted wrapper, ref from the well-formed one. Both hold + * the same key, so the producer has to have left them identical. */ +static int evp_pkey_match_reencoded(WOLFSSL_EVP_PKEY* bad, + WOLFSSL_EVP_PKEY* ref) +{ + EXPECT_DECLS; + unsigned char* badDer = NULL; + unsigned char* refDer = NULL; + int badSz = 0; + int refSz = 0; + + /* The invariant the producers have to restore: the header size lies + * inside the encoding it describes. Asserted before anything exports the + * key, so a regression fails here instead of reading off the end of the + * buffer. */ + ExpectIntGT(bad->pkey_sz, 0); + ExpectIntLT((int)bad->pkcs8HeaderSz, bad->pkey_sz); + /* Not just inside it: the crafted wrapper must leave exactly what the + * well-formed one does. */ + ExpectIntEQ(bad->pkey_sz, ref->pkey_sz); + ExpectIntEQ((int)bad->pkcs8HeaderSz, (int)ref->pkcs8HeaderSz); + + ExpectIntGT(refSz = wolfSSL_i2d_PrivateKey(ref, &refDer), 0); + ExpectIntGT(badSz = wolfSSL_i2d_PrivateKey(bad, &badDer), 0); + ExpectIntEQ(badSz, refSz); + ExpectNotNull(badDer); + ExpectNotNull(refDer); + ExpectBufEQ(badDer, refDer, (size_t)refSz); + XFREE(badDer, NULL, DYNAMIC_TYPE_OPENSSL); + XFREE(refDer, NULL, DYNAMIC_TYPE_OPENSSL); + badDer = NULL; + refDer = NULL; + +/* pkcs8_encode()/pkcs8_encrypt() and the PKCS#8 PEM writer that reaches them + * are built only for OPENSSL_ALL. */ +#if defined(OPENSSL_ALL) && !defined(NO_BIO) + { + WOLFSSL_BIO* badBio = NULL; + WOLFSSL_BIO* refBio = NULL; + char* badPem = NULL; + char* refPem = NULL; + + /* wolfSSL_PEM_write_bio_PKCS8PrivateKey() is the sink: pkcs8_encode() + * hands wc_CreatePKCS8Key() pkey.ptr + pkcs8HeaderSz and + * pkey_sz - pkcs8HeaderSz. */ + ExpectNotNull(badBio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem())); + ExpectNotNull(refBio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem())); + ExpectIntGT(wolfSSL_PEM_write_bio_PKCS8PrivateKey(refBio, ref, NULL, + NULL, 0, NULL, NULL), 0); + ExpectIntGT(wolfSSL_PEM_write_bio_PKCS8PrivateKey(badBio, bad, NULL, + NULL, 0, NULL, NULL), 0); + ExpectIntGT(refSz = wolfSSL_BIO_get_mem_data(refBio, &refPem), 0); + ExpectIntEQ(wolfSSL_BIO_get_mem_data(badBio, &badPem), refSz); + ExpectNotNull(refPem); + ExpectNotNull(badPem); + ExpectBufEQ(badPem, refPem, (size_t)refSz); + wolfSSL_BIO_free(badBio); + wolfSSL_BIO_free(refBio); + } + +#if !defined(NO_AES) && defined(HAVE_AES_CBC) && defined(WOLFSSL_AES_256) \ + && defined(HAVE_PBKDF2) && !defined(NO_HMAC) + { + WOLFSSL_BIO* bio = NULL; + char pkcs8Pass[] = "wolfssl-pkcs8-password"; + + /* The encrypting twin reaches pkcs8_encrypt(), which slices the same + * buffer at the same offset. The output carries a random salt and IV, + * so only the call itself can be checked. + * + * PBES2 derives the key with PBKDF2, which uses the password as an + * HMAC key, and a FIPS build rejects an HMAC key shorter than + * HMAC_FIPS_MIN_KEY (14 bytes) with HMAC_MIN_KEYLEN_E. Keep the + * password above that or the call fails for the wrong reason. + * + * Guarded on HAVE_PBKDF2 && !NO_HMAC to match the PKCS5v2 case of + * wc_CryptKey() in wc_encrypt.c: PBES2 is the only version that can + * carry AES-256-CBC, so without it this call cannot succeed. */ + ExpectNotNull(bio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem())); + ExpectIntGT(wolfSSL_PEM_write_bio_PKCS8PrivateKey(bio, bad, + wolfSSL_EVP_aes_256_cbc(), pkcs8Pass, (int)XSTRLEN(pkcs8Pass), + NULL, NULL), 0); + wolfSSL_BIO_free(bio); + } +#endif +#endif /* OPENSSL_ALL && !NO_BIO */ + + return EXPECT_RESULT(); +} +#endif /* TEST_EVP_PKEY_OVERSIZED_PKCS8 */ + +/* + * A PKCS#8 AlgorithmIdentifier may carry a parameters SEQUENCE that + * ToTraditionalInline_ex() skips without inspecting, so the inner-key offset + * it reports - what wolfSSL_EC_KEY_LoadDer_ex() and wolfSSL_RSA_LoadDer_ex() + * store as pkcs8HeaderSz - is attacker controlled and effectively unbounded. + * + * ECC_populate_EVP_PKEY() and PopulateRSAEvpPkeyDer() build a FRESH PKCS#8 + * blob for the EVP_PKEY, which never carries those parameters, but copied + * pkcs8HeaderSz straight off the EC_KEY/RSA object. The header then described + * a buffer that is gone: every export computes pkey.ptr + pkcs8HeaderSz and + * pkey_sz - pkcs8HeaderSz, so it read past the end of the new blob and handed + * wc_CreatePKCS8Key() a length of nearly 4 GiB. + */ +int test_wolfSSL_EVP_PKEY_set1_oversized_pkcs8_header(void) +{ + EXPECT_DECLS; +#ifdef TEST_EVP_PKEY_OVERSIZED_PKCS8 + /* Large enough that the header cannot fall inside the re-encoded blob for + * either algorithm, and larger than any of the keys involved. */ + const word32 padSz = 2000; + byte* inner = NULL; + size_t innerSz = 0; + byte* wrapped = NULL; + word32 wrappedSz = 0; + byte* p8 = NULL; + size_t p8Sz = 0; + word16 srcHdrSz = 0; + WOLFSSL_EVP_PKEY* bad = NULL; + WOLFSSL_EVP_PKEY* ref = NULL; + +#ifdef HAVE_ECC + { + static const byte ecAlg[] = { + /* id-ecPublicKey */ + 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, + /* prime256v1 */ + 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07 + }; + + /* ecc-key.der and ecc-keyPkcs8.der hold the same key, one bare and + * one wrapped. */ + ExpectIntEQ(load_file("./certs/ecc-key.der", &inner, &innerSz), 0); + ExpectNotNull(wrapped = der_wrap_pkcs8_padded(ecAlg, (word32)sizeof( + ecAlg), inner, (word32)innerSz, padSz, &wrappedSz)); + ExpectIntEQ(load_file("./certs/ecc-keyPkcs8.der", &p8, &p8Sz), 0); + ExpectIntEQ(evp_pkey_take_over_key(EVP_PKEY_EC, p8, (long)p8Sz, &ref, + &srcHdrSz), TEST_SUCCESS); + ExpectIntEQ(evp_pkey_take_over_key(EVP_PKEY_EC, wrapped, + (long)wrappedSz, &bad, &srcHdrSz), TEST_SUCCESS); + /* The crafted wrapper is accepted and the decoder did record the + * inflated header, so the check below is not vacuous. */ + ExpectIntGE((int)srcHdrSz, (int)padSz); + ExpectIntEQ(evp_pkey_match_reencoded(bad, ref), TEST_SUCCESS); + + wolfSSL_EVP_PKEY_free(bad); + wolfSSL_EVP_PKEY_free(ref); + bad = NULL; + ref = NULL; + XFREE(wrapped, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(inner, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(p8, NULL, DYNAMIC_TYPE_TMP_BUFFER); + wrapped = NULL; + inner = NULL; + p8 = NULL; + } +#endif /* HAVE_ECC */ + +#if !defined(NO_RSA) && defined(WOLFSSL_KEY_TO_DER) + { + static const byte rsaAlg[] = { + /* rsaEncryption */ + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01, + /* parameters NULL, required for rsaEncryption */ + 0x05, 0x00 + }; + + /* As above: server-key.der and server-keyPkcs8.der are one key. */ + ExpectIntEQ(load_file("./certs/server-key.der", &inner, &innerSz), 0); + ExpectNotNull(wrapped = der_wrap_pkcs8_padded(rsaAlg, (word32)sizeof( + rsaAlg), inner, (word32)innerSz, padSz, &wrappedSz)); + ExpectIntEQ(load_file("./certs/server-keyPkcs8.der", &p8, &p8Sz), 0); + ExpectIntEQ(evp_pkey_take_over_key(EVP_PKEY_RSA, p8, (long)p8Sz, &ref, + &srcHdrSz), TEST_SUCCESS); + ExpectIntEQ(evp_pkey_take_over_key(EVP_PKEY_RSA, wrapped, + (long)wrappedSz, &bad, &srcHdrSz), TEST_SUCCESS); + /* As above: the header really is the crafted one. */ + ExpectIntGE((int)srcHdrSz, (int)padSz); + ExpectIntEQ(evp_pkey_match_reencoded(bad, ref), TEST_SUCCESS); + + wolfSSL_EVP_PKEY_free(bad); + wolfSSL_EVP_PKEY_free(ref); + XFREE(wrapped, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(inner, NULL, DYNAMIC_TYPE_TMP_BUFFER); + XFREE(p8, NULL, DYNAMIC_TYPE_TMP_BUFFER); + } +#endif /* !NO_RSA && WOLFSSL_KEY_TO_DER */ +#endif /* TEST_EVP_PKEY_OVERSIZED_PKCS8 */ + return EXPECT_RESULT(); +} + +/* + * pkcs8_encode() and pkcs8_encrypt() slice the cached DER at pkcs8HeaderSz and + * pass the remainder, pkey_sz - pkcs8HeaderSz, on as a word32. A header size + * that does not lie inside the encoding makes that subtraction underflow, so + * they reject it rather than trusting whoever populated the pkey - the same + * bound wolfssl_i_evp_pkey_get_der() already applies. + * + * The producers keep the two in step (see + * test_wolfSSL_EVP_PKEY_set1_oversized_pkcs8_header()), so the only way to + * reach the guard is to put the pkey in that state directly. + */ +int test_wolfSSL_EVP_PKEY_pkcs8_header_bounds(void) +{ + EXPECT_DECLS; +/* pkcs8_encode(), pkcs8_encrypt(), wolfSSL_PEM_write_bio_PKCS8PrivateKey() and + * wolfSSL_i2d_PKCS8_PKEY() are all built only for OPENSSL_ALL. */ +#if defined(OPENSSL_ALL) && defined(HAVE_PKCS8) && defined(HAVE_ECC) && \ + !defined(NO_BIO) && !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && \ + !defined(NO_ASN) && !defined(NO_PWDBASED) + const unsigned char* in; + byte* buf = NULL; + size_t bufSz = 0; + word16 hdrSz = 0; +#if !defined(NO_AES) && defined(HAVE_AES_CBC) && defined(WOLFSSL_AES_256) \ + && defined(HAVE_PBKDF2) && !defined(NO_HMAC) + char pkcs8Pass[] = "wolfssl-pkcs8-password"; +#endif + WOLFSSL_EVP_PKEY* pkey = NULL; + WOLFSSL_BIO* bio = NULL; + + ExpectIntEQ(load_file("./certs/ecc-keyPkcs8.der", &buf, &bufSz), 0); + in = buf; + ExpectNotNull(pkey = wolfSSL_d2i_PrivateKey(EVP_PKEY_EC, NULL, &in, + (long)bufSz)); + ExpectIntGT(pkey->pkey_sz, 0); + + /* Baseline: the key exports while the header is the decoded one. */ + ExpectNotNull(bio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem())); + ExpectIntGT(wolfSSL_PEM_write_bio_PKCS8PrivateKey(bio, pkey, NULL, NULL, 0, + NULL, NULL), 0); + wolfSSL_BIO_free(bio); + bio = NULL; + + if (pkey != NULL) { + hdrSz = pkey->pkcs8HeaderSz; + /* The boundary: the header ends exactly where the encoding does, so + * there is no key left to wrap. Checked first because it is the one + * out-of-range value that stays inside the buffer - an unguarded + * build fails here rather than reading off the end of it. */ + pkey->pkcs8HeaderSz = (word16)pkey->pkey_sz; + } + ExpectNotNull(bio = wolfSSL_BIO_new(wolfSSL_BIO_s_mem())); + ExpectIntEQ(wolfSSL_PEM_write_bio_PKCS8PrivateKey(bio, pkey, NULL, NULL, 0, + NULL, NULL), 0); +#if !defined(NO_AES) && defined(HAVE_AES_CBC) && defined(WOLFSSL_AES_256) \ + && defined(HAVE_PBKDF2) && !defined(NO_HMAC) + /* At least HMAC_FIPS_MIN_KEY (14) bytes, or a FIPS build rejects the + * PBKDF2 password before the header bound is ever reached. The guard + * above keeps this out of builds with no PBES2 at all, where the call + * would return 0 without the header bound deciding anything. */ + ExpectIntEQ(wolfSSL_PEM_write_bio_PKCS8PrivateKey(bio, pkey, + wolfSSL_EVP_aes_256_cbc(), pkcs8Pass, (int)XSTRLEN(pkcs8Pass), NULL, + NULL), 0); +#endif + ExpectIntLT(wolfSSL_i2d_PKCS8_PKEY(pkey, NULL), 0); + + /* Past the end, where pkey_sz - pkcs8HeaderSz underflows to nearly 4 GiB. + * The Expect macros stop evaluating after a failure, so these calls are + * only made once the boundary above has been rejected. */ + if (pkey != NULL) { + pkey->pkcs8HeaderSz = (word16)(pkey->pkey_sz + 1); + } + ExpectIntEQ(wolfSSL_PEM_write_bio_PKCS8PrivateKey(bio, pkey, NULL, NULL, 0, + NULL, NULL), 0); + ExpectIntLT(wolfSSL_i2d_PKCS8_PKEY(pkey, NULL), 0); + wolfSSL_BIO_free(bio); + bio = NULL; + + /* Put it back so the key is freed in a consistent state. */ + if (pkey != NULL) { + pkey->pkcs8HeaderSz = hdrSz; + } + wolfSSL_EVP_PKEY_free(pkey); + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); +#endif + return EXPECT_RESULT(); +} + /* * wolfSSL_EVP_PKEY_get1_EC_KEY() hands the caller a reference of its own, so * releasing it has to leave the copy the EVP_PKEY holds intact and usable. @@ -2036,6 +2438,177 @@ int test_wolfSSL_EVP_PKEY_sign_verify_rsa(void) #endif return EXPECT_RESULT(); } +#if defined(OPENSSL_EXTRA) && !defined (NO_DSA) && \ + !defined(WC_FIPS_186_5_PLUS) && !defined(HAVE_SELFTEST) && \ + defined(WOLFSSL_KEY_GEN) +/* + * The EVP layer is the only place that knows the caller's tbslen and siglen, + * so it has to bound them before handing the pointers to + * wolfssl_dsa_do_sign()/wolfssl_dsa_do_verify(). + * + * Those helpers take the digest length, so a digest shorter than the hash + * wolfCrypt would otherwise assume is signed as given rather than over-read. + * What still has to be rejected is a digest longer than WC_MAX_DIGEST_SIZE + * (the length goes down as an int), a digest shorter than the + * WC_SHA_DIGEST_SIZE that verify reads, and a signature buffer shorter than + * the 2 * |q| bytes verify reads. + * + * The buffers below are allocated at exactly the length handed to the API, so + * a missing bound shows up as a heap over-read under a sanitizer as well as in + * the return value. + */ +static int test_evp_pkey_dsa_bad_lengths(void) +{ + EXPECT_DECLS; + WOLFSSL_DSA* dsa = NULL; + WOLFSSL_EVP_PKEY* pkey = NULL; + WOLFSSL_EVP_PKEY_CTX* ctx = NULL; + /* WC_MAX_DIGEST_SIZE, not WC_SHA_DIGEST_SIZE: this is the baseline digest + * that the success cases sign and verify, so its length has to be accepted + * in every build this test runs in. wc_DsaSign_ex() rejects anything below + * WC_MIN_DIGEST_SIZE_FOR_SIGN, which a build may raise above 20, and + * WC_MAX_DIGEST_SIZE is by definition at or above it. DSA truncates the + * digest to |q| identically on sign and verify, so a longer one still + * round-trips. The rejection cases below allocate their own buffers at + * exactly the short length they pass. */ + byte digest[WC_MAX_DIGEST_SIZE]; + byte* sig = NULL; + byte* tmp = NULL; + size_t siglen = 0; + + XMEMSET(digest, 0x5a, sizeof(digest)); + + ExpectNotNull(dsa = DSA_new()); + ExpectIntEQ(DSA_generate_parameters_ex(dsa, 2048, NULL, 0, NULL, NULL, + NULL), 1); + ExpectIntEQ(DSA_generate_key(dsa), 1); + ExpectNotNull(pkey = EVP_PKEY_new()); + ExpectIntEQ(EVP_PKEY_set1_DSA(pkey, dsa), WOLFSSL_SUCCESS); + + /* The length query reports 2 * |q|. */ + ExpectNotNull(ctx = EVP_PKEY_CTX_new(pkey, NULL)); + ExpectIntEQ(EVP_PKEY_sign_init(ctx), WOLFSSL_SUCCESS); + ExpectIntEQ(EVP_PKEY_sign(ctx, NULL, &siglen, digest, sizeof(digest)), + WOLFSSL_SUCCESS); + ExpectIntGT((int)siglen, 0); + ExpectNotNull(sig = (byte*)XMALLOC(siglen, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + + /* No digest at all is an error from wolfssl_dsa_do_sign(), which the EVP + * layer passes straight back, so this is negative rather than zero. */ + ExpectIntLT(EVP_PKEY_sign(ctx, sig, &siglen, NULL, sizeof(digest)), 0); + ExpectIntLT(EVP_PKEY_sign(ctx, sig, &siglen, digest, 0), 0); + + /* Longer than the length can carry down as an int. */ + ExpectNotNull(tmp = (byte*)XMALLOC(WC_MAX_DIGEST_SIZE + 1, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + if (tmp != NULL) { + XMEMSET(tmp, 0x5a, WC_MAX_DIGEST_SIZE + 1); + } + ExpectIntEQ(EVP_PKEY_sign(ctx, sig, &siglen, tmp, + WC_MAX_DIGEST_SIZE + 1), WOLFSSL_FAILURE); + XFREE(tmp, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + tmp = NULL; + + /* A digest shorter than WC_SHA_DIGEST_SIZE is signed as given: the length + * is forwarded, so there is nothing to over-read. Whether it is accepted + * depends on the build - wc_DsaSign_ex() rejects anything below the + * smallest enabled hash, which is 20 when SHA-1 is the smallest and in + * FIPS 186-4 builds. Either way the point is that the length decides, not + * a fixed read of WC_SHA_DIGEST_SIZE bytes. A build can also raise the + * minimum past 20 with WC_MIN_DIGEST_SIZE_FOR_SIGN; nothing else can, + * since settings.h requires SHA-1 whenever DSA is enabled. The comparison + * is made at run time because old FIPS headers define the minimum as an + * enum, which the preprocessor cannot compare. */ + ExpectNotNull(tmp = (byte*)XMALLOC(WC_SHA_DIGEST_SIZE - 1, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + if (tmp != NULL) { + XMEMCPY(tmp, digest, WC_SHA_DIGEST_SIZE - 1); + } + ExpectIntEQ(EVP_PKEY_sign(ctx, sig, &siglen, tmp, WC_SHA_DIGEST_SIZE - 1), + ((WC_SHA_DIGEST_SIZE - 1) >= (int)WC_MIN_DIGEST_SIZE_FOR_SIGN) ? + (int)WOLFSSL_SUCCESS : (int)WOLFSSL_FATAL_ERROR); + XFREE(tmp, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + tmp = NULL; + + /* Sign the full digest for the verify cases below. */ + ExpectIntEQ(EVP_PKEY_sign(ctx, sig, &siglen, digest, sizeof(digest)), + WOLFSSL_SUCCESS); + EVP_PKEY_CTX_free(ctx); + ctx = NULL; + + ExpectNotNull(ctx = EVP_PKEY_CTX_new(pkey, NULL)); + ExpectIntEQ(EVP_PKEY_verify_init(ctx), WOLFSSL_SUCCESS); + ExpectIntEQ(EVP_PKEY_verify(ctx, sig, siglen, digest, sizeof(digest)), + WOLFSSL_SUCCESS); + + /* Absent signature or digest. */ + ExpectIntEQ(EVP_PKEY_verify(ctx, NULL, siglen, digest, sizeof(digest)), + WOLFSSL_FAILURE); + ExpectIntEQ(EVP_PKEY_verify(ctx, sig, siglen, NULL, sizeof(digest)), + WOLFSSL_FAILURE); + + /* Verify reads WC_SHA_DIGEST_SIZE bytes, so a shorter digest is rejected + * rather than read past - in a buffer holding only what is claimed. */ + ExpectNotNull(tmp = (byte*)XMALLOC(WC_SHA_DIGEST_SIZE - 1, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + if (tmp != NULL) { + XMEMCPY(tmp, digest, WC_SHA_DIGEST_SIZE - 1); + } + ExpectIntEQ(EVP_PKEY_verify(ctx, sig, siglen, tmp, + WC_SHA_DIGEST_SIZE - 1), WOLFSSL_FAILURE); + XFREE(tmp, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + tmp = NULL; + + /* And a digest longer than the length can carry down as an int. */ + ExpectNotNull(tmp = (byte*)XMALLOC(WC_MAX_DIGEST_SIZE + 1, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + if (tmp != NULL) { + XMEMSET(tmp, 0x5a, WC_MAX_DIGEST_SIZE + 1); + } + ExpectIntEQ(EVP_PKEY_verify(ctx, sig, siglen, tmp, + WC_MAX_DIGEST_SIZE + 1), WOLFSSL_FAILURE); + XFREE(tmp, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + tmp = NULL; + + /* A signature buffer shorter than the 2 * |q| bytes read, again holding + * only what is claimed. */ + ExpectNotNull(tmp = (byte*)XMALLOC(siglen - 1, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + if (tmp != NULL) { + XMEMCPY(tmp, sig, siglen - 1); + } + ExpectIntEQ(EVP_PKEY_verify(ctx, tmp, siglen - 1, digest, sizeof(digest)), + WOLFSSL_FAILURE); + XFREE(tmp, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + tmp = NULL; + + ExpectIntEQ(EVP_PKEY_verify(ctx, sig, 0, digest, sizeof(digest)), + WOLFSSL_FAILURE); + + /* 2 * |q| is a minimum, not an exact length: trailing bytes beyond the + * signature are ignored. OpenSSL rejects them, because its DSA signatures + * are DER and the decode fails on trailing data. */ + ExpectNotNull(tmp = (byte*)XMALLOC(siglen + 1, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER)); + if (tmp != NULL) { + XMEMCPY(tmp, sig, siglen); + tmp[siglen] = 0x00; + } + ExpectIntEQ(EVP_PKEY_verify(ctx, tmp, siglen + 1, digest, sizeof(digest)), + WOLFSSL_SUCCESS); + XFREE(tmp, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + tmp = NULL; + + XFREE(sig, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + EVP_PKEY_CTX_free(ctx); + EVP_PKEY_free(pkey); + DSA_free(dsa); + return EXPECT_RESULT(); +} +#endif /* OPENSSL_EXTRA && !NO_DSA && !WC_FIPS_186_5_PLUS && !HAVE_SELFTEST && + * WOLFSSL_KEY_GEN */ + int test_wolfSSL_EVP_PKEY_sign_verify_dsa(void) { EXPECT_DECLS; @@ -2043,6 +2616,7 @@ int test_wolfSSL_EVP_PKEY_sign_verify_dsa(void) #if !defined (NO_DSA) && !defined(WC_FIPS_186_5_PLUS) && \ !defined(HAVE_SELFTEST) && defined(WOLFSSL_KEY_GEN) ExpectIntEQ(test_wolfSSL_EVP_PKEY_sign_verify(EVP_PKEY_DSA), TEST_SUCCESS); + ExpectIntEQ(test_evp_pkey_dsa_bad_lengths(), TEST_SUCCESS); #endif #endif return EXPECT_RESULT(); diff --git a/tests/api/test_evp_pkey.h b/tests/api/test_evp_pkey.h index 874391edb21..d24faed75d8 100644 --- a/tests/api/test_evp_pkey.h +++ b/tests/api/test_evp_pkey.h @@ -54,6 +54,8 @@ int test_wolfSSL_EVP_PKEY_keygen(void); int test_wolfSSL_EVP_PKEY_keygen_reuse(void); int test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8(void); int test_wolfSSL_EVP_PKEY_set1_shrinking_der(void); +int test_wolfSSL_EVP_PKEY_set1_oversized_pkcs8_header(void); +int test_wolfSSL_EVP_PKEY_pkcs8_header_bounds(void); int test_wolfSSL_EVP_PKEY_get1_EC_KEY_reuse(void); int test_wolfSSL_EVP_PKEY_keygen_dh_reuse(void); int test_wolfSSL_EVP_SignInit_ex(void); @@ -115,6 +117,9 @@ int test_wolfSSL_CTX_use_PrivateKey_pkcs8_repopulate(void); TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_set1_EC_KEY_no_pkcs8), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_set1_shrinking_der), \ + TEST_DECL_GROUP("evp_pkey", \ + test_wolfSSL_EVP_PKEY_set1_oversized_pkcs8_header), \ + TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_pkcs8_header_bounds), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_get1_EC_KEY_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_PKEY_keygen_dh_reuse), \ TEST_DECL_GROUP("evp_pkey", test_wolfSSL_EVP_SignInit_ex), \ diff --git a/tests/api/test_ossl_asn1.c b/tests/api/test_ossl_asn1.c index c646141ed7d..0aa81370a8f 100644 --- a/tests/api/test_ossl_asn1.c +++ b/tests/api/test_ossl_asn1.c @@ -134,6 +134,9 @@ int test_wolfSSL_ASN1_INTEGER(void) ExpectNotNull(a = d2i_ASN1_INTEGER(NULL, &p, sizeof(longDer))); ExpectPtrNE(p, longDer); ExpectNotNull(dup = wolfSSL_ASN1_INTEGER_dup(a)); + /* The copy of a heap-backed value must hold the value, not be empty. */ + ExpectIntEQ(wolfSSL_ASN1_INTEGER_get_length(dup), 32); + ExpectIntEQ(ASN1_INTEGER_cmp(a, dup), 0); ASN1_INTEGER_free(dup); ASN1_INTEGER_free(a); #endif @@ -169,6 +172,83 @@ int test_wolfSSL_ASN1_INTEGER_cmp(void) ExpectIntLT(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); ExpectIntGT(wolfSSL_ASN1_INTEGER_cmp(b, a), 0); + /* The same number in the two layouts compares equal. A DER value carries + * a 0x00 pad when its top bit is set, so 128 is 00 80 as a DER value but + * 80 as bare value octets - as a CRL revoked serial holds it. */ + ExpectIntEQ(ASN1_INTEGER_set(a, 128), 1); + if (b != NULL) { + b->data[0] = 0x80; + b->length = 1; + b->dataIsRaw = 1; + b->negative = 0; + } + ExpectIntEQ(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + ExpectIntEQ(wolfSSL_ASN1_INTEGER_cmp(b, a), 0); + /* Ordering still holds across the layouts: 127 < 128. */ + ExpectIntEQ(ASN1_INTEGER_set(a, 127), 1); + ExpectIntLT(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + ExpectIntGT(wolfSSL_ASN1_INTEGER_cmp(b, a), 0); + /* Zero is 00 as a DER value and empty as a bare one. */ + ExpectIntEQ(ASN1_INTEGER_set(a, 0), 1); + if (b != NULL) { + b->length = 0; + } + ExpectIntEQ(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + + /* A basic constraints pathLenConstraint, as X509_get_ext() and + * X509_get_ext_d2i() build it: the path length goes in ->length with + * ->data left zeroed, so the object holds no value in either layout. + * Comparing one with itself must still say equal, and ->length must still + * order them. */ + if ((a != NULL) && (b != NULL)) { + XMEMSET(a->data, 0, (size_t)a->dataMax); + XMEMSET(b->data, 0, (size_t)b->dataMax); + a->dataIsRaw = 0; + b->dataIsRaw = 0; + a->negative = 0; + b->negative = 0; + a->length = 3; + b->length = 3; + } + ExpectIntEQ(wolfSSL_ASN1_INTEGER_cmp(a, a), 0); + ExpectIntEQ(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + if (b != NULL) { + b->length = 5; + } + ExpectIntLT(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + ExpectIntGT(wolfSSL_ASN1_INTEGER_cmp(b, a), 0); + + /* It must also order correctly against an object that does hold an + * encoded value, by value and not by storage length: 1, 3 and 127 all + * encode in three octets of DER, so a comparison of lengths would call a + * path length of 3 equal to each of them. a->length is 3 here. */ + if (b != NULL) { + ExpectIntEQ(ASN1_INTEGER_set(b, 1), 1); + } + ExpectIntGT(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + ExpectIntLT(wolfSSL_ASN1_INTEGER_cmp(b, a), 0); + if (b != NULL) { + ExpectIntEQ(ASN1_INTEGER_set(b, 127), 1); + } + ExpectIntLT(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + ExpectIntGT(wolfSSL_ASN1_INTEGER_cmp(b, a), 0); + if (b != NULL) { + ExpectIntEQ(ASN1_INTEGER_set(b, 3), 1); + } + ExpectIntEQ(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + ExpectIntEQ(wolfSSL_ASN1_INTEGER_cmp(b, a), 0); + + /* A value too wide to decode into a word is greater than any path length + * whatever its own byte count. */ + if ((b != NULL) && (b->dataMax >= 8)) { + XMEMSET(b->data, 0xff, 8); + b->dataIsRaw = 1; + b->negative = 0; + b->length = 8; + ExpectIntLT(wolfSSL_ASN1_INTEGER_cmp(a, b), 0); + ExpectIntGT(wolfSSL_ASN1_INTEGER_cmp(b, a), 0); + } + ASN1_INTEGER_free(b); ASN1_INTEGER_free(a); #endif @@ -193,14 +273,13 @@ int test_wolfSSL_ASN1_INTEGER_BN(void) /* at the moment hard setting since no set function */ if (ai != NULL) { - ai->data[0] = 0xff; /* No DER encoding. */ + ai->data[0] = 0xff; /* The value, not a DER encoding. */ ai->length = 1; + ai->dataIsRaw = 1; } -#if defined(WOLFSSL_QT) || defined(WOLFSSL_HAPROXY) + /* An object that says its data is the value is read that way in every + * build. This used to depend on WOLFSSL_QT/WOLFSSL_HAPROXY. */ ExpectNotNull(bn = ASN1_INTEGER_to_BN(ai, NULL)); -#else - ExpectNull(bn = ASN1_INTEGER_to_BN(ai, NULL)); -#endif BN_free(bn); bn = NULL; @@ -209,13 +288,11 @@ int test_wolfSSL_ASN1_INTEGER_BN(void) ai->data[1] = 0x04; /* bad length of integer */ ai->data[2] = 0x03; ai->length = 3; + ai->dataIsRaw = 0; /* claims a DER layout */ } -#if defined(WOLFSSL_QT) || defined(WOLFSSL_HAPROXY) - /* Interpreted as a number 0x020403. */ - ExpectNotNull(bn = ASN1_INTEGER_to_BN(ai, NULL)); -#else + /* The object claims DER and the header does not span its length, so it is + * rejected rather than read as a value - in every build. */ ExpectNull(bn = ASN1_INTEGER_to_BN(ai, NULL)); -#endif BN_free(bn); bn = NULL; @@ -287,11 +364,8 @@ int test_wolfSSL_ASN1_INTEGER_get_set(void) ExpectNotNull(a = ASN1_INTEGER_new()); /* Invalid parameter testing. */ ExpectIntEQ(ASN1_INTEGER_get(NULL), 0); -#if defined(WOLFSSL_QT) || defined(WOLFSSL_HAPROXY) - ExpectIntEQ(ASN1_INTEGER_get(a), 0); -#else + /* No value octets: -1 in every build, as OpenSSL reports. */ ExpectIntEQ(ASN1_INTEGER_get(a), -1); -#endif ASN1_INTEGER_free(a); a = NULL; @@ -651,6 +725,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[0] = ASN_INTEGER; a->intData[1] = 1; a->intData[2] = 40; + a->length = 3; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 1); ExpectNotNull(pp = (unsigned char*)XMALLOC(ret + 1, NULL, @@ -670,6 +745,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[0] = ASN_INTEGER; a->intData[1] = 1; a->intData[2] = 128; + a->length = 3; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 2); ExpectNotNull(pp = (unsigned char*)XMALLOC(ret + 1, NULL, @@ -690,6 +766,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[0] = ASN_INTEGER; a->intData[1] = 1; a->intData[2] = 40; + a->length = 3; a->negative = 1; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 1); @@ -710,6 +787,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[0] = ASN_INTEGER; a->intData[1] = 1; a->intData[2] = 128; + a->length = 3; a->negative = 1; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 1); @@ -730,6 +808,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[0] = ASN_INTEGER; a->intData[1] = 1; a->intData[2] = 200; + a->length = 3; a->negative = 1; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 2); @@ -750,6 +829,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) if (a != NULL) { a->intData[0] = ASN_INTEGER; a->intData[1] = 0; + a->length = 2; a->negative = 0; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 1); @@ -770,6 +850,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[0] = ASN_INTEGER; a->intData[1] = 1; a->intData[2] = 0; + a->length = 3; a->negative = 1; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 1); @@ -791,6 +872,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[1] = 2; a->intData[2] = 0x01; a->intData[3] = 0x00; + a->length = 4; a->negative = 0; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 2); @@ -813,6 +895,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[1] = 2; a->intData[2] = 0x80; a->intData[3] = 0x00; + a->length = 4; a->negative = 1; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 2); @@ -835,6 +918,7 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) a->intData[1] = 2; a->intData[2] = 0x80; a->intData[3] = 0x01; + a->length = 4; a->negative = 1; } ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 3); @@ -850,6 +934,85 @@ int test_wolfSSL_i2c_ASN1_INTEGER(void) ExpectIntEQ(tpp[2], 0xFF); } XFREE(pp, NULL, DYNAMIC_TYPE_TMP_BUFFER); + pp = NULL; + + /* Value octets without a DER header, as X509_CRL_get_REVOKED() produces. + * The object has to say so - dataIsRaw is what stops the second byte being + * read as a DER length. */ + if (a != NULL) { + a->dataIsRaw = 1; + a->intData[0] = 0x01; + a->intData[1] = 0x02; + a->intData[2] = 0x03; + a->length = 3; + a->negative = 0; + } + ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 3); + ExpectNotNull(pp = (unsigned char*)XMALLOC(ret + 1, NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + tpp = pp; + if (tpp != NULL) { + ExpectNotNull(XMEMSET(tpp, 0, ret + 1)); + ExpectIntEQ(i2c_ASN1_INTEGER(a, &tpp), 3); + tpp -= 3; + ExpectIntEQ(tpp[0], 0x01); + ExpectIntEQ(tpp[1], 0x02); + ExpectIntEQ(tpp[2], 0x03); + } + XFREE(pp, NULL, DYNAMIC_TYPE_TMP_BUFFER); + pp = NULL; + + /* Value octets that look like a DER header claiming more than a->length. + * Still the value, because the object says so. */ + if (a != NULL) { + a->dataIsRaw = 1; + a->intData[0] = ASN_INTEGER; + a->intData[1] = 0x82; + a->intData[2] = 0x40; + a->intData[3] = 0x00; + a->length = 4; + a->negative = 0; + } + ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 4); + ExpectNotNull(pp = (unsigned char*)XMALLOC(ret + 1, NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + tpp = pp; + if (tpp != NULL) { + ExpectNotNull(XMEMSET(tpp, 0, ret + 1)); + ExpectIntEQ(i2c_ASN1_INTEGER(a, &tpp), 4); + tpp -= 4; + ExpectIntEQ(tpp[0], ASN_INTEGER); + ExpectIntEQ(tpp[1], 0x82); + ExpectIntEQ(tpp[2], 0x40); + ExpectIntEQ(tpp[3], 0x00); + } + XFREE(pp, NULL, DYNAMIC_TYPE_TMP_BUFFER); + pp = NULL; + + /* A length that does not lie inside the buffer holding the data. No + * producer in the tree creates this, but the object is caller visible, so + * the length must not be trusted to bound the read. */ + if (a != NULL) { + a->dataIsRaw = 1; + a->length = (int)a->dataMax + 1; + a->negative = 0; + } + ExpectIntEQ(i2c_ASN1_INTEGER(a, NULL), 0); + /* A length of zero still encodes as the single byte that means zero. */ + if (a != NULL) { + a->length = 0; + } + ExpectIntEQ(ret = i2c_ASN1_INTEGER(a, NULL), 1); + ExpectNotNull(pp = (unsigned char*)XMALLOC(ret + 1, NULL, + DYNAMIC_TYPE_TMP_BUFFER)); + tpp = pp; + if (tpp != NULL) { + ExpectNotNull(XMEMSET(tpp, 0xff, ret + 1)); + ExpectIntEQ(i2c_ASN1_INTEGER(a, &tpp), 1); + tpp -= 1; + ExpectIntEQ(tpp[0], 0x00); + } + XFREE(pp, NULL, DYNAMIC_TYPE_TMP_BUFFER); wolfSSL_ASN1_INTEGER_free(a); #endif /* OPENSSL_EXTRA && !NO_ASN */ diff --git a/tests/api/test_ossl_ec.c b/tests/api/test_ossl_ec.c index 90b892dce7c..14b783bdd32 100644 --- a/tests/api/test_ossl_ec.c +++ b/tests/api/test_ossl_ec.c @@ -315,6 +315,10 @@ int test_wolfSSL_EC_POINT(void) EC_POINT* get_point = NULL; EC_POINT* infinity = NULL; EC_POINT* dup_point = NULL; +#if !defined(HAVE_ECC_BRAINPOOL) && !defined(WOLFSSL_SP_MATH) && \ + !defined(WOLF_CRYPTO_CB_ONLY_ECC) + EC_POINT* jacobian_point = NULL; +#endif BIGNUM* k = NULL; BIGNUM* Gx = NULL; BIGNUM* Gy = NULL; @@ -526,6 +530,37 @@ int test_wolfSSL_EC_POINT(void) ExpectIntEQ(EC_POINT_invert(group, new_point, ctx), 1); #endif +#ifndef HAVE_ECC_BRAINPOOL + /* group2 is a curve wolfCrypt was not built with, so its curve_idx is -1 + * and the curve parameters these operations need are not in ecc_sets. + * Each has to fail rather than index ecc_sets with that. Every call is + * guarded the way the function it calls is built. */ +#if !defined(WOLFSSL_ATECC508A) && !defined(WOLFSSL_ATECC608A) && \ + !defined(WOLFSSL_MICROCHIP_TA100) && !defined(HAVE_SELFTEST) && \ + !defined(WOLFSSL_SP_MATH) && !defined(WOLF_CRYPTO_CB_ONLY_ECC) && \ + !defined(WOLFSSL_NO_ECC_SW) + ExpectIntEQ(EC_POINT_add(group2, new_point, new_point, Gxy, ctx), 0); + ExpectIntEQ(EC_POINT_mul(group2, new_point, Gx, Gxy, k, ctx), 0); +#endif + ExpectIntEQ(EC_POINT_invert(group2, new_point, ctx), 0); +#if !defined(WOLFSSL_SP_MATH) && !defined(WOLF_CRYPTO_CB_ONLY_ECC) && \ + !defined(WOLFSSL_NO_ECC_SW) + /* get_affine_coordinates_GFp() only converts when the point is in + * Jacobian form, and the conversion is what needs the curve's prime. An + * already-affine point is returned without touching the curve, so give it + * one with Z != 1. */ + ExpectNotNull(jacobian_point = EC_POINT_dup(Gxy, group)); + if (jacobian_point != NULL) { + ExpectIntEQ(BN_set_word(jacobian_point->Z, 2), 1); + } + ExpectIntEQ(wolfSSL_EC_POINT_get_affine_coordinates_GFp(group2, + jacobian_point, X, Y, ctx), 0); + /* The same point is converted fine on a curve wolfCrypt does have. */ + ExpectIntEQ(wolfSSL_EC_POINT_get_affine_coordinates_GFp(group, + jacobian_point, X, Y, ctx), 1); +#endif +#endif + #if !defined(WOLFSSL_ATECC508A) && !defined(WOLFSSL_ATECC608A) && \ !defined(WOLFSSL_MICROCHIP_TA100) && \ !defined(HAVE_SELFTEST) && !defined(WOLFSSL_SP_MATH) && \ @@ -867,6 +902,10 @@ int test_wolfSSL_EC_POINT(void) BN_free(set_point_bn); EC_POINT_free(infinity); EC_POINT_free(dup_point); +#if !defined(HAVE_ECC_BRAINPOOL) && !defined(WOLFSSL_SP_MATH) && \ + !defined(WOLF_CRYPTO_CB_ONLY_ECC) + EC_POINT_free(jacobian_point); +#endif EC_POINT_free(new_point); EC_POINT_free(set_point); EC_POINT_clear_free(Gxy); diff --git a/tests/api/test_ossl_tsp.c b/tests/api/test_ossl_tsp.c index ef4605b70d9..b35ac7914f0 100644 --- a/tests/api/test_ossl_tsp.c +++ b/tests/api/test_ossl_tsp.c @@ -472,6 +472,7 @@ int test_wolfSSL_TS_REQ(void) nbuf[1] = (unsigned char)(MAX_TS_NONCE_SZ + 1); /* > max, no pad */ XMEMSET(nbuf + 2, 0x55, MAX_TS_NONCE_SZ + 1); bigNonce.data = nbuf; + bigNonce.dataMax = (unsigned int)sizeof(nbuf); bigNonce.length = 2 + MAX_TS_NONCE_SZ + 1; ExpectIntEQ(TS_REQ_set_nonce(req, &bigNonce), 0); } diff --git a/wolfcrypt/src/evp.c b/wolfcrypt/src/evp.c index 3b212155b64..0e759dbf925 100644 --- a/wolfcrypt/src/evp.c +++ b/wolfcrypt/src/evp.c @@ -9554,6 +9554,16 @@ static int PopulateRSAEvpPkeyDer(WOLFSSL_EVP_PKEY *pkey) XFREE(keyBuf, pkey->heap, DYNAMIC_TYPE_DER); pkey->pkey.ptr = (char*)derBuf; pkey->pkey_sz = (int)pkcs8Sz; + if (ret > 0) { + word32 hdrIdx = 0; + word32 hdrAlg = 0; + /* Locate the header in the DER just built. */ + ret = ToTraditionalInline_ex(derBuf, &hdrIdx, sz, + &hdrAlg); + if (ret >= 0) { + pkey->pkcs8HeaderSz = (word16)hdrIdx; + } + } } else { /* The encoding is abandoned but keyBuf stays on the pkey, @@ -10040,7 +10050,16 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) derBuf = (byte*)XMALLOC((size_t)derSz, pkey->heap, DYNAMIC_TYPE_OPENSSL); if (derBuf) { - if (wc_EccKeyToPKCS8(ecc, derBuf, &derSzOut) >= 0) { + word32 hdrIdx = 0; + word32 hdrAlg = 0; + /* Encode key as PKCS#8. */ + if (wc_EccKeyToPKCS8(ecc, derBuf, &derSzOut) < 0) { + XFREE(derBuf, pkey->heap, DYNAMIC_TYPE_OPENSSL); + derBuf = NULL; + } + /* Locate the PKCS#8 header in the DER just built. */ + else if (ToTraditionalInline_ex(derBuf, &hdrIdx, derSzOut, + &hdrAlg) >= 0) { derSz = (int)derSzOut; if (pkey->pkey.ptr) { /* The outgoing buffer can hold a private key @@ -10054,10 +10073,12 @@ static int ECC_populate_EVP_PKEY(WOLFSSL_EVP_PKEY* pkey, WOLFSSL_EC_KEY *key) } pkey->pkey_sz = (int)derSz; pkey->pkey.ptr = (char*)derBuf; - pkey->pkcs8HeaderSz = key->pkcs8HeaderSz; + pkey->pkcs8HeaderSz = (word16)hdrIdx; return WOLFSSL_SUCCESS; } else { + /* The buffer can hold a private key encoding. */ + ForceZero(derBuf, (word32)derSz); XFREE(derBuf, pkey->heap, DYNAMIC_TYPE_OPENSSL); derBuf = NULL; } diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 991a818a541..bd0f0e906e6 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -8495,6 +8495,12 @@ WOLFSSL_LOCAL int wolfssl_asn1_obj_set(WOLFSSL_ASN1_OBJECT* obj, const byte* der, word32 len, int addHdr); #endif +#if defined(OPENSSL_EXTRA) || defined(WOLFSSL_WPAS_SMALL) || \ + defined(OPENSSL_EXTRA_X509_SMALL) +WOLFSSL_LOCAL int wolfssl_asn1_integer_value(const WOLFSSL_ASN1_INTEGER* a, + word32* idx, int* len); +#endif + #if defined(OPENSSL_EXTRA) || defined(OPENSSL_EXTRA_X509_SMALL) WOLFSSL_LOCAL int pkcs8_encode(WOLFSSL_EVP_PKEY* pkey, byte* key, word32* keySz); diff --git a/wolfssl/ssl.h b/wolfssl/ssl.h index 24ebab8000d..7c3d2414fb5 100644 --- a/wolfssl/ssl.h +++ b/wolfssl/ssl.h @@ -3851,6 +3851,12 @@ WOLFSSL_API void wolfSSL_X509_CRL_free(WOLFSSL_X509_CRL *crl); #if defined(HAVE_CRL) && defined(OPENSSL_EXTRA) WOLFSSL_API WOLFSSL_X509_CRL* wolfSSL_X509_CRL_new(void); #ifdef WOLFSSL_CERT_GEN +/* revoked->serialNumber->data must be the bare serial value octets, with no + * DER tag or length, and ->length their count. That is what + * wolfSSL_X509_CRL_get_REVOKED() gives. An ASN1_INTEGER built with the + * ASN1_INTEGER API holds a DER INTEGER instead and is not accepted here: pass + * wolfSSL_ASN1_INTEGER_get0_data() and wolfSSL_ASN1_INTEGER_get_length() of + * it. */ WOLFSSL_API int wolfSSL_X509_CRL_add_revoked(WOLFSSL_X509_CRL* crl, WOLFSSL_X509_REVOKED* revoked); WOLFSSL_API int wolfSSL_X509_CRL_add_revoked_cert(WOLFSSL_X509_CRL* crl, diff --git a/wolfssl/wolfcrypt/asn_public.h b/wolfssl/wolfcrypt/asn_public.h index 63dc5426298..181a5702372 100644 --- a/wolfssl/wolfcrypt/asn_public.h +++ b/wolfssl/wolfcrypt/asn_public.h @@ -381,8 +381,20 @@ typedef struct WOLFSSL_ASN1_INTEGER { unsigned char* data; unsigned int dataMax; /* max size of data buffer */ WC_BITFIELD isDynamic:1; /* flag for if data pointer dynamic (1 is yes 0 is no) */ - - int length; /* Length of DER encoding. */ + /* How data is laid out: the value octets on their own when 1, a DER + * INTEGER - tag, length then value - when 0. Set by whoever fills the + * object in; the value octets must never be inspected to work it out, + * because a value may legitimately be byte-identical to a DER INTEGER + * encoding. + * + * Zero - and so a zero-filled object - means DER, which is what the + * intData comment above describes and what every producer here writes. + * The exceptions set it: X509_CRL_get_REVOKED() serial numbers, and + * X509_get_serialNumber() under WOLFSSL_QT or WOLFSSL_HAPROXY. */ + WC_BITFIELD dataIsRaw:1; + + int length; /* Length of data: of the value octets when dataIsRaw is set, + * of the DER encoding when it is not. */ int type; /* ASN.1 type. Includes negative flag. */ } WOLFSSL_ASN1_INTEGER;