Add a runtime option to enable or disable the secure renegotation check. #9596
Conversation
|
🛟 Devin Lifeguard found 2 likely issues in this PR
@kareem-wolfssl |
dgarske
requested review from
Copilot and
dgarske
and removed request for
wolfSSL-Bot
There was a problem hiding this comment.
Pull request overview
This PR adds a runtime option to enable or disable the secure renegotiation (SCR) check in wolfSSL. The feature allows clients to control whether to enforce RFC 9325 requirements that servers acknowledge the secure renegotiation extension. Additionally, it implements sending a no_renegotiation alert when rejecting renegotiation attempts on servers without secure renegotiation support.
Key changes:
- Adds
scr_check_enabledbit field to the WOLFSSL structure with getter/setter API functions - Implements logic to send
no_renegotiationalert when server receives ClientHello after handshake completion without secure renegotiation support - Makes the existing SCR check in DoServerHello conditional on the new
scr_check_enabledflag
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssl/ssl.h | Adds API declarations for wolfSSL_get_scr_check_enabled and wolfSSL_set_scr_check_enabled |
| wolfssl/internal.h | Adds scr_check_enabled bit field to WOLFSSL structure |
| src/ssl.c | Implements getter/setter functions for the SCR check flag |
| src/internal.c | Initializes scr_check_enabled to 1 (enabled by default); adds no_renegotiation alert logic; makes SCR check conditional on the flag |
| tests/api.c | Adds comprehensive test for the SCR check enable/disable functionality |
| doc/dox_comments/header_files/ssl.h | Adds documentation for the new API functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| /* Test SCR check when server doesn't reply to secure_renegotiation. */ |
There was a problem hiding this comment.
There's a typo in the PR title and line 12847 comment: "renegotation" should be "renegotiation" (missing 'i').
| /* IO callback to remove secure renegotiation extension from ServerHello */ | ||
| static int test_SCR_check_remove_ext_io_cb(WOLFSSL *ssl, char *buf, int sz, void *ctx) | ||
| { | ||
| static int sentServerHello = FALSE; |
There was a problem hiding this comment.
The static variable 'sentServerHello' maintains state across multiple test invocations. This could cause test failures if the test is run multiple times in the same process, as the variable will remain TRUE after the first test execution. Consider resetting this variable at the beginning of each test or making it non-static and passed through the context parameter.
| return test_memio_write_cb(ssl, buf, sz, ctx); | ||
|
|
||
| /* Search for the extension in the buffer */ | ||
| for (i = 0; i < (size_t)sz - sizeof(renegExt); i++) { |
There was a problem hiding this comment.
The loop condition 'i < (size_t)sz - sizeof(renegExt)' could miss checking the last valid position. When i equals sz - sizeof(renegExt), we can still safely access buf[i] and buf[i+1] (since sizeof(renegExt) is 2). The condition should be 'i <= (size_t)sz - sizeof(renegExt)' to check all valid positions.
| if (ssl->options.handShakeState == HANDSHAKE_DONE && type == client_hello && | ||
| ssl->options.side == WOLFSSL_SERVER_END){ |
There was a problem hiding this comment.
There's inconsistent spacing in the conditional check. The opening parenthesis on line 18082 should align with standard formatting. The condition has extra spacing that makes it harder to read. Consider formatting as 'if (ssl->options.handShakeState == HANDSHAKE_DONE && type == client_hello && ssl->options.side == WOLFSSL_SERVER_END)'.
|
|
||
| \sa wolfSSL_set_scr_check_enabled | ||
| */ | ||
| byte wolfSSL_get_scr_check_enabled(WOLFSSL* ssl); |
There was a problem hiding this comment.
The return type in the documentation is 'byte' but the actual function declaration in wolfssl/ssl.h line 4731 returns 'int'. These should match for consistency and accuracy.
| byte wolfSSL_get_scr_check_enabled(WOLFSSL* ssl); | |
| int wolfSSL_get_scr_check_enabled(WOLFSSL* ssl); |
| #if !defined(HAVE_SECURE_RENEGOTIATION) | ||
| if (ssl->options.handShakeState == HANDSHAKE_DONE && type == client_hello && | ||
| ssl->options.side == WOLFSSL_SERVER_END){ | ||
| WOLFSSL_MSG("Renegotiation request rejected"); | ||
| SendAlert(ssl, alert_fatal, no_renegotiation); | ||
| WOLFSSL_ERROR_VERBOSE(SECURE_RENEGOTIATION_E); | ||
| return SECURE_RENEGOTIATION_E; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
The PR title and description mention sending a no_renegotiation alert when rejecting renegotiation attempts. However, this logic in internal.c only sends the alert when HAVE_SECURE_RENEGOTIATION is not defined, and checks if the handshake is done and a client_hello is received on the server side. This appears to be checking for renegotiation attempts when secure renegotiation is disabled entirely, which is a different scenario than checking whether SCR is enabled/acknowledged. Consider clarifying if this is the intended behavior or if this logic should also be conditioned on the scr_check_enabled flag.
3 participants
Description
Send no_renegotiation alert when rejecting renegotation attempt as defined in RFC 5246 section 7.2.2.
Fixes zd#19378
Testing
Built in tests
Checklist