Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Subject: Re: Support for NSS as a libpq TLS backend
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <55c94351495efe34e167d4d2b1541b8194f1318c.camel@vmware.com>
Date: Mon, 27 Sep 2021 15:44:30 +0200
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
 "hlinnaka@iki.fi" <hlinnaka@iki.fi>,
 "andrew.dunstan@2ndquadrant.com" <andrew.dunstan@2ndquadrant.com>,
 "andres@anarazel.de" <andres@anarazel.de>,
 "thomas.munro@gmail.com" <thomas.munro@gmail.com>,
 "sfrost@snowman.net" <sfrost@snowman.net>,
 "michael@paquier.xyz" <michael@paquier.xyz>
Content-Transfer-Encoding: quoted-printable
Message-Id: <55454C3B-1BCB-4F08-AB93-FAF9100B6201@yesql.se>
References: <1b16041447d57b45ca273f41d26f6e298d756f99.camel@vmware.com>
 <04E81B50-ACE9-40D3-9C66-7041F969A958@yesql.se>
 <21FABD55-5F80-4D8B-B994-3DB81D8742EE@yesql.se>
 <20210321234950.GQ20766@tamriel.snowman.net>
 <F25D6A6A-8FCB-4188-B6E5-5587BABE4D3E@yesql.se>
 <743a11e6668bef648c7a53f862038e6b2fad3755.camel@vmware.com>
 <YFqHmspKASMHcH7V@paquier.xyz>
 <05da9d530ab01e6959bd50c993d590ad73575c21.camel@vmware.com>
 <20210324170036.GS20766@tamriel.snowman.net>
 <8f741a425fd135ca38a28063730715499f7f6445.camel@vmware.com>
 <20210324181016.GU20766@tamriel.snowman.net>
 <50c5a423329ea9996a2a2b167931711189ed5d1b.camel@vmware.com>
 <2FD5BB88-595A-47A0-8F51-0A1B5EEC93FA@yesql.se>
 <F1975F74-DC71-4036-99B4-CBB2D5085EB4@yesql.se>
 <4a15bbab42cd85b6bec83c2609305552d246f68e.camel@vmware.com>
 <8335133C-920E-438D-9CEB-DEF4C1533F73@yesql.se>
 <4ec146256e31afa0542f9fa970ec258c5f1a5f98.camel@vmware.com>
 <70A12E33-90B8-4761-8FA6-8DDB9EEA4D3E@yesql.se>
 <14b33300304507db6f4e4d8a77da105a8d18ea1a.camel@vmware.com>
 <FBBFDB5D-644C-45E7-B8F0-016068464B0C@yesql.se>
 <02AB4044-0823-4F77-B69B-18DB378C8743@yesql.se>
 <a0335cb2367c2763ea8da8c79feca80a74c30a07.camel@vmware.com>
 <A89F3823-40BD-4469-AFA8-819C20C0B24D@yesql.se>
 <55c94351495efe34e167d4d2b1541b8194f1318c.camel@vmware.com>
To: Jacob Champion <pchampion@vmware.com>
Archived-At: 
 <https://www.postgresql.org/message-id/55454C3B-1BCB-4F08-AB93-FAF9100B6201%40yesql.se>
Precedence: bulk

> On 21 Sep 2021, at 02:06, Jacob Champion <pchampion@vmware.com> wrote:
>=20
> 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.
>>=20
>> 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.
>=20
> I dug a bit deeper into the SNI stuff:
>=20
>> +	server_hostname =3D SSL_RevealURL(conn->pr_fd);
>> +	if (!server_hostname || server_hostname[0] =3D=3D '\0')
>> +	{
>> +		/* If SNI is enabled we must have a hostname set */
>> +		if (conn->sslsni && conn->sslsni[0])
>> +			status =3D SECFailure;
>=20
> 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=3D"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.") =3D=3D strlen(host) ||
>> +		  strchr(host, ':')))
>> +		SSL_SetURL(conn->pr_fd, host);
>=20
> 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/