Horton

Re: Make query cancellation keys longer

Jelte Fennema-Nio <postgres@jeltef.nl>

View thread View on postgr.es
From: Jelte Fennema-Nio <postgres@jeltef.nl>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, pgsql-hackers <pgsql-hackers@postgresql.org>, Thomas Munro <thomas.munro@gmail.com>, Robert Haas <robertmhaas@gmail.com>
Date: 2024-09-05T16:36:10Z
Lists: pgsql-hackers 
On Thu, 5 Sept 2024 at 17:43, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Has there been any work/discussion around not sending the cancel key
> in plaintext from psql? It's not a prerequisite or anything (the
> longer length is a clear improvement either way), but it seems odd
> that this longer "secret" is still just going to be exposed on the
> wire when you press Ctrl+C.

Totally agreed that it would be good to update psql to use the new
much more secure libpq function introduced in PG17[1]. This is not a
trivial change though because it requires refactoring the way we
handle signals (which is why I didn't do it as part of introducing
these new APIs). I had hoped that the work in [2] would either do that
or at least make it a lot easier, but that thread seems to have
stalled. So +1 for doing this, but I think it's a totally separate
change and so should be discussed on a separate thread.

[1]: https://www.postgresql.org/docs/17/libpq-cancel.html#LIBPQ-CANCEL-FUNCTIONS
[2]: https://www.postgresql.org/message-id/flat/20240331222502.03b5354bc6356bc5c388919d%40sraoss.co.jp#1450c8fee45408acaa5b5a1b9a6f70fc

> For the cancel key implementation in particular, I agree with you that
> it's probably not a serious problem. But if other security code starts
> using timingsafe_bcmp() then it might be something to be concerned
> about. Are there any platform/architecture combos that don't provide a
> native timingsafe_bcmp() *and* need a DIT bit for safety?

It sounds to me like we should at least use OpenSSL's CRYPTO_memcmp if
we linked against it and the OS doesn't provide a timingsafe_bcmp.
Would that remove your concerns? I expect anyone that cares about
security to link against some TLS library. That way our "fallback"
implementation is only used on the rare systems where that's not the
case.

Commits

  1. Add timingsafe_bcmp(), for constant-time memory comparison

  2. Add missing declarations to pg_config.h.in

  3. docs: Add a new section and a table listing protocol versions

  4. Make cancel request keys longer

  5. libpq: Add min/max_protocol_version connection options

  6. libpq: Handle NegotiateProtocolVersion message differently

  7. docs: Update phrase on message lengths in the protocol

  8. libpq: Trace all NegotiateProtocolVersion fields

  9. libpq: Add PQfullProtocolVersion to exports.txt

  10. Move cancel key generation to after forking the backend