fix(connection): Make set_config validator replacement transactional#5847
Open
alexw91 wants to merge 2 commits into
Open
fix(connection): Make set_config validator replacement transactional#5847alexw91 wants to merge 2 commits into
alexw91 wants to merge 2 commits into
Conversation
59c8459 to
a5ac431
Compare
s2n_connection_set_config wiped the connection's x509_validator before initializing a replacement from the new config. If the replacement init failed (e.g. X509_STORE_CTX_new returning NULL under OOM, or an invalid max chain depth), the connection was left with a wiped validator while conn->config still referenced the old config. A subsequent handshake would fail in cert validation. Stage the new validator into a stack-local struct. If any init step fails, wipe the local and propagate the error, leaving the connection's existing validator intact. Only swap on full success. Add a regression test that triggers a validator init failure via an invalid max chain depth and verifies the old validator is preserved.
a5ac431 to
c86f5d3
Compare
jmayclin
approved these changes
Apr 24, 2026
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
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.
Goal
Make the x509 validator replacement in
s2n_connection_set_configtransactional, so a failed init does not leave the connection with a wiped validator.Why
s2n_connection_set_configwiped the connection'sx509_validatorbefore initializing a replacement from the incoming config. If the replacement initialization failed (e.g.X509_STORE_CTX_newreturning NULL under OOM, ors2n_x509_validator_set_max_chain_depthrejecting an invalid depth), the function returned an error with the validator already wiped toUNINITstate whileconn->configstill referenced the previous config. A subsequent handshake attempt on that connection would fail in cert validation.How
The new validator is built into a stack-local
struct s2n_x509_validator. Each fallible step (s2n_x509_validator_init,s2n_x509_validator_set_max_chain_depth) is checked individually. On failure, the local is wiped to avoid leaking any partially-initialized OpenSSL objects, and the error is propagated. The connection's existing validator is untouched. Only after the local is fully initialized is the old validator wiped and replaced via struct assignment.Callouts
verify_host_fnassignment remains inside theelseblock in the same position relative to the other operations as in the original code. It is not part of the validator and does not need rollback.s2n_x509_validator_initcan partially populate the struct (e.g. settrust_store, allocatecert_chain_from_wireviask_X509_new_null()) before failing onX509_STORE_CTX_new. The explicits2n_x509_validator_wipe(&new_validator)on the init failure path prevents leaking those objects.s2n_x509_validator_init_no_x509_validationcannot currently fail (it only does assignments andsk_X509_new_null), but thePOSIX_GUARDis retained as a defensive measure.Testing
Added a regression test in
s2n_config_test.cthat:INITstate.max_verify_cert_chain_depthset to 0 (invalid) andmax_verify_cert_chain_depth_setenabled, which causess2n_x509_validator_set_max_chain_depthto fail withS2N_ERR_INVALID_ARGUMENT.s2n_connection_set_configwith the bad config and asserts it fails.conn->configstill points to the original config.conn->x509_validator.stateis stillINIT(notUNINIT).Related
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.