tls_construct_server_key_exchange:internal error / tls_process_server_certificate:length mismatch

[james-callahan]

We're running into an issue in OpenSSL 1.1.1+ where we're trying to change the key/cert based on SNI and ALPN (the code worked with previous releases). For compat with earlier OpenSSL versions we're using SSL_set_SSL_CTX. The issue only seems to happen when the original context is not TLS 1.3. If the server uses TLS1.3 then everything seems to work out....

The server side reports:

starttls: error:141EC044:SSL routines:tls_construct_server_key_exchange:internal error

Client side reports:

140620744139200:error:1416F09F:SSL routines:tls_process_server_certificate:length mismatch:../ssl/statem/statem_clnt.c:1848:

Test case here: https://gist.github.com/james-callahan/7f021533ad20ed107938885f2304b43e#file-test-server-lua

[davidben]

Can you output the full error data (line number) on the server? There're multiple internal error paths in tls_construct_server_key_exchange. The client is just complaining about bad syntax. The messages don't quite match, which is odd, but if the server tripped over itself producing an adjacent message, that's probably the root problem.

[james-callahan]

Can you output the full error data (line number) on the server?

I'm not sure why, but there is no line number printed.

[james-callahan]

I'm not sure why, but there is no line number printed.

Got it: ../ssl/statem/statem_srvr.c:2747

[mattcaswell]

That corresponds to this code:

openssl/ssl/statem/statem_srvr.c

Lines 2736 to 2749 in 1708e3e

	if (lu != NULL) {
	EVP_PKEY *pkey = s->s3->tmp.cert->privatekey;
	const EVP_MD *md;
	unsigned char *sigbytes1, *sigbytes2, *tbs;
	size_t siglen, tbslen;
	int rv;
	if (pkey == NULL || !tls1_lookup_md(lu, &md)) {
	/* Should never happen */
	SSLfatal(s, SSL_AD_INTERNAL_ERROR,
	SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
	ERR_R_INTERNAL_ERROR);
	goto err;
	}

This is a "should never happen" error. My guess is s->s3->tmp.cert->privatekey has ended up being NULL due to something SNI related when it shouldn't be.

If I understand what you wrote you are changing the key/cert by using SSL_set_SSL_CTX. What callback are you using to do that?

Based on the line number for this error it seems you are using 1.1.1 rather than 1.1.1a? It might be worth trying 1.1.1a as there have been numerous defect fixes - it would be good to rule those out as a cause for this.

[james-callahan]

If I understand what you wrote you are changing the key/cert by using SSL_set_SSL_CTX. What callback are you using to do that?

ALPN via SSL_CTX_set_alpn_select_cb (please see the test case in the initial report)

Based on the line number for this error it seems you are using 1.1.1 rather than 1.1.1a? It might be worth trying 1.1.1a as there have been numerous defect fixes - it would be good to rule those out as a cause for this.

Will try when I'm at work on monday

[james-callahan]

1.1.1a has the same error. It now occurs at ../ssl/statem/statem_srvr.c:2751

[james-callahan]

I've updated the earlier gist with a pure-C reproduction of the issue: https://gist.github.com/james-callahan/7f021533ad20ed107938885f2304b43e#file-test-c

[mattcaswell]

ALPN via SSL_CTX_set_alpn_select_cb (please see the test case in the initial report)

Oh, sorry - somehow missed that. Does this patch help?

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index e7c11c4bea..791c030fb8 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -2258,6 +2258,17 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
                     goto err;
                 }
                 s->s3->tmp.new_cipher = cipher;
+
+                /*
+                 * Call alpn_select callback if needed.  Has to be done after SNI and
+                 * cipher negotiation (HTTP/2 restricts permitted ciphers). In TLSv1.3
+                 * we already did this because cipher negotiation happens earlier, and
+                 * we must handle ALPN before we decide whether to accept early_data.
+                 */
+                if (!tls_handle_alpn(s)) {
+                    /* SSLfatal() already called */
+                    goto err;
+                }
             }
             if (!s->hit) {
                 if (!tls_choose_sigalg(s, 1)) {
@@ -2277,6 +2288,10 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
         } else {
             /* Session-id reuse */
             s->s3->tmp.new_cipher = s->session->cipher;
+            if (!SSL_IS_TLS13(s) && !tls_handle_alpn(s)) {
+                /* SSLfatal() already called */
+                goto err;
+            }
         }
 
         /*-
@@ -2299,16 +2314,6 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
             /* SSLfatal() already called */
             goto err;
         }
-        /*
-         * Call alpn_select callback if needed.  Has to be done after SNI and
-         * cipher negotiation (HTTP/2 restricts permitted ciphers). In TLSv1.3
-         * we already did this because cipher negotiation happens earlier, and
-         * we must handle ALPN before we decide whether to accept early_data.
-         */
-        if (!SSL_IS_TLS13(s) && !tls_handle_alpn(s)) {
-            /* SSLfatal() already called */
-            goto err;
-        }
 
         wst = WORK_MORE_C;
     }

[james-callahan]

Does this patch help?

The test case (linked above) now fails with error:14201076:SSL routines:tls_choose_sigalg:no suitable signature algorithm from ssl/t1_lib.c:2716

[james-callahan]

I think this would be the right fix? master...james-callahan:7660-alpn_select-cert-change

I'd PR it but I'm having trouble writing a test for it.

[james-callahan]

@mattcaswell ping? does the link above look correct?
Do you have any advice for writing a test?

[mattcaswell]

It looks correct to me. As for a test I think something should be added to sslapitest. We do already have a test there which calls the alpn callback but it is specific to early data with PSKs.

openssl/test/sslapitest.c

Lines 2751 to 2772 in 871493a

	static int alpn_select_cb(SSL *ssl, const unsigned char **out,
	unsigned char *outlen, const unsigned char *in,
	unsigned int inlen, void *arg)
	{
	unsigned int protlen = 0;
	const unsigned char *prot;
	for (prot = in; prot < in + inlen; prot += protlen) {
	protlen = *prot++;
	if (in + inlen < prot + protlen)
	return SSL_TLSEXT_ERR_NOACK;
	if (protlen == strlen(servalpn)
	&& memcmp(prot, servalpn, protlen) == 0) {
	*out = prot;
	*outlen = protlen;
	return SSL_TLSEXT_ERR_OK;
	}
	}
	return SSL_TLSEXT_ERR_NOACK;
	}

Probably we need a new "test_alpn" test in that file. I'd recommend you take a look at how some of the other tests are written in that file and see if you can base something on that.

[glyph]

I bumped into this as well, while trying to write a Python server to do something similar. I removed my SNI and info callbacks, and still ended up getting the same error. Just an ALPN callback that calls SSL_set_SSL_CTX.

[davidben]

(@glyph and I already chatted out-of-band, but for posterity.)

Calling SSL_set_SSL_CTX in the ALPN callback is kinda iffy because that happens after parameter negotiation. HTTP/2's opinions on TLS cipher suites mean you want those resolved first, so you can take that into account. But if you've resolve cipher suites, you're past the point of no return w.r.t. changing certificates and such.

The ClientHello callback is probably best if you want to treat ALPN as a funny SNI.

[james-callahan]

The ClientHello callback is probably best if you want to treat ALPN as a funny SNI.

As noted in my original post: we are trying to be consistent across OpenSSL 1.0.2 and OpenSSL 1.1.1.
1.0.2 did not have a client hello callback.

[t8m]

The version 1.1.1 is not supported anymore. Closing.