On Mon, 2021-07-26 at 15:26 +0200, Daniel Gustafsson wrote:
> > On 19 Jul 2021, at 21:33, Jacob Champion <pchampion@vmware.com> wrote:
> > ..client connections will crash if
> > hostaddr is provided rather than host, because SSL_SetURL can't handle
> > a NULL argument. I'm running with 0002 to fix it for the moment, but
> > I'm not sure yet if it does the right thing for IP addresses, which the
> > OpenSSL side has a special case for.
>
> AFAICT the idea is to handle it in the cert auth callback, so I've added some
> PoC code to check for sslsni there and updated the TODO comment to reflect
> that.
I dug a bit deeper into the SNI stuff:
> + server_hostname = SSL_RevealURL(conn->pr_fd);
> + if (!server_hostname || server_hostname[0] == '\0')
> + {
> + /* If SNI is enabled we must have a hostname set */
> + if (conn->sslsni && conn->sslsni[0])
> + status = SECFailure;
conn->sslsni can be explicitly set to "0" to disable it, so this should
probably be changed to a check for "1", but I'm not sure that would be
correct either. If the user has the default sslsni="1" and supplies an
IP address for the host parameter, I don't think we should fail the
connection.
> + if (host && host[0] &&
> + !(strspn(host, "0123456789.") == strlen(host) ||
> + strchr(host, ':')))
> + SSL_SetURL(conn->pr_fd, host);
It looks like NSS may already have some code that prevents SNI from
being sent for IP addresses, so that part of the guard might not be
necessary. (And potentially counterproductive, because it looks like
NSS can perform verification against the certificate's SANs if you pass
an IP address to SSL_SetURL().)
Speaking of IP addresses in SANs, it doesn't look like our OpenSSL
backend can handle those. That's a separate conversation, but I might
take a look at a patch for next commitfest.
--Jacob