-
Notifications
You must be signed in to change notification settings - Fork 987
Harden chain depth bounds and parser input validation #10209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
94b36c9
eb180db
359fb7e
0ca238c
5b8b61b
ed11ac7
1cdf37f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3831,6 +3831,108 @@ static int test_wolfSSL_CTX_use_certificate_chain_buffer_format(void) | |
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| /* wolfSSL_get_chain_{length,cert,X509} must reject out-of-range idx. */ | ||
| static int test_wolfSSL_get_chain_idx_bounds(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(SESSION_CERTS) && \ | ||
| defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) | ||
| struct test_memio_ctx test_ctx; | ||
| WOLFSSL_CTX* ctx_c = NULL; | ||
| WOLFSSL_CTX* ctx_s = NULL; | ||
| WOLFSSL* ssl_c = NULL; | ||
| WOLFSSL* ssl_s = NULL; | ||
| WOLFSSL_X509_CHAIN* chain = NULL; | ||
| int count = 0; | ||
|
|
||
| XMEMSET(&test_ctx, 0, sizeof(test_ctx)); | ||
| ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, | ||
| wolfTLS_client_method, wolfTLS_server_method), 0); | ||
| ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); | ||
|
|
||
| ExpectNotNull(chain = wolfSSL_get_peer_chain(ssl_c)); | ||
| ExpectIntGT(count = wolfSSL_get_chain_count(chain), 0); | ||
|
|
||
| ExpectIntEQ(wolfSSL_get_chain_length(chain, -1), 0); | ||
| ExpectIntEQ(wolfSSL_get_chain_length(chain, count), 0); | ||
| ExpectIntEQ(wolfSSL_get_chain_length(chain, MAX_CHAIN_DEPTH), 0); | ||
| ExpectNull(wolfSSL_get_chain_cert(chain, -1)); | ||
| ExpectNull(wolfSSL_get_chain_cert(chain, count)); | ||
| ExpectNull(wolfSSL_get_chain_cert(chain, MAX_CHAIN_DEPTH)); | ||
| #ifdef OPENSSL_EXTRA | ||
| { | ||
| WOLFSSL_X509* x = NULL; | ||
| ExpectNull(x = wolfSSL_get_chain_X509(chain, -1)); | ||
| if (x != NULL) { wolfSSL_X509_free(x); x = NULL; } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 [Low] Max-depth load test overshoots the limit and omits the exact-boundary success case The test sets Fix: Add a positive boundary test (exactly MAX_CHAIN_DEPTH certs -> success) and clarify the per-file cert count in the comment. |
||
| ExpectNull(x = wolfSSL_get_chain_X509(chain, count)); | ||
| if (x != NULL) { wolfSSL_X509_free(x); x = NULL; } | ||
| ExpectNull(x = wolfSSL_get_chain_X509(chain, MAX_CHAIN_DEPTH)); | ||
| if (x != NULL) { wolfSSL_X509_free(x); x = NULL; } | ||
| } | ||
| #endif | ||
|
|
||
| wolfSSL_free(ssl_c); | ||
| wolfSSL_free(ssl_s); | ||
| wolfSSL_CTX_free(ctx_c); | ||
| wolfSSL_CTX_free(ctx_s); | ||
| #endif | ||
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| /* 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; | ||
| #if !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && !defined(NO_TLS) && \ | ||
| !defined(NO_WOLFSSL_CLIENT) && !defined(NO_RSA) && \ | ||
| defined(WOLFSSL_PEM_TO_DER) | ||
| WOLFSSL_CTX* ctx = NULL; | ||
| unsigned char* one = NULL; | ||
| unsigned char* buf = NULL; | ||
| size_t oneLen = 0; | ||
| 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; | ||
|
|
||
| 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, buf, | ||
| (long)(oneLen * (size_t)atMax + tailLen)), WOLFSSL_SUCCESS); | ||
| wolfSSL_CTX_free(ctx); | ||
| 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 | ||
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| static int test_wolfSSL_CTX_use_certificate_chain_file_format(void) | ||
| { | ||
| EXPECT_DECLS; | ||
|
|
@@ -34777,6 +34879,8 @@ TEST_CASE testCases[] = { | |
| TEST_DECL(test_wolfSSL_CTX_add1_chain_cert), | ||
| TEST_DECL(test_wolfSSL_add_to_chain_overflow), | ||
| TEST_DECL(test_wolfSSL_CTX_use_certificate_chain_buffer_format), | ||
| TEST_DECL(test_wolfSSL_CTX_use_certificate_chain_buffer_max_depth), | ||
| TEST_DECL(test_wolfSSL_get_chain_idx_bounds), | ||
| TEST_DECL(test_wolfSSL_CTX_use_certificate_chain_file_format), | ||
| TEST_DECL(test_wolfSSL_use_certificate_chain_file), | ||
| TEST_DECL(test_wolfSSL_CTX_trust_peer_cert), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚪ [Info] SendTls13Certificate depth guard is now defense-in-depth only and its direct test path was removed
With the new
ProcessUserChainlimit, an oversized chain can no longer be loaded (it fails with MAX_CHAIN_ERROR at load time), so the newssl->buffers.certChainCnt > MAX_CHAIN_DEPTHguard insideSendTls13Certificateis effectively unreachable via the normal load path -- it is a reasonable belt-and-suspenders check, but untested. The PR simultaneously deletestest_dtls13_oversized_cert_chain, which was the only test that drove an oversized chain through the actual TLS1.3 certificate-send path. The deletion itself is justified (that test expectedWOLFSSL_SUCCESSon load, which is no longer possible), and thetest_dtls13.hmacro list trailing-comma update is correct. Net effect is reduced direct coverage of the send-side guard.Fix: Optional: keep the guard (cheap and defensive) but note in a comment that it is unreachable under normal loading, or add a unit test that sets ssl->buffers.certChainCnt directly to exercise the MAX_CHAIN_ERROR return.