public inbox for [email protected]
help / color / mirror / Atom feedFrom: Jacob Champion <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Stephen Frost <[email protected]>
Cc: Thomas Munro <[email protected]>
Subject: Re: Support for NSS as a libpq TLS backend
Date: Mon, 16 Nov 2020 20:00:47 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<CAA8=A7-suZg90M7Fgyo1Rfymkrpdh_Z547RKXrbHirsAmojQSw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
On Nov 13, 2020, at 4:14 AM, Daniel Gustafsson <[email protected]> wrote:
>> On 12 Nov 2020, at 23:12, Jacob Champion <[email protected]> wrote:
>>
>> I'm not completely sure why this is exposed so easily with an OpenSSL
>> server -- I'm guessing the implementation slices up its packets
>> differently on the wire, causing a read event before NSS is able to
>> decrypt a full record -- but it's worth noting that this case also shows
>> up during NSS-to-NSS psql connections, when handling notifications at
>> the end of every query. PQconsumeInput() reports a hard failure with the
>> current implementation, but its return value is ignored by
>> PrintNotifications(). Otherwise this probably would have showed up
>> earlier.
>
> Should there perhaps be an Assert there to catch those?
Hm. From the perspective of helping developers out, perhaps, but from
the standpoint of "don't crash when an endpoint outside our control does
something strange", I think that's a harder sell. Should the error be
bubbled all the way up instead? Or perhaps, if psql isn't supposed to
treat notification errors as "hard" failures, it should at least warn
the user that something is fishy?
>> (What's the best way to test this case? Are there lower-level tests for
>> the protocol/network layer somewhere that I'm missing?)
>
> Not AFAIK. Having been knee-deep now, do you have any ideas on how to
> implement?
I think that testing these sorts of important edge cases needs a
friendly DSL -- something that doesn't want to make devs tear their hair
out while building tests. I've been playing a little bit with Scapy [1]
to understand more of the libpq v3 protocol; I'll see if that can be
adapted for pieces of the TLS handshake in a way that's easy to
maintain. If it can be, maybe that'd be a good starting example.
> I've incorporated this patch as well as the previous patch for the assertion
> failure on private callback data into the attached v19 patchset. I also did a
> spellcheck and pgindent run on it for ease of review.
Commit 6be725e70 got rid of some psql error messaging that the tests
were keying off of, so there are a few new failures after a rebase onto
latest master.
I've attached a patch that gets the SCRAM tests a little further
(certificate hashing was caught in an infinite loop). I also added error
checks to those loops, along the lines of the existing OpenSSL
implementation: if a suitable digest can't be found, the user will see
an error like
psql: error: could not find digest for OID 'PKCS #1 SHA-256 With RSA Encryption'
It's a little verbose but I don't think this case should come up in
normal practice.
--Jacob
[1] https://scapy.net/
Attachments:
[application/octet-stream] nss-fix-hang-when-hashing-certificates.patch (3.2K, ../[email protected]/2-nss-fix-hang-when-hashing-certificates.patch)
download | inline diff:
From 2f1d58f9c376429ca3ff84d0489824603d2bcd52 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 16 Nov 2020 08:55:33 -0800
Subject: [PATCH] nss: fix hang when hashing certificates
---
src/backend/libpq/be-secure-nss.c | 19 ++++++++++++-------
src/interfaces/libpq/fe-secure-nss.c | 22 ++++++++++++++++------
2 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/src/backend/libpq/be-secure-nss.c b/src/backend/libpq/be-secure-nss.c
index e7540c4756..b1982ee596 100644
--- a/src/backend/libpq/be-secure-nss.c
+++ b/src/backend/libpq/be-secure-nss.c
@@ -813,6 +813,7 @@ be_tls_get_certificate_hash(Port *port, size_t *len)
SECOidTag signature_alg;
SECOidTag digest_alg;
int digest_len;
+ const NSSSignatureAlgorithms *candidate;
SECStatus status;
PLArenaPool *arena = NULL;
SECItem digest;
@@ -824,20 +825,24 @@ be_tls_get_certificate_hash(Port *port, size_t *len)
return NULL;
signature_alg = SECOID_GetAlgorithmTag(&certificate->signature);
- /* Set sha-256 as a default */
- digest_alg = SEC_OID_SHA256;
- digest_len = SHA256_LENGTH;
- while (NSS_SCRAMDigestAlgorithm->signature)
+ candidate = NSS_SCRAMDigestAlgorithm;
+ while (candidate->signature)
{
- if (signature_alg == NSS_SCRAMDigestAlgorithm->signature)
+ if (signature_alg == candidate->signature)
{
- digest_alg = NSS_SCRAMDigestAlgorithm->hash;
- digest_len = NSS_SCRAMDigestAlgorithm->len;
+ digest_alg = candidate->hash;
+ digest_len = candidate->len;
break;
}
+
+ candidate++;
}
+ if (!candidate->signature)
+ elog(ERROR, "could not find digest for OID '%s'",
+ SECOID_FindOIDTagDescription(signature_alg));
+
arena = PORT_NewArena(SEC_ASN1_DEFAULT_ARENA_SIZE);
digest.data = PORT_ArenaZAlloc(arena, sizeof(unsigned char) * digest_len);
digest.len = digest_len;
diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 787bc140a6..2f10e94f72 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -540,6 +540,7 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
SECOidTag signature_alg;
SECOidTag digest_alg;
int digest_len;
+ const NSSSignatureAlgorithms *candidate;
PLArenaPool *arena;
SECItem digest;
char *ret;
@@ -552,17 +553,26 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
return NULL;
signature_alg = SECOID_GetAlgorithmTag(&server_cert->signature);
- digest_alg = SEC_OID_SHA256;
- digest_len = SHA256_LENGTH;
- while (NSS_SCRAMDigestAlgorithm->signature)
+ candidate = NSS_SCRAMDigestAlgorithm;
+ while (candidate->signature)
{
- if (signature_alg == NSS_SCRAMDigestAlgorithm->signature)
+ if (signature_alg == candidate->signature)
{
- digest_alg = NSS_SCRAMDigestAlgorithm->hash;
- digest_len = NSS_SCRAMDigestAlgorithm->len;
+ digest_alg = candidate->hash;
+ digest_len = candidate->len;
break;
}
+
+ candidate++;
+ }
+
+ if (!candidate->signature)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not find digest for OID '%s'\n"),
+ SECOID_FindOIDTagDescription(signature_alg));
+ return NULL;
}
arena = PORT_NewArena(SEC_ASN1_DEFAULT_ARENA_SIZE);
--
2.24.1
view thread (181+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Support for NSS as a libpq TLS backend
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox