public inbox for [email protected]  

help / color / mirror / Atom feed

From: Daniel Gustafsson <[email protected]>
To: Jacob Champion <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Support for NSS as a libpq TLS backend
Date: Mon, 27 Sep 2021 15:44:30 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

> On 21 Sep 2021, at 02:06, Jacob Champion <[email protected]> wrote:
> 
> On Mon, 2021-07-26 at 15:26 +0200, Daniel Gustafsson wrote:
>>> On 19 Jul 2021, at 21:33, Jacob Champion <[email protected]> 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/

view thread (181+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Support for NSS as a libpq TLS backend
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox