nginx 1.19.4 feature "ssl_reject_handshake" does not work as intended with openssl

[i81b4u]

Nginx 1.19.4 introduced a new feature called "ssl_reject_handshake" which can be used to block unwanted SSL handshakes. I noticed that, when enabled, it effectively turns off TLSv1.3. I opened the following ticket: https://trac.nginx.org/nginx/ticket/2071

It seems like the problem arises due to the way openssl handles requests, because (and I quote): "since it checks for supported protocol versions before switching context, and nginx can't influence it".

Can you confirm the findings as stated by the nginx engineer and if so, would you please consider fixing it in a future openssl release?

[i81b4u]

In the mean time I recompiled nginx with boringssl and can confirm that "ssl_reject_handshake" now works as intended.

[mattcaswell]

"since it checks for supported protocol versions before switching context, and nginx can't influence it".

The server name callback occurs later on in the ClientHello processing after certain extensions and decisions have already been taken. Changing this is probably not feasible for backwards compatibility reasons (something boringssl does not worry too much about).

If very early context switching is required (i.e. before protocol negotiation) then a ClientHello callback can be used instead:

https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_client_hello_cb.html

This would give nginx to ability to have the desired influence.

[mdounin]

If very early context switching is required (i.e. before protocol negotiation) then a ClientHello callback can be used instead:

It cannot be used. Most notably, because server name requested is not known during ClientHello callback, and making it known depends on decisions made by the OpenSSL library after the callback is called. For example, if the only available protocol on the server side is SSLv3, server name from the ClientHello must not be used at all, yet this is generally not known to the application. Not to mention it is from hard to impossible to correctly extract server name in a portable way from a ClientHello, especially taking eSNI / ECH into account.

Rather, I would say it is simply a bug to assume TLSv1.3 cannot (or can) be used based on the certificates set in the SSL_CTX before the servername callback is called, which is specifically designed to configure servername-specific certificates.

[mattcaswell]

It cannot be used. Most notably, because server name requested is not known during ClientHello callback, and making it known depends on decisions made by the OpenSSL library after the callback is called.

Well this seems like a catch-22. You want the server name call back to be called later in the process after certain decisions have been made....but you also want it before those decisions are made in order to influence them :-)

For example, if the only available protocol on the server side is SSLv3, server name from the ClientHello must not be used at all, yet this is generally not known to the application. Not to mention it is from hard to impossible to correctly extract server name in a portable way from a ClientHello, especially taking eSNI / ECH into account.

Well eSNI/ECH is not yet supported anyway so that shouldn't currently be an issue (although could become one). I'd be interested in the thoughts of @tmshort and @kaduk on the above since the client hello cb was designed specifically with the server name extension in mind.

Rather, I would say it is simply a bug to assume TLSv1.3 cannot (or can) be used based on the certificates set in the SSL_CTX before the servername callback is called, which is specifically designed to configure servername-specific certificates.

I suppose is_tls13_capable could just return 1 if a servername cb has been set (it already does that if a PSK cb has been set).

[mdounin]

Well this seems like a catch-22. You want the server name call back to be called later in the process after certain decisions have been made....but you also want it before those decisions are made in order to influence them :-)

Not really. In this particular case I simply do not want some decisions to be made before the servername callback is called, as they are clearly made out of incorrect assumptions.

In more generic use case, as discussed as a part of servername-specific list of supported protocols[1], OpenSSL can maintain a set of available protocols, and only choose a specific protocol once the servername callback is called. It is understood that this implies a lot of burden for the library though.

Well eSNI/ECH is not yet supported anyway so that shouldn't currently be an issue (although could become one). I'd be interested in the thoughts of @tmshort and @kaduk on the above since the client hello cb was designed specifically with the server name extension in mind.

For the record, previous attempts to use ClientHello callback in nginx can be found here. The resulting code is overcomplicated even assuming OpenSSL-only implementation, and have obvious drawbacks of not being compatible with SSLv3 and eSNI/ECH.

I suppose is_tls13_capable could just return 1 if a servername cb has been set (it already does that if a PSK cb has been set).

Yes, this should fix the issue observed by the reporter.

[mattcaswell]

Fix for master in #13304, and fix for 1.1.1 in #13305.

[i81b4u]

Implemented the fix and after testing, I can confirm that nginx 1.19.4 feature "ssl_reject_handshake" now also can be used with TLSv1.3.

