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@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