fix: check whether a protocol is enabled during the length calculation in create_npn_advertisement

[yknoya]

Problem

If the following conditions are met, an issue occurs where ATS sends an invalid Server Hello during the TLS handshake:

• Next Protocol Negotiation (NPN) extension is used.
• HTTP/2 is disabled in sni.yaml.

How to reproduce

1. Add the following remap to the remap.config:

map https://cdn.example.com/ http://example.com

2. Add the following configuration to the sni.yaml:

sni:
  - fqdn: cdn.example.com
    http2: off

3. Launch Traffic Server.
4. Execute the following command:

openssl s_client \
  -connect localhost:443 \
  -servername cdn.example.com \
  -tls1_2 \
  -nextprotoneg http/2,http/1.1 \
  -CAfile /etc/pki/tls/certs/ca-bundle.crt \
  </dev/null \
| grep error

If reproduced, the following error will be displayed:

140451424151360:error:1423506E:SSL routines:ssl_next_proto_validate:bad extension:ssl/statem/extensions_clnt.c:1565:

Cause

The issue is caused by ATS setting a length of the NPN string greater than the actual length in the NPN extension.
For example, when HTTP/2 is disabled in sni.yaml, the NPN string should be 8http/1.18http/1.0 and its length should be 18, but ATS sets the length as 21.

Here are some pointers to the relevant bits of code:

The ssl_next_protos_advertised_callback function is responsible for setting the NPN string and its length.

trafficserver/iocore/net/SSLUtils.cc

Lines 1611 to 1616 in adac616

	bool
	SSLMultiCertConfigLoader::_set_npn_callback(SSL_CTX *ctx)
	{
	SSL_CTX_set_next_protos_advertised_cb(ctx, ssl_next_protos_advertised_callback, nullptr);
	return true;
	}

The NPN string is stored in the ALPNSupport::npn, and its length is stored in the ALPNSupport::npnsz.

trafficserver/iocore/net/SSLUtils.cc

Lines 479 to 490 in adac616

	int
	ssl_next_protos_advertised_callback(SSL *ssl, const unsigned char **out, unsigned *outlen, void *)
	{
	ALPNSupport *alpns = ALPNSupport::getInstance(ssl);
	ink_assert(alpns);
	if (alpns) {
	return alpns->advertise_next_protocol(ssl, out, outlen);
	}
	return SSL_TLSEXT_ERR_NOACK;
	}

trafficserver/iocore/net/ALPNSupport.cc

Lines 84 to 87 in adac616

	int
	ALPNSupport::advertise_next_protocol(SSL *ssl, const unsigned char **out, unsigned *outlen)
	{
	if (this->getNPN(out, outlen)) {

trafficserver/iocore/net/P_ALPNSupport.h

Lines 58 to 63 in adac616

	bool
	getNPN(const unsigned char **out, unsigned int *outlen) const
	{
	if (this->npn && this->npnsz) {
	*out = this->npn;
	*outlen = this->npnsz;

The values of ALPNSupport::npn and ALPNSupport::npnsz are set in the SSLNextProtocolSet::create_npn_advertisement function.

trafficserver/iocore/net/ALPNSupport.cc

Line 126 in adac616

npnSet->create_npn_advertisement(protoenabled, &npn, &npnsz);

In SSLNextProtocolSet::create_npn_advertisement, it is checked whether each protocol is enabled when setting the value for ALPNSupport::npn.

trafficserver/iocore/net/SSLNextProtocolSet.cc

Lines 66 to 71 in adac616

	for (ep = endpoints.head; ep != nullptr; ep = endpoints.next(ep)) {
	if (enabled.contains(globalSessionProtocolNameRegistry.toIndex(swoc::TextView{ep->protocol, strlen(ep->protocol)}))) {
	Debug("ssl", "advertising protocol %s, %p", ep->protocol, ep->endpoint);
	advertised = append_protocol(ep->protocol, advertised);
	}
	}

However, when setting the value for ALPNSupport::npnsz, it isn't checked whether each protocol is enabled.

trafficserver/iocore/net/SSLNextProtocolSet.cc

Lines 56 to 59 in adac616

	for (ep = endpoints.head; ep != nullptr; ep = endpoints.next(ep)) {
	ink_release_assert((strlen(ep->protocol) > 0));
	*len += (strlen(ep->protocol) + 1);
	}

As a result, when HTTP/2 is disabled in sni.yaml, the value excluding HTTP/2 is set for ALPNSupport::npn, but the value including HTTP/2 is set for ALPNSupport::npnsz.
Therefore, the length of the NPN string is greater than the actual length.

[masaori335]

Looks reasonable. The length should be incremented only if it's appended.