Re: Support for NSS as a libpq TLS backend

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(at)yesql(dot)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