> On 21 Sep 2021, at 02:06, Jacob Champion <pchamp...@vmware.com> wrote: > > On Mon, 2021-07-26 at 15:26 +0200, Daniel Gustafsson wrote: >>> On 19 Jul 2021, at 21:33, Jacob Champion <pchamp...@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",
Agreed. > 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. Maybe not, but doing so is at least in line with how the OpenSSL support will handle the same config AFAICT. Or am I missing something? >> + 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().) Skimming the NSS code I wasn't able find the countermeasures, can you provide a reference to where I should look? Feel free to post a new version of the NSS patch with these changes if you want. > 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. Please do. -- Daniel Gustafsson https://vmware.com/