Re: Support for NSS as a libpq TLS backend

> On 21 Sep 2021, at 02:06, Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Mon, 2021-07-26 at 15:26 +0200, Daniel Gustafsson wrote:
>>> On 19 Jul 2021, at 21:33, Jacob Champion <pchampion(at)vmware(dot)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/