Skip to content
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

fix: better error for setting bad quic policy #4598

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Jun 12, 2024

Resolved issues:

Related to #4594

Description of changes:

A customer ran into the situation where they attempted to set "default_fips" as their security policy while using s2n-quic. "default_fips" does not include any TLS1.3 cipher suites, so was not valid and led to runtime failures.

This change makes an (admittedly inadequate) attempt to prevent similar failures. My stance is that it's better than nothing-- see Call-outs below.

However, this change is still maybe a breaking change. Technically you could call s2n_config_enable_quic, s2n_config_set_cipher_preferences(TLS1.2), and then s2n_config_set_cipher_preferences(TLS1.3) before this change without issue, but that sequence of calls would now result in an error. I don't know if that is a realistic use case though, and it's not a use case that should obviously work. Once you've enabled quic, a TLS1.2-only policy is wrong.

Call-outs:

Could we do better? I couldn't think of an actually good solution.

The biggest problem use cases:

  1. s2n_config_set_cipher_preferences(TLS1.2), s2n_config_enable_quic, s2n_config_set_cipher_preferences(TLS1.3): This is why we can't validate in s2n_config_enable_quic. The application is allowed to change the security policy later, and this sequence is valid.
  2. s2n_connection_set_cipher_preferences(TLS1.2), s2n_connection_enable_quic, s2n_connection_set_cipher_preferences(TLS1.3): Basically the same as the previous case. This is why we can't validate in s2n_connection_enable_quic. The application is allowed to change the security policy later, and this sequence is valid.
  3. s2n_config_set_cipher_preferences(TLS1.2), s2n_config_enable_quic, s2n_connection_set_config: Not a valid sequence. We could catch the error if we validated in s2n_connection_set_config, but the error would have nothing to do with the connection and be fairly unexpected. In Rust, it would not show up in the config::Builder.
  4. s2n_config_set_cipher_preferences(TLS1.2), s2n_connection_set_config, s2n_connection_enable_quic: Not a valid sequence. We couldn't catch this error even if we validated in s2n_connection_set_config.
  5. s2n_config_enable_quic, s2n_connection_set_config, s2n_connection_set_cipher_preferences(tls1.3): A valid sequence, but would be broken by any validation of the config in s2n_connection_set_config or config::Builder. The config isn't valid on its own, but is valid in combination with the connection.

Validation in s2n_connection_set_config or config::Builder couldn't catch cases 2 or 4. And because of case 5, I don't believe we can actually do any validation in s2n_connection_set_config or config::Builder without breaking valid use cases.

Since the only current quic integration is s2n-quic and therefore Rust, we could focus on our Rust bindings, specifically on config::Builder::build. Given the above constraints, the only "better" solutions I can think of are:

  1. Add some sort of new validation method to call from config::Builder::build. This might be useful to C customers too, except that we couldn't assume that our C customers actually call it. So it'd probably go in s2n_internal.h. Not possible because of case 5.
  2. Save the policy set on the builder and delay calling s2n_config_set_cipher_preferences until config::Builder::build. That's not free because the policy can be backed by a CString we'd need to clone because the builder only takes a reference to the policy. I suppose the config builder probably doesn't need to be crazy performant, but it does seem really wasteful :/
  3. Add a s2n_config_get_cipher_preferences method to s2n_internal.h to call from config::Builder::build so that we can call s2n_config_set_cipher_preferences again from config::Builder::build. That just seems like a more hacky version of solution 1 though.

Testing:

Unit tests.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jun 12, 2024
@camshaft
Copy link
Contributor

Add some sort of new validation method to call from config::Builder::build. This might be useful to C customers too, except that we couldn't assume that our C customers actually call it. So it'd probably go in s2n_internal.h.

This would be my preference

@lrstewart
Copy link
Contributor Author

Add some sort of new validation method to call from config::Builder::build. This might be useful to C customers too, except that we couldn't assume that our C customers actually call it. So it'd probably go in s2n_internal.h.

This would be my preference

I started on that solution and ran into a problem. The case "s2n_config_enable_quic, s2n_connection_set_config, s2n_connection_set_cipher_preferences(tls1.3)" is valid, but would be flagged as invalid by any check we do in s2n_connection_set_config / config::Builder::build. Validating in config::Builder::build might not be possible at all given the connection overrides.

Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants