Re: Support for NSS as a libpq TLS backend

On Tue, 2020-10-27 at 21:07 +0100, Daniel Gustafsson wrote:
> > On 20 Oct 2020, at 21:15, Andres Freund  wrote:
> > 
> > > +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.

I'm going to try to tackle this part next. It looks like NSS uses RFC
4514 (or something like it) backslash-quoting, which this code either
needs to undo or bypass before performing a comparison.

--Jacob