Repository navigation
Conversation
SSL_CTX_set_ciphersuites and SSL_set_ciphersuites installed an empty TLS 1.3 list before reporting that a rule matched nothing, and the legacy cipher-list setters returned before merging the TLS 1.3 suites back in, so a rule that matched nothing left the combined list empty and a TLS 1.3 client rejected whatever suite the server chose. Parse the TLS 1.3 rule aside and commit it only after the combined list is rebuilt, and have the legacy setters always re-merge the TLS 1.3 suites while keeping the OpenSSL behavior of clearing the TLS 1.2 and below suites on no match. The setters no longer push a redundant SSL_R_NO_CIPHER_MATCH over the error ssl_create_cipher_list already reports, so a strict parse error now surfaces as SSL_R_INVALID_COMMAND; ERR_get_error ordering is unchanged. Document the failure semantics in ssl.h, and add regression tests, including a standalone ssl_ciphers_mem_test that injects allocation failures to check each setter is a no-op on failure. Testing: ssl_test (2716 passed) in Debug with ENABLE_CRYPTO_POLICIES=ON; ssl_ciphers_mem_test (15 passed) in Debug and in a FIPS shared-library build.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const bool had_local_list = ssl && ssl->config->cipher_list; | ||
|
|
||
| ERR_clear_error(); | ||
| int ret; |
Contributor
There was a problem hiding this comment.
warning: variable 'ret' is not initialized [cppcoreguidelines-init-variables]
Suggested change
| int ret; | |
| int ret = 0; |
|
|
||
| ERR_clear_error(); | ||
| int ret; | ||
| size_t count, reallocations; |
Contributor
There was a problem hiding this comment.
warning: variable 'count' is not initialized [cppcoreguidelines-init-variables]
Suggested change
| size_t count, reallocations; | |
| size_t count = 0, reallocations; |
|
|
||
| ERR_clear_error(); | ||
| int ret; | ||
| size_t count, reallocations; |
Contributor
There was a problem hiding this comment.
warning: variable 'reallocations' is not initialized [cppcoreguidelines-init-variables]
Suggested change
| size_t count, reallocations; | |
| size_t count, reallocations = 0; |
| ERR_clear_error(); | ||
| int ret; | ||
| size_t count, reallocations; | ||
| bool failed; |
Contributor
There was a problem hiding this comment.
warning: variable 'failed' is not initialized [cppcoreguidelines-init-variables]
Suggested change
| bool failed; | |
| bool failed = false; |
Contributor
|
🔒 Security Review — View Report Please review before merging. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3638 +/- ##
==========================================
+ Coverage 78.46% 78.56% +0.09%
==========================================
Files 705 706 +1
Lines 130645 130932 +287
Branches 17912 17948 +36
==========================================
+ Hits 102516 102866 +350
+ Misses 27150 27079 -71
- Partials 979 987 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context and motivation
A cipher rule that matches nothing currently wipes out the TLS 1.3 suites too.
SSL_CTX_set_ciphersuitesstores an empty TLS 1.3 list before reporting the failure, and the legacy*_set_cipher_listsetters return before re-merging the TLS 1.3 suites into the combined list. Either way the endpoint is left unable to negotiate TLS 1.3; a client rejects the server's choice withSSL_R_WRONG_CIPHER_RETURNED.Description of changes
A failed TLS 1.3 setter now leaves both lists untouched, and a failed legacy setter keeps the TLS 1.3 suites while still clearing the TLS 1.2 and below suites, as OpenSSL does. The setters also stop pushing a redundant
SSL_R_NO_CIPHER_MATCHon top of the error already reported, so a strict parse error surfaces asSSL_R_INVALID_COMMAND(ERR_get_error()ordering is unchanged). The failure semantics are now documented on each setter inssl.h.Testing
New
ssl_ciphers_test.cccases check that a no-match rule leaves the TLS 1.3 policy (including order) intact and that TLS 1.3 handshakes still succeed afterwards, which fails onmain. A new standalonessl_ciphers_mem_testfails each allocation in turn to verify the setters are no-ops on failure.ssl_testandssl_ciphers_mem_testpass in Debug and in a FIPS shared-library build.Review considerations
Success paths are unchanged. A caller that ignores a 0 return now has working TLS 1.3 instead of no ciphers at all.
SSL_CTX_set_cipher_list(ctx, "")still returns 1, an existing divergence from OpenSSL and BoringSSL that is now documented. Making the server honor an explicitly empty per-connection TLS 1.3 list is a deliberate behavior change and will come as a separate PR.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.