Should SSL_get_servername() depend on SNI callback (no-)ACK?

[Benjamin Kaduk]

There's some (additional?) discussion on this topic in
https://github.com/openssl/openssl/pull/10018 .  A couple comments inline, though...

On Tue, Oct 22, 2019 at 02:30:37PM +0200, Yann Ylavic wrote:
> Hi,
> 
> in master (and 1.1.1), SSL_get_servername() returns either
> s->session->ext.hostname (when s->hit == 1), or s->ext.hostname
> (otherwise).
> 
> It seems, according to final_server_name(), that
> s->session->ext.hostname is set only:
>         if (sent && ret == SSL_TLSEXT_ERR_OK && (!s->hit || SSL_IS_TLS13(s))) {
>             ...
>             s->session->ext.hostname = OPENSSL_strdup(s->ext.hostname);
>             ...
>         }
> that is if a SNI callback exists and returns OK (no callback
> defaulting to NOACK).
> 
> And it _seems_ that browsers (or Chrome only?) don't set a
> tlsext_hostname in their session (ASN1) on resumption, so

I don't understand what you mean by "set a tlsext_hostname in their session (ASN1)
on resumption".  Are you saying that browsers do not send the server_name_indication
extension in the ClientHello for resumption handshakes?

> s->session->ext.hostname isn't set by that either. Note that this
> leaves s->servername_done = 0 in the below code from
> tls_parse_ctos_server_name():
>     if (s->hit) {
>         /*
>          * TODO(openssl-team): if the SNI doesn't match, we MUST
>          * fall back to a full handshake.
>          */
>         s->servername_done = (s->session->ext.hostname != NULL)
>             && PACKET_equal(&hostname, s->session->ext.hostname,
>                             strlen(s->session->ext.hostname));
> 
>         if (!s->servername_done && s->session->ext.hostname != NULL)
>             s->ext.early_data_ok = 0;
>     }
> 
> So it's possible that, on session resumption (s->hit == 1),
> SSL_get_servername() returns NULL (unset s->session->ext.hostname)
> even though an SNI is provided by ClientHello. Shouldn't it return the
> ClientHello's SNI regardless of the presence/return of the callback?
> Something like the below (where s->servername_done is also tested)?

There's something of a philosophical debate; some additional (heated)
discussion can be found in https://github.com/openssl/openssl/pull/7115
but I think the real cause of problems in this space is that
the SSL_get_servername() API contract has been underspecified from the
start.  Note that the documentation for SSL_get_servername() is in
the page for SSL_CTX_set_tlsext_servername_callback() and claims to
return the value from the Client Hello (if present).  Yet, historically,
prior to TLS 1.3 on resumption we did not even look at the ClientHello
to see whether the extension was present; we just resumed and thus the
"Client Hello" in the above would be the *initial* ClientHello.  This
was perhaps tolerable if the servername callback was never called, if
you assume that the API would only be called from that callback, but
that assumption is no longer anywhere close to true.

-Ben

> const char *SSL_get_servername(const SSL *s, const int type)
> {
>     if (type != TLSEXT_NAMETYPE_host_name)
>         return NULL;
> 
>     /*
>      * SNI is not negotiated in pre-TLS-1.3 resumption flows, so fake up an
>      * SNI value to return if we are resuming/resumed.  N.B. that we still
>      * call the relevant callbacks for such resumption flows, and callbacks
>      * might error out if there is not a SNI value available.
>      */
>     if (s->hit && s->servername_done)
>         return s->session->ext.hostname;
>     return s->ext.hostname;
> }
> 
> 
> Regards,
> Yann.