FIPS security policies do not work when using `Provider::from_version("default_fips")`

[jkalez]

Problem:

When using the Rust bindings, I call security::Policy::from_version("default_fips"). I then pass this policy to a config::Builder and create a Config. The config is eventually used to create an s2n-quic Client & Server. However, it appears when using the "default_fips" policy, the generated ClientHellos, do not include any CipherSuites. See the attached pcap for details.

Solution:

ClientHellos are generated with some number of acceptable FIPS CipherSuites, or if there are no acceptable CipherSuites, the call to from_version fails.

• Does this change what S2N sends over the wire? If yes, explain. yes, by properly adding FIPS cipher suites
• Does this change any public APIs? If yes, explain. No
• Which versions of TLS will this impact? all FIPS versions

Requirements / Acceptance Criteria:

ClientHellos are generated with some number of acceptable FIPS CipherSuites. no_client_hello.pcapng.tar.gz

[lrstewart]

Thanks for bringing this issue to our attention!

The problem here isn't actually related to FIPS. The problem is that "default_fips" doesn't support TLS1.3 (yet) and QUIC requires TLS1.3.

It seems reasonable to error if we're constructing a ClientHello and discover no cipher suites are valid. We shouldn't send a ClientHello with no cipher suites.

But detecting setting an invalid security policy on a quic connection might be trickier, since order of operations would matter. We could error when a non-TLS1.3 policy is set on a quic connection or on a quic config. But we'd potentially run into ordering issues if we tried to error on a non-TLS1.3 policy set on a config which is then set on a quic connection. For example: setting a default config with the default policy on a quic connection, then overriding the policy at the connection level.

[jkalez]

Thanks for the info!

I can't find any issues directly addressing the first problem you described above:

The problem is that "default_fips" doesn't support TLS1.3 (yet) and QUIC requires TLS1.3.

Is this something the team is tracking or is on the road-map?

As far as error behavior goes, my 2 cents: It seems reasonable to me to fail on the call to config::Builder::build() if enable_quic() is set and a non TLS1.3 policy is set. IMO you don't have to an perhaps shouldn't fail when the builder sets those options, only when the builder tries to build() with those options. At that point, the user would be trying to build an invalid Config (QUIC but not TLS1.3), so the failure is acceptable and expected.

Of course, this still leaves the potential runtime issue when the policy is modified at the connection level. I don't know that I have a great answer on that as I'm not familiar with that part of the API.

[lrstewart]

I can't find any issues directly addressing the first problem you described above

It's not really a problem per-se that default_fips doesn't support TLS1.3, unless you try to use it with QUIC. There are many immutable, versioned policies that will never support TLS1.3 and therefore never be usable with QUIC. However, we do plan to add TLS1.3 support to default_fips.

IMO you don't have to an perhaps shouldn't fail when the builder sets those options, only when the builder tries to build() with those options

Oh, definitely agreed. The issue is that while we have a builder in Rust, we don't have one in the underlying C 🙃 Although I suppose since this is a quic-specific problem and the primary customer is therefore s2n-quic, fixing those edge cases I called out only in Rust might not be the end of the world.

[lrstewart]

While working on fixes for this I realized I removed documentation of the suggested TLS1.3 + FIPS policy when I updated our default policies: https://github.com/aws/s2n-tls/commit/5b316bd69669fd07944ecbe542374ef34b997609 That's an oversight since the updated policies still don't cover that TLS1.3 + FIPS niche. I'll put the documentation back.

In the meantime, if you were wondering which policy to use, the suggested TLS1.3 + FIPS policy was 20230317.