Seed cipher lists and version bounds from the system crypto policy - #3503
Seed cipher lists and version bounds from the system crypto policy#3503WillChilds-Klein wants to merge 3 commits into
Conversation
| # temporarily clears this variable to read the real system policy file, so the | ||
| # feature is still validated end-to-end on Amazon Linux 2023 (and skips | ||
| # elsewhere). | ||
| export AWSLC_CRYPTO_POLICY_FILE=/nonexistent/aws-lc-crypto-policy-disabled |
There was a problem hiding this comment.
all well and good, but we still need an integration test against an actual policy file on the filesystem. let's have a test run only the crypto policy tests against AL23's default crypto policy file.
There was a problem hiding this comment.
Done, and it found a real bug.
CryptoPolicySystemTest now runs with no environment override, so SSL_CTX_new reads whatever policy file the host actually has, and it compares the auto-seeded context against one the test seeds by hand from the same path. The Amazon Linux 2023 job sets AWSLC_CRYPTO_POLICY_TEST_REQUIRE_SYSTEM=1, which turns a missing file into a failure; elsewhere the suite skips. The CI script re-runs --gtest_filter='CryptoPolicy*' against the live /etc/crypto-policies/back-ends/opensslcnf.config after the bulk suite, and dumps the policy name and file first so a failure is diagnosable from the log.
The fixture in this PR is also now the AL2023 DEFAULT file copied byte for byte instead of a hand-written approximation.
What that turned up: every policy AL2023 ships writes Groups = *X25519:.... The * is the OpenSSL 3.5 key-share marker, which AWS-LC's name lookup does not handle, so X25519 -- the first and most-preferred group -- was silently dropped from every seeded context on AL2023. Fixed in #3504, which strips * and ? and honors - as removal, with the group assertions the new suite needs to catch it.
Verified against all four real policy files (DEFAULT, FIPS, FUTURE, LEGACY) pulled out of the AL2023 container image.
952a912 to
6c7c325
Compare
| // Seeding is best-effort, so the errors its failures queue must not reach the | ||
| // caller. Neither may the caller's own queue be disturbed, since this runs | ||
| // inside |SSL_CTX_new|, which no caller expects to touch the error queue at | ||
| // all. Cutting the queue back to the length it had leaves everything below the | ||
| // cut alone: the caller's entries, the data pointer their last | ||
| // |ERR_get_error_line_data| handed out, and any mark they set. It allocates | ||
| // nothing, so it has no failure mode, and if seeding queued nothing it does | ||
| // nothing. | ||
| const size_t num_errors = ERR_num_errors(); |
There was a problem hiding this comment.
Reason not to use the ERR_save_state?
There was a problem hiding this comment.
That was the approach i took initially, preserving mark state across copies. Claude found a few issues with that:
ERR_save_statenull return value indicates both queue empty + OOM- saving the mark across copies (effectively re-setting the mark on every restore) had some unintended consequences elsewhere in the code base
- performance -- 2 additional malloc's on every
SSL_CTX_newif error queue non-empty
| @@ -3732,7 +3737,29 @@ bool ssl_crypto_policy_parse_file(const char *path, CryptoPolicyConfig *out); | |||
| // The environment override is ignored in processes running with elevated | |||
| // privileges, where the environment sits on the far side of a privilege boundary | |||
| // from the root-owned default path. | |||
| const char *ssl_crypto_policy_default_path(void); | |||
| // | |||
| // Marked with OPENSSL_EXPORT to make it available for unit tests. | |||
| OPENSSL_EXPORT const char *ssl_crypto_policy_default_path(void); | |||
|
|
|||
| // ssl_ctx_apply_crypto_policy seeds |ctx| from the crypto-policies OpenSSL | |||
| // back-end file at |path|. It is best-effort and never fails: a missing or | |||
| // malformed file, or a directive AWS-LC rejects, leaves the corresponding | |||
| // built-in default in place. Errors already queued by the caller are preserved; | |||
| // errors this function provokes are not. | |||
| // | |||
| // |is_dtls| selects the TLS.* vs DTLS.* protocol directives. |version_locked| | |||
| // must be true when |ctx| came from one of the legacy version-locked | |||
| // |SSL_METHOD|s (|ssl_method_st.version| non-zero), in which case the policy's | |||
| // protocol floor and ceiling are skipped: the caller pinned a single version and | |||
| // a system-wide default must not silently widen it. | |||
| // | |||
| // The parsed file is cached process-wide, keyed on |path|, so repeated | |||
| // |SSL_CTX_new| calls do not re-read it. | |||
| // | |||
| // Marked with OPENSSL_EXPORT to make it available for unit tests. | |||
| OPENSSL_EXPORT void ssl_ctx_apply_crypto_policy(SSL_CTX *ctx, const char *path, | |||
| bool is_dtls, | |||
| bool version_locked); | |||
There was a problem hiding this comment.
With -DENABLE_DIST_PKG=ON -DBUILD_SHARED_LIBS=ON -DENABLE_CRYPTO_POLICIES=ON, ssl_test fails to link on Linux with undefined references to all three helpers.
OPENSSL_EXPORTis not sufficient andssl/libssl.map'slocal: *;hides anything missing from the registry. (Disabling symbol versioning makes the same build link.)- You should register these as
PRIVATE_CXXinssl/libssl.txtand regenerate the map (or keep them private and test through public APIs?). The extractor also needs anAWSLC_CRYPTO_POLICIESconfiguration; its default scan misses these declarations.
|
|
||
| ApplyPolicyToCtx(ctx, path, is_dtls, version_locked); | ||
|
|
||
| ERR_pop_to_count(num_errors); |
There was a problem hiding this comment.
Restoring the count cannot restore entries evicted by overflow. I reproduced this through SSL_CTX_new with 14 caller errors (capacity 15) and a policy whose CipherString and Ciphersuites both match nothing: the two probes evict the oldest caller error, and ERR_pop_to_count(14) leaves one probe error behind. An already-full queue also fails.
Skipping only when num_errors == ERR_NUM_ERRORS - 1 is not enough. This should prevent overflow across the whole seeding operation and add tests for both full and one-slot-free caller queues.
|
Also address the secure-execution and partial-read issues noted on #3502. |
Stack, split out of #3442, to merge bottom-up: 1. **#3501 -- error-queue primitives (this PR)** 2. #3502 -- policy file reader 3. #3503 -- cipher lists and version bounds 4. #3504 -- groups and signature algorithms 5. #3505 -- post-quantum defaults ## Description - Adds `ERR_num_errors` and `ERR_pop_to_count`, so code that calls into libcrypto on a caller's behalf can drop the errors it raised and leave the queue it was handed untouched. - The existing mark APIs cannot do this. `ERR_set_mark` needs an entry to mark, so it is a no-op on an empty queue, and popping to a mark consumes one the caller had already set. - `ERR_clear_error` and `ERR_restore_state` rebuild the queue, which dangles the data pointer the caller got from its last `ERR_get_error_line_data`. - A count is a position rather than a mark, so it nests inside a caller's mark without disturbing it. ## Testing / verification - New error-queue tests cover popping back to a recorded count, a count taken from an empty queue, and a count at or above the queue's length. - One case fills the ring past its capacity to confirm a stale count leaves the caller's errors alone. - One case wraps a nested call in the caller's own mark and an error carrying a data string, then checks the mark still pops and the string is intact. - One case pins that a mark does not survive `ERR_save_state`/`ERR_restore_state`, since a snapshot can be restored many times and would re-arm a mark nobody set. 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.
6c7c325 to
e12618d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/crypto-policies-2-parser #3503 +/- ##
================================================================
Coverage ? 78.37%
================================================================
Files ? 700
Lines ? 125744
Branches ? 17390
================================================================
Hits ? 98557
Misses ? 26315
Partials ? 872 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e12618d to
390a732
Compare
SSL_CTX_new now applies the policy's CipherString, Ciphersuites, and protocol bounds when built with ENABLE_CRYPTO_POLICIES. Seeding is best-effort, so it cannot report failure and must leave the caller's error queue as it found it. Two directives cannot be applied blind. A failing SSL_CTX_set_cipher_list installs its empty result before returning, so a rule AWS-LC cannot satisfy would leave the context with no ciphers rather than the defaults. The version setters validate each bound only against the method's whole range, so an inverted pair would be accepted and fail every later handshake.
The policy file was re-read on every SSL_CTX_new; cache the parse keyed on the resolved path so a changed AWSLC_CRYPTO_POLICY_FILE still misses. The three internal entry points need OPENSSL_EXPORT to reach ssl_test across a shared libssl, which -fvisibility=hidden otherwise blocks. A temp-file write failure now fails the test instead of skipping it; the platform skip moves to the fixtures.
Every existing case composes its own fixture, so nothing noticed that the framework's Groups value carries a '*' key-share marker and its Ciphersuites value names a suite AWS-LC lacks. The fixture now copies Amazon Linux 2023's DEFAULT file verbatim, and a new suite runs against whatever the host installed: the AL2023 CI job sets AWSLC_CRYPTO_POLICY_TEST_REQUIRE_SYSTEM, so an absent file fails there and skips elsewhere.
390a732 to
cb08448
Compare
|
🔒 Security Review — View Report Please review before merging. |
4 participants
Stack, split out of #3442, to merge bottom-up:
Description
SSL_CTX_newnow applies the policy's cipher lists and protocol bounds when built with the flag, taking the TLS or DTLS directives according to the context's method.SSL_CTX_set_cipher_listinstalls its empty result before reporting failure, and the version setters check each bound only against the method's full range, so an inverted pair is accepted and then fails every handshake.@SECLEVEL=Nprefix ofCipherStringis parsed and dropped, since AWS-LC has no security levels; the key-size and hash constraints a level implies are therefore not enforced.openssl.cnf: a policy change takes effect only in processes started afterward, while a changed override path still misses.Testing / verification
crypto-policiesitself renders, comparing the automatically seeded context against one seeded by hand from the same path; an absent file fails the job rather than skipping.ERR_get_error_line_data.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.