Date: Tue, 27 Oct 2020 23:39:57 -0700
From: Andres Freund <andres@anarazel.de>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
	Michael Paquier <michael@paquier.xyz>,
	Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
	Stephen Frost <sfrost@snowman.net>,
	Thomas Munro <thomas.munro@gmail.com>
Subject: Re: Support for NSS as a libpq TLS backend
Message-ID: <20201028063957.cvln377jgao33ssu@alap3.anarazel.de>
References: 
 <CAA8=A7-suZg90M7Fgyo1Rfymkrpdh_Z547RKXrbHirsAmojQSw@mail.gmail.com>
 <20200904012334.GF19499@paquier.xyz>
 <20200917074134.GX2873@paquier.xyz>
 <BAD11CCA-E712-4906-8A27-94C2AA0E696E@yesql.se>
 <20200929055939.GF7117@paquier.xyz>
 <DE377D9A-BFFC-492C-A2B8-2AE6EBBBE740@yesql.se>
 <411593A7-E037-474D-BFD7-D3D6683C1D46@yesql.se>
 <B1ADEE76-3E33-4E9D-B830-3AA89B1C42D2@yesql.se>
 <20201020191529.5yverw3ybaube3pg@alap3.anarazel.de>
 <907FA7D1-9BD9-464B-A6EC-DEEB53438D36@yesql.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <907FA7D1-9BD9-464B-A6EC-DEEB53438D36@yesql.se>
Precedence: bulk

Hi,

On 2020-10-27 21:07:01 +0100, Daniel Gustafsson wrote:
> > On 2020-10-20 14:24:24 +0200, Daniel Gustafsson wrote:
> >> From 0cb0e6a0ce9adb18bc9d212bd03e4e09fa452972 Mon Sep 17 00:00:00 2001
> >> From: Daniel Gustafsson <daniel@yesql.se>
> >> Date: Thu, 8 Oct 2020 18:44:28 +0200
> >> Subject: [PATCH] Support for NSS as a TLS backend v12
> >> ---
> >> configure                                     |  223 +++-
> >> configure.ac                                  |   39 +-
> >> contrib/Makefile                              |    2 +-
> >> contrib/pgcrypto/Makefile                     |    5 +
> >> contrib/pgcrypto/nss.c                        |  773 +++++++++++
> >> contrib/pgcrypto/openssl.c                    |    2 +-
> >> contrib/pgcrypto/px.c                         |    1 +
> >> contrib/pgcrypto/px.h                         |    1 +
> > 
> > Personally I'd like to see this patch broken up a bit - it's quite
> > large. Several of the changes could easily be committed separately, no?
> 
> Not sure how much of this makes sense committed separately (unless separately
> means in quick succession), but it could certainly be broken up for the sake of
> making review easier.

Committing e.g. the pgcrypto pieces separately from the backend code
seems unproblematic. But yes, I would expect them to go in close to each
other. I'm mainly concerned with smaller review-able units.

Have you done testing to ensure that NSS PG cooperates correctly with
openssl PG? Is there a way we can make that easier to do? E.g. allowing
to build frontend with NSS and backend with openssl and vice versa?


