Subject: Re: Re: Add support to TLS 1.3 cipher suites and curves lists

From: Jacob Champion <[email-masked]>
Date: Fri, 7 Jun 2024 10:14:50 -0700

On Fri, Jun 7, 2024 at 3:02 AM Erica Zhang <[email-masked]> wrote:
>
> For some security consideration, we prefer to use TLS1.3 cipher suites in our product with some customization values instead of default value "HIGH:MEDIUM:+3DES:!aNULL". Moreover we prefer to set a group of ecdh keys instead of a single value.

+1 for the curve list feature, at least. No opinions on the 1.3
ciphersuites half, yet.

I've added this patch to my planned review for the v18 cycle. Some
initial notes:

- Could you separate the two features into two patches? That would
make it easier for reviewers. (They can still share the same thread
and CF entry.)
- The "curve" APIs have been renamed "group" in newer OpenSSLs for a
while now, and we should probably use those if possible.
- I think parsing apart the groups list to check NIDs manually could
lead to false negatives. From a docs skim, 3.0 allows providers to add
their own group names, and 3.3 now supports question marks in the
string to allow graceful fallbacks.
- I originally thought it'd be better to just stop calling
SSL_set_tmp_ecdh() entirely by default, so we could use OpenSSL's
builtin list of groups. But that may have denial-of-service concerns
[1]?
- We should maybe look into SSL_CTX_config(), if we haven't discussed
that already on the list, but that's probably a bigger tangent and
doesn't need to be part of this patch.

Thanks,
--Jacob

[1] https://www.openssl.org/blog/blog/2022/10/21/tls-groups-configuration/index.html

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 10 Jun 2024 12:30:36 +0200

> On 7 Jun 2024, at 19:14, Jacob Champion <[email-masked]> wrote:

> make it easier for reviewers. (They can still share the same thread

+1, please do.

> while now, and we should probably use those if possible.

Agreed.  While not deprecated per se the curve API is considered obsolete and
is just aliased to the group API (OpenSSL using both the term obsolete and
deprecated to mean the same thing but with very different mechanics is quite
confusing).

> string to allow graceful fallbacks.

Parsing the list will likely risk false negatives as you say, but from skimming
the code there doesn't seem to be a good errormessage from SSL_set1_groups_list
to indicate if listitems were invalid (unless all of them were).  Maybe calling
the associated _get function to check the number of set groups can be used to
verify what happenend?

Regarding the ciphersuites portion of the patch.  I'm not particularly thrilled
about having a GUC for TLSv1.2 ciphers and one for TLSv1.3 ciphersuites, users
not all that familiar with TLS will likely find it confusing to figure out what
to do.

In which way is this feature needed since this can be achieved with the config
directive "Ciphersuites" in openssl.conf IIUC?

If we add this I think we should keep it blank and if so skip setting it at all
falling back on OpenSSL defaults.  The below default for the GUC does not match
the OpenSSL default and I think we are better off trusting them on this.

+	"TLS_AES_256_GCM_SHA384:TLS_AES_128_GCM_SHA256",

--
Daniel Gustafsson

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Wed, 12 Jun 2024 10:51:05 +0200

> not all that familiar with TLS will likely find it confusing to figure out what

I don't think it's easy to create a single GUC because OpenSSL has
different APIs for both. So we'd have to add some custom parsing for
the combined string, which is likely to cause some problems imho. I
think separating them is the best option from the options we have and
I don't think it matters much practice for users. Users not familiar
with TLS might indeed be confused, but those users shouldn't touch
these settings anyway, and just use the defaults. The users that care
about this probably already get two cipher strings from their
compliance teams, because many other applications also have two
separate options for specifying both.