Re: Support for NSS as a libpq TLS backend

On 10/29/20 11:20 AM, Daniel Gustafsson wrote:
>> On 28 Oct 2020, at 07:39, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> 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?
> When I wrote the Secure Transport patch I had a patch against PostgresNode
> which allowed for overriding the server binaries like so:
>
> SSLTEST_SERVER_BIN=/path/bin/ make -C src/test/ssl/ check
>
> I've used that coupled with manual testing so far to make sure that an openssl
> client can talk to an NSS backend and so on. Before any other backend is added
> we clearly need *a* way of doing this, one which no doubt will need to be
> improved upon to suit more workflows.
>
> This is sort of the same situation as pg_upgrade, where two trees is needed to
> really test it.
>
> I can clean that patch up and post as a starting point for discussions.
>
>>>>> 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.
> Thats another option, with --with-openssl being an alias for --with-ssl=openssl.
>
> After another round of thinking I like this even better as it makes the build
> infra cleaner, so the attached patch has this implemented.
>
>> 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.
> It would make testing easier, but the expense seems potentially rather high.
> How would a GUC switch be allowed to operate, would we have mixed backends or
> would be require all openssl connectins to be dropped before serving nss ones?
>
>>>>> + 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?
> One one of my boxes I have NSS/NSPR installed via homebrew and they don't ship
> an nss-config AFAICT. I wouldn't be surprised if there are other cases.
>
>>>> 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...
> It does however make visual parsing of the source files easer since it's clear
> which ssl.h is being referred to. I'm in favor of keeping it.
>
>>>>> +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...
> Agreed, I'll try to hack up a testcase.
>
>>>> 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?
> It was mostly just a belts-and-suspenders thing, I don't have any hard evidence
> that it's been a thing in any modern NSS version so it can be removed.
>
>>>>> +/*
>>>>> + * 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.
> Right, but that's Import*FD*, not Import*TCPSocket*. We use ImportFD as well
> since it's the API for importing an NSPR socket into NSS and enabling SSL/TLS
> on it. Thats been a public API for a long time. ImportTCPSocket is used to
> import an already opened socket into NSPR, else NSPR must open the socket
> itself. That part has been kept private for reasons unknown, as it's
> incredibly useful.
>
>>>>> + 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?
> Sorry, that sentence wasn't really finished. What I meant to write was that I
> don't really have good answers here. The available implementation is via the
> static var, and there are no alternative APIs. I've tried googling for
> insights but haven't come across any.
>
> The only datapoint I have is that I can't recall there ever being a complaint
> against libcurl doing this exact thing. That of course doesn't mean it cannot
> happen or cause problems.
>
>>> + /*
>>> + * 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 *ce rt,
>> SECKEYPrivateKey *key, SSLKEAType kea);
> They don't, I had missed the deprecation warning as it's not mentioned at all
> in the online documentation:
>
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/SSL_functions/sslfnc.html
>
> (SSL_ConfigServerCert isn't at all mentioned there which dates it to before
> this went it obsoleting SSL_ConfigSecureServer.)
>
> Fixed by removing the superfluous call.
>

I've been looking through the new patch set, in particular the testing
setup.

The way it seems to proceed is to use the existing openssl generated
certificates and imports them into NSS certificate databases. That seems
fine to bootstrap testing, but it seems to me it would be more sound not
to rely on openssl at all. I'd rather see the Makefile containing
commands to create these from scratch, which mirror the openssl
variants. IOW you should be able to build and test this from scratch,
including certificate generation, without having openssl installed at all.

I also notice that the invocations to pk12util don't contain the "sql:"
prefix to the -d option, even though the database was created with that
prefix a few lines above. That seems like a mistake from my reading of
the pk12util man page.

cheers

andrew