> >> if test "$with_openssl" = yes ; then
> >> +  if test x"$with_nss" = x"yes" ; then
> >> +    AC_MSG_ERROR([multiple SSL backends cannot be enabled simultaneously"])
> >> +  fi
> > 
> > Based on a quick look there's no similar error check for the msvc
> > build. Should there be?
> 
> Thats a good question.  When embarking on this is seemed quite natural to me
> that it should be, but now I'm not so sure.  Maybe there should be a
> --with-openssl-preferred like how we handle readline/libedit or just allow
> multiple and let the last one win?  Do you have any input on what would make
> sense?
>
> The only thing I think makes no sense is to allow multiple ones at the same
> time given the current autoconf switches, even if it would just be to pick say
> pg_strong_random from one and libpq TLS from another.

Maybe we should just have --with-ssl={openssl,nss}? That'd avoid needing
to check for errors.

Even better, of course, would be to allow switching of the SSL backend
based on config options (PGC_POSTMASTER GUC for backend, connection
string for frontend). Mainly because that would make testing of
interoperability so much easier.  Obviously still a few places like
pgcrypto, randomness, etc, where only a compile time decision seems to
make sense.


> >> +  CLEANLDFLAGS="$LDFLAGS"
> >> +  # TODO: document this set of LDFLAGS
> >> +  LDFLAGS="-lssl3 -lsmime3 -lnss3 -lplds4 -lplc4 -lnspr4 $LDFLAGS"
> > 
> > Shouldn't this use nss-config or such?
> 
> Indeed it should, where available.  I've added rudimentary support for that
> without a fallback as of now.

When would we need a fallback?


> > I think it'd also be better if we could include these files as nss/ssl.h
> > etc - ssl.h is a name way too likely to conflict imo.
> 
> I've changed this to be nss/ssl.h and nspr/nspr.h etc, but the include path
> will still need the direct path to the headers (from autoconf) since nss.h
> includes NSPR headers as #include <nspr.h> and so on.

Hm. Then it's probably not worth going there...


> >> +static SECStatus
> >> +pg_cert_auth_handler(void *arg, PRFileDesc * fd, PRBool checksig, PRBool isServer)
> >> +{
> >> +	SECStatus	status;
> >> +	Port	   *port = (Port *) arg;
> >> +	CERTCertificate *cert;
> >> +	char	   *peer_cn;
> >> +	int			len;
> >> +
> >> +	status = SSL_AuthCertificate(CERT_GetDefaultCertDB(), port->pr_fd, checksig, PR_TRUE);
> >> +	if (status == SECSuccess)
> >> +	{
> >> +		cert = SSL_PeerCertificate(port->pr_fd);
> >> +		len = strlen(cert->subjectName);
> >> +		peer_cn = MemoryContextAllocZero(TopMemoryContext, len + 1);
> >> +		if (strncmp(cert->subjectName, "CN=", 3) == 0)
> >> +			strlcpy(peer_cn, cert->subjectName + strlen("CN="), len + 1);
> >> +		else
> >> +			strlcpy(peer_cn, cert->subjectName, len + 1);
> >> +		CERT_DestroyCertificate(cert);
> >> +
> >> +		port->peer_cn = peer_cn;
> >> +		port->peer_cert_valid = true;
> > 
> > Hm. We either should have something similar to
> > 
> > 			/*
> > 			 * Reject embedded NULLs in certificate common name to prevent
> > 			 * attacks like CVE-2009-4034.
> > 			 */
> > 			if (len != strlen(peer_cn))
> > 			{
> > 				ereport(COMMERROR,
> > 						(errcode(ERRCODE_PROTOCOL_VIOLATION),
> > 						 errmsg("SSL certificate's common name contains embedded null")));
> > 				pfree(peer_cn);
> > 				return -1;
> > 			}
> > here, or a comment explaining why not.
> 
> We should, but it's proving rather difficult as there is no equivalent API call
> to get the string as well as the expected length of it.

Hm. Should at least have a test to ensure that's not a problem then. I
hope/assume NSS rejects this somewhere internally...


> > Also, what's up with the CN= bit? Why is that needed here, but not for
> > openssl?
> 
> OpenSSL returns only the value portion, whereas NSS returns key=value so we
> need to skip over the key= part.

Why is it a conditional path though?


> >> +/*
> >> + * PR_ImportTCPSocket() is a private API, but very widely used, as it's the
> >> + * only way to make NSS use an already set up POSIX file descriptor rather
> >> + * than opening one itself. To quote the NSS documentation:
> >> + *
> >> + *		"In theory, code that uses PR_ImportTCPSocket may break when NSPR's
> >> + *		implementation changes. In practice, this is unlikely to happen because
> >> + *		NSPR's implementation has been stable for years and because of NSPR's
> >> + *		strong commitment to backward compatibility."
> >> + *
> >> + * https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/PR_ImportTCPSocket
> >> + *
> >> + * The function is declared in <private/pprio.h>, but as it is a header marked
> >> + * private we declare it here rather than including it.
> >> + */
> >> +NSPR_API(PRFileDesc *) PR_ImportTCPSocket(int);
> > 
> > Ugh. This is really the way to do this? How do other applications deal
> > with this problem?
> 
> They either #include <private/pprio.h> or they do it like this (or vendor NSPR
> which makes calling private APIs less problematic).  It sure is ugly, but there
> is no alternative to using this function.

Hm - in debian unstable's NSS this function appears to be in nss/ssl.h,
not pprio.h:

/*
** Imports fd into SSL, returning a new socket.  Copies SSL configuration
** from model.
*/
SSL_IMPORT PRFileDesc *SSL_ImportFD(PRFileDesc *model, PRFileDesc *fd);

and ssl.h starts with:
/*
 * This file contains prototypes for the public SSL functions.


> >> +
> >> +	PK11_SetPasswordFunc(PQssl_passwd_cb);
> > 
> > Is it actually OK to do stuff like this when other users of NSS might be
> > present? That's obviously more likely in the libpq case, compared to the
> > backend case (where it's also possible, of course). What prevents us
> > from overriding another user's callback?
> 
> The password callback pointer is stored in a static variable in NSS (in the
> file lib/pk11wrap/pk11auth.c).

But, uh, how is that not a problem? What happens if a backend imports
libpq? What if plpython imports curl which then also uses nss?


> +	/*
> +	 * Finally we must configure the socket for being a server by setting the
> +	 * certificate and key.
> +	 */
> +	status = SSL_ConfigSecureServer(model, server_cert, private_key, kt_rsa);
> +	if (status != SECSuccess)
> +		ereport(ERROR,
> +				(errmsg("unable to configure secure server: %s",
> +						pg_SSLerrmessage(PR_GetError()))));
> +	status = SSL_ConfigServerCert(model, server_cert, private_key, NULL, 0);
> +	if (status != SECSuccess)
> +		ereport(ERROR,
> +				(errmsg("unable to configure server for TLS server connections: %s",
> +						pg_SSLerrmessage(PR_GetError()))));

Why do both of these need to get called? The NSS docs say:

/*
** Deprecated variant of SSL_ConfigServerCert.
**
...
SSL_IMPORT SECStatus SSL_ConfigSecureServer(
    PRFileDesc *fd, CERTCertificate *cert,
    SECKEYPrivateKey *key, SSLKEAType kea);


Greetings,

Andres Freund