From 1cdf37f321df98746839d660f82c6ad5866cdf68 Mon Sep 17 00:00:00 2001 From: Colton Willey Date: Mon, 8 Jun 2026 17:27:26 -0700 Subject: [PATCH] Address review feedback on chain-depth and parser hardening - ProcessUserChain: enforce MAX_CHAIN_DEPTH after a cert is parsed, so a full-depth chain followed by trailing data still loads instead of failing with MAX_CHAIN_ERROR. - SendCertificateStatus: error on an over-long OCSP chain instead of silently truncating the stapled response; keep the chainOcspRequest[] index bound. - wc_PKCS7_DecodeAuthEnvelopedData: bounds-check the authTag tag read under NO_PKCS7_STREAM. - Tests: chain-depth test now pins the boundary (exactly MAX_CHAIN_DEPTH loads, one more rejected). --- src/internal.c | 2 +- src/ssl_load.c | 14 ++++++------- tests/api.c | 49 ++++++++++++++++++++++++++++++------------- wolfcrypt/src/pkcs7.c | 6 ++++++ 4 files changed, 48 insertions(+), 23 deletions(-) diff --git a/src/internal.c b/src/internal.c index 4ca81cfcb2..31fd94354d 100644 --- a/src/internal.c +++ b/src/internal.c @@ -26238,7 +26238,7 @@ int SendCertificateStatus(WOLFSSL* ssl) } if (chain && chain->buffer) { - while (ret == 0 && i < MAX_CHAIN_DEPTH && + while (ret == 0 && idx + OPAQUE24_LEN < chain->length) { c24to32(chain->buffer + idx, &der.length); idx += OPAQUE24_LEN; diff --git a/src/ssl_load.c b/src/ssl_load.c index 494f290e3e..b52eadf62c 100644 --- a/src/ssl_load.c +++ b/src/ssl_load.c @@ -325,17 +325,17 @@ static int ProcessUserChain(WOLFSSL_CTX* ctx, WOLFSSL* ssl, while ((ret == 0) && (consumed < sz)) { DerBuffer* part = NULL; - /* Enforce maximum chain depth. */ - if (cnt >= MAX_CHAIN_DEPTH) { - WOLFSSL_MSG("Chain depth limit reached"); - ret = MAX_CHAIN_ERROR; - break; - } - /* Get a certificate as DER. */ ret = DataToDerBuffer(buff + consumed, (word32)(sz - consumed), format, type, info, heap, &part, NULL); if (ret == 0) { + /* Enforce maximum chain depth once a real cert is parsed. */ + if (cnt >= MAX_CHAIN_DEPTH) { + WOLFSSL_MSG("Chain depth limit reached"); + FreeDer(&part); + ret = MAX_CHAIN_ERROR; + break; + } /* Process the user certificate. */ ret = ProcessUserCert(ctx->cm, &part, type, verify, chain.buffer, &idx, (word32)maxSz); diff --git a/tests/api.c b/tests/api.c index 1723e605fc..7757f53fc2 100644 --- a/tests/api.c +++ b/tests/api.c @@ -3879,7 +3879,9 @@ static int test_wolfSSL_get_chain_idx_bounds(void) return EXPECT_RESULT(); } -/* Reject chain buffers containing more than MAX_CHAIN_DEPTH certificates. */ +/* Chain-depth boundary: exactly MAX_CHAIN_DEPTH chain certs (plus trailing data + * that forces one more parse pass) must load; one more cert must be rejected. + * cliCertFile is single-cert, so copy 0 is the leaf and N copies => N-1 chain. */ static int test_wolfSSL_CTX_use_certificate_chain_buffer_max_depth(void) { EXPECT_DECLS; @@ -3888,26 +3890,43 @@ static int test_wolfSSL_CTX_use_certificate_chain_buffer_max_depth(void) defined(WOLFSSL_PEM_TO_DER) WOLFSSL_CTX* ctx = NULL; unsigned char* one = NULL; - unsigned char* big = NULL; + unsigned char* buf = NULL; size_t oneLen = 0; - size_t bigLen; + const char* tail = "# trailing comment\n"; + size_t tailLen = XSTRLEN(tail); + /* +1 copy for the leaf yields exactly MAX_CHAIN_DEPTH chain certs. */ + const int atMax = MAX_CHAIN_DEPTH + 1; int i; - const int nCerts = MAX_CHAIN_DEPTH + 1; - ExpectIntEQ(load_file(svrCertFile, &one, &oneLen), 0); - bigLen = oneLen * (size_t)nCerts; - ExpectNotNull(big = (unsigned char*)XMALLOC(bigLen, NULL, - DYNAMIC_TYPE_TMP_BUFFER)); - for (i = 0; EXPECT_SUCCESS() && i < nCerts; i++) - XMEMCPY(big + (size_t)i * oneLen, one, oneLen); + ExpectIntEQ(load_file(cliCertFile, &one, &oneLen), 0); + /* Exactly MAX_CHAIN_DEPTH chain certs + trailing data: loads. */ + ExpectNotNull(buf = (unsigned char*)XMALLOC( + oneLen * (size_t)atMax + tailLen, NULL, DYNAMIC_TYPE_TMP_BUFFER)); + for (i = 0; EXPECT_SUCCESS() && i < atMax; i++) + XMEMCPY(buf + (size_t)i * oneLen, one, oneLen); + if (buf != NULL) + XMEMCPY(buf + (size_t)atMax * oneLen, tail, tailLen); ExpectNotNull(ctx = wolfSSL_CTX_new(wolfSSLv23_client_method())); - ExpectIntEQ(wolfSSL_CTX_use_certificate_chain_buffer(ctx, big, - (long)bigLen), WC_NO_ERR_TRACE(MAX_CHAIN_ERROR)); - + ExpectIntEQ(wolfSSL_CTX_use_certificate_chain_buffer(ctx, buf, + (long)(oneLen * (size_t)atMax + tailLen)), WOLFSSL_SUCCESS); wolfSSL_CTX_free(ctx); - if (big != NULL) - XFREE(big, NULL, DYNAMIC_TYPE_TMP_BUFFER); + ctx = NULL; + if (buf != NULL) + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); + buf = NULL; + + /* One more chain cert: rejected. */ + ExpectNotNull(buf = (unsigned char*)XMALLOC(oneLen * (size_t)(atMax + 1), + NULL, DYNAMIC_TYPE_TMP_BUFFER)); + for (i = 0; EXPECT_SUCCESS() && i < atMax + 1; i++) + XMEMCPY(buf + (size_t)i * oneLen, one, oneLen); + ExpectNotNull(ctx = wolfSSL_CTX_new(wolfSSLv23_client_method())); + ExpectIntEQ(wolfSSL_CTX_use_certificate_chain_buffer(ctx, buf, + (long)(oneLen * (size_t)(atMax + 1))), WC_NO_ERR_TRACE(MAX_CHAIN_ERROR)); + wolfSSL_CTX_free(ctx); + if (buf != NULL) + XFREE(buf, NULL, DYNAMIC_TYPE_TMP_BUFFER); if (one != NULL) XFREE(one, NULL, DYNAMIC_TYPE_TMP_BUFFER); #endif diff --git a/wolfcrypt/src/pkcs7.c b/wolfcrypt/src/pkcs7.c index 5ab3c54e34..df6ede4296 100644 --- a/wolfcrypt/src/pkcs7.c +++ b/wolfcrypt/src/pkcs7.c @@ -15197,6 +15197,12 @@ authenv_atrbend: localIdx = idx; + #ifdef NO_PKCS7_STREAM + if (ret == 0 && localIdx >= pkiMsgSz) { + ret = BUFFER_E; + } + #endif + /* Get authTag OCTET STRING */ if (ret == 0 && pkiMsg[localIdx] != ASN_OCTET_STRING) { ret = ASN_PARSE_E;