[kaduk]

[clienthello callback] cannot be used. Most notably, because server name requested is not known during ClientHello callback, and making it known depends on decisions made by the OpenSSL library after the callback is called.

This is not true. The server name requested is absolutely available during the clienthello callback, subjected to the documented caveats of the clienthello callback. The fact that with the currently available APIs you end up parsing a decent amount of TLS procotol in the application does not make the information "not known". In fact, the code to do so without relying on library internals has been available since the clienthello callback was introduced (

openssl/test/handshake_helper.c

Lines 158 to 187 in 3ee3c4d

	/*
	* The server_name extension was given too much extensibility when it
	* was written, so parsing the normal case is a bit complex.
	*/
	if (!SSL_client_hello_get0_ext(s, TLSEXT_TYPE_server_name, &p,
	&remaining) ||
	remaining <= 2)
	return 0;
	/* Extract the length of the supplied list of names. */
	len = (*(p++) << 8);
	len += *(p++);
	if (len + 2 != remaining)
	return 0;
	remaining = len;
	/*
	* The list in practice only has a single element, so we only consider
	* the first one.
	*/
	if (remaining == 0 || *p++ != TLSEXT_NAMETYPE_host_name)
	return 0;
	remaining--;
	/* Now we can finally pull out the byte array with the actual hostname. */
	if (remaining <= 2)
	return 0;
	len = (*(p++) << 8);
	len += *(p++);
	if (len + 2 > remaining)
	return 0;
	remaining = len;
	servername = (const char *)p;

), and we would quite eagerly accept a dedicated API to retrieve the server name during the early callback. I have some intention to write such a thing myself, but have not yet had the time to do so.

For example, if the only available protocol on the server side is SSLv3, server name from the ClientHello must not be used at all, yet this is generally not known to the application.

First off, I'm not entirely sure I understand the scenario here where the application might not know that only SSLv3 is available. Second, why is SSLv3 even a consideration for new code? SSLv3 is as dead as it can be officially via the IETF, and I don't know of any reason to use it when more modern versions of TLS are available. SSLv3 should be considered legacy, and it may well be appropriate to require dedicated compatibility servers to interact with legacy SSLv3-only clients.

Not to mention it is from hard to impossible to correctly extract server name in a portable way from a ClientHello, especially taking eSNI / ECH into account.

As was already noted, it seems specious to make presumptions about what APIs will be used to expose features that are not yet implemented (or even have a finalized specification). ECH has not even reached a point where the TLS WG has sent it to me for AD evaluation, so has a significant potential for future design changes.

Rather, I would say it is simply a bug to assume TLSv1.3 cannot (or can) be used based on the certificates set in the SSL_CTX before the servername callback is called, which is specifically designed to configure servername-specific certificates.

I disagree here as well. The design of TLS is such that the first operation performed at a protocol level when processing a ClientHello is to determine what version of TLS is in use. All other decisions and processing of the ClientHello will then be dependent on the selected protocol version. If the application needs to influence what versions of TLS are permitted, then that application logic needs to run very early in the protocol processing; the mechanism that openssl provides for doing so is the clienthello callback. The legacy servername_cb simply was not designed with influencing TLS versions in mind, and what configuration it allowed the application to change is effectively just an artifact of its initial implementation. We have, in fact, been stuck adding more and more callbacks over the years (we have cert callback and client cert callback, for example, in addition to servername callback) since the initial semantics were so under-specified.

Not really. In this particular case I simply do not want some decisions to be made before the servername callback is called, as they are clearly made out of incorrect assumptions.

Sorry, what decisions, and what assumptions are incorrect?

In more generic use case, as discussed as a part of servername-specific list of supported protocols[1], OpenSSL can maintain a set of available protocols, and only choose a specific protocol once the servername callback is called. It is understood that this implies a lot of burden for the library though.

"a lot of burden" is an understatement; we would need to essentially process with all protocols in parallel and the code is already quite complex.

For the record, previous attempts to use ClientHello callback in nginx can be found here. The resulting code is overcomplicated even assuming OpenSSL-only implementation, and have obvious drawbacks of not being compatible with SSLv3 and eSNI/ECH.

I'm afraid I don't fully understand your objections here. As I said earlier, we'd be happy to abstract much of the complexity into an openssl API, but someone has to write the code to do it, and the relevant SSLv3 and ECH scenarios remain unclear to me based only on what has been described here.

[mdounin]

The fact that with the currently available APIs you end up parsing a decent amount of TLS procotol in the application does not make the information "not known".

Unless the application knows if the only available protocol is SSLv3 or not, it does not know if "parsing a decent amount of TLS procotol" will end up in a valid server name which should be used, or a garbage which should not be used. The decision which protocol to use is currently on OpenSSL.

And yes, with "the currently available APIs" you end up with the code which effectively asks for re-implementation of the library. Moreover, any relevant protocol additions will make the code useless. While this might be a good enough from library point of view, this is not how application code is generally written: instead, the generic idea is to rely on library to hide protocol details.

Given the proper API to set servername-specific configuration exists for years and portable across multiple libraries, it is really a better idea to improve this existing API rather than trying to insist that ClientHello callback should be used instead.

First off, I'm not entirely sure I understand the scenario here where the application might not know that only SSLv3 is available.

There are many reasons why particular protocols might be available or not, including: the library defaults (and protocols supported by the library), library default configuration applied during SSL_CTX creation, SSL_CONF_cmd() commands configured directly by the user. Not to mention the SSL_OP_NO_* options, SSL_CTX_set_min_proto_version() and SSL_CTX_set_max_proto_version() set by the application itself. Also, there are various surprising "we've decided to switch of TLSv1.3 silently because there are no certificates in the SSL_CTX" things as discussed in this issue.

While a sophisticated enough application might try hard to follow all these, and might succeed in most cases, this is not going to be trivial. Further, likely there will be edge cases where it will fail.

Second, why is SSLv3 even a consideration for new code? SSLv3 is as dead as it can be officially via the IETF, and I don't know of any reason to use it when more modern versions of TLS are available. SSLv3 should be considered legacy, and it may well be appropriate to require dedicated compatibility servers to interact with legacy SSLv3-only clients.

While it is understood that SSLv3 is legacy now, in practice it is still used in various legacy setups. And it is a good example how ClientHello callback fails to abstract SSL/TLS protocol details from the application, resulting in incorrect behaviour. Another example is eSNI / ECH.

As was already noted, it seems specious to make presumptions about what APIs will be used to expose features that are not yet implemented (or even have a finalized specification). ECH has not even reached a point where the TLS WG has sent it to me for AD evaluation, so has a significant potential for future design changes.

Yet it is clear that parsing server name in ClientHello callback won't work without major changes. On the other hand, the servername callback can easily continue to work without any modifications on the application side (except may be for loading appropriate keys).

I disagree here as well.

The servername callback was specifically designed to set servername-specific certificates. Current OpenSSL code to disable TLSv1.3 based on "bad" certificates assumes certificates are not changed by the servername callback. These are two facts, nothing to disagree with. The only options here are: the servername callback is buggy and cannot be used to provide servername-specific certificates or the code which disables TLSv1.3 before calling it is buggy.

As I tried to explain above, deprecating the servername callback as buggy is not a wise decision from the application point of view.

since the initial semantics were so under-specified

From the application point of view the semantic is clear enough: it is called once the server name is available via SSL_get_servername(), and can be used to change configuration to match the requested name, most notably to change certificates. While this semantic might be indeed under-specified, it is believed to be clear enough to be implementable without breaking POLA.

Sorry, what decisions, and what assumptions are incorrect?

In this particular issue the wrong decision is to disable TLSv1.3. This decision is made before the servername callback is called out of the incorrect assumption that certificates will not be changed by the servername callback.

[mattcaswell]

Reopening this. It was auto-closed by the merge of ebda646 which fixes this issue in master. However, #13305 which fixes it in 1.1.1 has yet to be merged.

[davidben]

Not to mention it is from hard to impossible to correctly extract server name in a portable way from a ClientHello, especially taking eSNI / ECH into account.

FWIW, we're envisioning for BoringSSL that we'd resolve ECH before the callback and just pass the inner ClientHello to callback, since that matches what most folks want. And then if we come across any need for exposing the outer ClientHello, we might explore that. But since the outer ClientHello is largely ignored once you've decided to use the inner one, I don't expect there to be a need here.

[mattcaswell]

The 1.1.1 version of the fix (from #13305) has now also been merged, so closing this.

[i81b4u]

To all who were involved: Thank you very much!