Make cancel request keys longer

Currently, cancel request key is 32-bit token, which isn't very much
entropy. If you want to cancel another session's query, you can
brute-force it. In most environments, an unauthorized cancellation of
a query isn't very serious, but it nevertheless would be nice to have
more protection from it. Hence make the key longer, to make it harder
to guess.

The longer cancellation keys are generated when using the new protocol
version 3.2. For connections using version 3.0, short 4-bytes keys are
still used.

The new longer key length is not hardcoded in the protocol anymore,
the client is expected to deal with variable length keys, up to 256
bytes. This flexibility allows e.g. a connection pooler to add more
information to the cancel key, which might be useful for finding the
connection.

Reviewed-by: Jelte Fennema-Nio <postgres@jeltef.nl>
Reviewed-by: Robert Haas <robertmhaas@gmail.com> (earlier versions)
Discussion: https://www.postgresql.org/message-id/508d0505-8b7a-4864-a681-e7e5edfe32aa@iki.fi

Author: hlinnaka

doc/src/sgml/protocol.sgml

@@ -49,6 +49,13 @@
   supported by the server.
  </para>
 
+ <para>
+  The difference between protocol version 3.0 and 3.2 is that the secret key
+  used in query cancellation was enlarged from 4 bytes to a variable length
+  field. The BackendKeyData message was changed to accomodate that, and the
+  CancelRequest message was redefined to have a variable length payload.
+ </para>
+
   <para>
    In order to serve multiple clients efficiently, the server launches
    a new <quote>backend</quote> process for each client.
@@ -4062,7 +4069,7 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
       </varlistentry>
 
       <varlistentry>
-       <term>Int32(12)</term>
+       <term>Int32</term>
        <listitem>
         <para>
          Length of message contents in bytes, including self.
@@ -4080,14 +4087,18 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
       </varlistentry>
 
       <varlistentry>
-       <term>Int32</term>
+       <term>Byte<replaceable>n</replaceable></term>
        <listitem>
         <para>
-         The secret key of this backend.
+         The secret key of this backend. This field extends to the end of the
+         message, indicated by the length field. The maximum key length is 256 bytes.
         </para>
        </listitem>
       </varlistentry>
      </variablelist>
+     <para>
+      Before protocol version 3.2, the secret key was always 4 bytes long.
+     </para>
     </listitem>
    </varlistentry>
 
@@ -4293,14 +4304,18 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
       </varlistentry>
 
       <varlistentry>
-       <term>Int32</term>
+       <term>Byte<replaceable>n</replaceable></term>
        <listitem>
         <para>
-         The secret key for the target backend.
+         The secret key for the target backend. This field extends to the end of the
+         message, indicated by the length field. The maximum key length is 256 bytes.
         </para>
        </listitem>
       </varlistentry>
      </variablelist>
+     <para>
+      Before protocol version 3.2, the secret key was always 4 bytes long.
+     </para>
     </listitem>
    </varlistentry>
 

src/backend/storage/ipc/procsignal.c

@@ -63,8 +63,8 @@
 typedef struct
 {
 	pg_atomic_uint32 pss_pid;
-	bool		pss_cancel_key_valid;
-	int32		pss_cancel_key;
+	int			pss_cancel_key_len; /* 0 means no cancellation is possible */
+	char		pss_cancel_key[MAX_CANCEL_KEY_LENGTH];
 	volatile sig_atomic_t pss_signalFlags[NUM_PROCSIGNALS];
 	slock_t		pss_mutex;		/* protects the above fields */
 
@@ -148,8 +148,7 @@ ProcSignalShmemInit(void)
 
 			SpinLockInit(&slot->pss_mutex);
 			pg_atomic_init_u32(&slot->pss_pid, 0);
-			slot->pss_cancel_key_valid = false;
-			slot->pss_cancel_key = 0;
+			slot->pss_cancel_key_len = 0;
 			MemSet(slot->pss_signalFlags, 0, sizeof(slot->pss_signalFlags));
 			pg_atomic_init_u64(&slot->pss_barrierGeneration, PG_UINT64_MAX);
 			pg_atomic_init_u32(&slot->pss_barrierCheckMask, 0);
@@ -163,12 +162,13 @@ ProcSignalShmemInit(void)
  *		Register the current process in the ProcSignal array
  */
 void
-ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
+ProcSignalInit(char *cancel_key, int cancel_key_len)
 {
 	ProcSignalSlot *slot;
 	uint64		barrier_generation;
 	uint32		old_pss_pid;
 
+	Assert(cancel_key_len >= 0 && cancel_key_len <= MAX_CANCEL_KEY_LENGTH);
 	if (MyProcNumber < 0)
 		elog(ERROR, "MyProcNumber not set");
 	if (MyProcNumber >= NumProcSignalSlots)
@@ -199,8 +199,9 @@ ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
 		pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration);
 	pg_atomic_write_u64(&slot->pss_barrierGeneration, barrier_generation);
 
-	slot->pss_cancel_key_valid = cancel_key_valid;
-	slot->pss_cancel_key = cancel_key;
+	if (cancel_key_len > 0)
+		memcpy(slot->pss_cancel_key, cancel_key, cancel_key_len);
+	slot->pss_cancel_key_len = cancel_key_len;
 	pg_atomic_write_u32(&slot->pss_pid, MyProcPid);
 
 	SpinLockRelease(&slot->pss_mutex);
@@ -254,8 +255,7 @@ CleanupProcSignalState(int status, Datum arg)
 
 	/* Mark the slot as unused */
 	pg_atomic_write_u32(&slot->pss_pid, 0);
-	slot->pss_cancel_key_valid = false;
-	slot->pss_cancel_key = 0;
+	slot->pss_cancel_key_len = 0;
 
 	/*
 	 * Make this slot look like it's absorbed all possible barriers, so that
@@ -725,7 +725,7 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
  * fields in the ProcSignal slots.
  */
 void
-SendCancelRequest(int backendPID, int32 cancelAuthCode)
+SendCancelRequest(int backendPID, char *cancel_key, int cancel_key_len)
 {
 	Assert(backendPID != 0);
 
@@ -754,7 +754,8 @@ SendCancelRequest(int backendPID, int32 cancelAuthCode)
 		}
 		else
 		{
-			match = slot->pss_cancel_key_valid && slot->pss_cancel_key == cancelAuthCode;
+			match = slot->pss_cancel_key_len == cancel_key_len &&
+				timingsafe_bcmp(slot->pss_cancel_key, cancel_key, cancel_key_len) == 0;
 
 			SpinLockRelease(&slot->pss_mutex);
 

src/backend/tcop/backend_startup.c

@@ -59,6 +59,7 @@ ConnectionTiming conn_timing = {.ready_for_use = TIMESTAMP_MINUS_INFINITY};
 static void BackendInitialize(ClientSocket *client_sock, CAC_state cac);
 static int	ProcessSSLStartup(Port *port);
 static int	ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done);
+static void ProcessCancelRequestPacket(Port *port, void *pkt, int pktlen);
 static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
 static void process_startup_packet_die(SIGNAL_ARGS);
 static void StartupPacketTimeoutHandler(void);
@@ -557,28 +558,7 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
 
 	if (proto == CANCEL_REQUEST_CODE)
 	{
-		/*
-		 * The client has sent a cancel request packet, not a normal
-		 * start-a-new-connection packet.  Perform the necessary processing.
-		 * Nothing is sent back to the client.
-		 */
-		CancelRequestPacket *canc;
-		int			backendPID;
-		int32		cancelAuthCode;
-
-		if (len != sizeof(CancelRequestPacket))
-		{
-			ereport(COMMERROR,
-					(errcode(ERRCODE_PROTOCOL_VIOLATION),
-					 errmsg("invalid length of startup packet")));
-			return STATUS_ERROR;
-		}
-		canc = (CancelRequestPacket *) buf;
-		backendPID = (int) pg_ntoh32(canc->backendPID);
-		cancelAuthCode = (int32) pg_ntoh32(canc->cancelAuthCode);
-
-		if (backendPID != 0)
-			SendCancelRequest(backendPID, cancelAuthCode);
+		ProcessCancelRequestPacket(port, buf, len);
 		/* Not really an error, but we don't want to proceed further */
 		return STATUS_ERROR;
 	}
@@ -878,6 +858,37 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
 	return STATUS_OK;
 }
 
+/*
+ * The client has sent a cancel request packet, not a normal
+ * start-a-new-connection packet.  Perform the necessary processing.  Nothing
+ * is sent back to the client.
+ */
+static void
+ProcessCancelRequestPacket(Port *port, void *pkt, int pktlen)
+{
+	CancelRequestPacket *canc;
+	int			len;
+
+	if (pktlen < offsetof(CancelRequestPacket, cancelAuthCode))
+	{
+		ereport(COMMERROR,
+				(errcode(ERRCODE_PROTOCOL_VIOLATION),
+				 errmsg("invalid length of query cancel packet")));
+		return;
+	}
+	len = pktlen - offsetof(CancelRequestPacket, cancelAuthCode);
+	if (len == 0 || len > 256)
+	{
+		ereport(COMMERROR,
+				(errcode(ERRCODE_PROTOCOL_VIOLATION),
+				 errmsg("invalid length of query cancel key")));
+		return;
+	}
+
+	canc = (CancelRequestPacket *) pkt;
+	SendCancelRequest(pg_ntoh32(canc->backendPID), canc->cancelAuthCode, len);
+}
+
 /*
  * Send a NegotiateProtocolVersion to the client.  This lets the client know
  * that they have either requested a newer minor protocol version than we are

src/backend/tcop/postgres.c

@@ -4259,16 +4259,20 @@ PostgresMain(const char *dbname, const char *username)
 	 * Generate a random cancel key, if this is a backend serving a
 	 * connection. InitPostgres() will advertise it in shared memory.
 	 */
-	Assert(!MyCancelKeyValid);
+	Assert(MyCancelKeyLength == 0);
 	if (whereToSendOutput == DestRemote)
 	{
-		if (!pg_strong_random(&MyCancelKey, sizeof(int32)))
+		int			len;
+
+		len = (MyProcPort == NULL || MyProcPort->proto >= PG_PROTOCOL(3, 1))
+			? MAX_CANCEL_KEY_LENGTH : 4;
+		if (!pg_strong_random(&MyCancelKey, len))
 		{
 			ereport(ERROR,
 					(errcode(ERRCODE_INTERNAL_ERROR),
 					 errmsg("could not generate random cancel key")));
 		}
-		MyCancelKeyValid = true;
+		MyCancelKeyLength = len;
 	}
 
 	/*
@@ -4323,10 +4327,11 @@ PostgresMain(const char *dbname, const char *username)
 	{
 		StringInfoData buf;
 
-		Assert(MyCancelKeyValid);
+		Assert(MyCancelKeyLength > 0);
 		pq_beginmessage(&buf, PqMsg_BackendKeyData);
 		pq_sendint32(&buf, (int32) MyProcPid);
-		pq_sendint32(&buf, (int32) MyCancelKey);
+
+		pq_sendbytes(&buf, MyCancelKey, MyCancelKeyLength);
 		pq_endmessage(&buf);
 		/* Need not flush since ReadyForQuery will do it. */
 	}

src/backend/utils/init/globals.c

@@ -24,6 +24,7 @@
 #include "miscadmin.h"
 #include "postmaster/postmaster.h"
 #include "storage/procnumber.h"
+#include "storage/procsignal.h"
 
 
 ProtocolVersion FrontendProtocol;
@@ -48,8 +49,8 @@ pg_time_t	MyStartTime;
 TimestampTz MyStartTimestamp;
 struct ClientSocket *MyClientSocket;
 struct Port *MyProcPort;
-bool		MyCancelKeyValid = false;
-int32		MyCancelKey = 0;
+char		MyCancelKey[MAX_CANCEL_KEY_LENGTH];
+uint8		MyCancelKeyLength = 0;
 int			MyPMChildSlot;
 
 /*

src/backend/utils/init/postinit.c

@@ -753,7 +753,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
 	 */
 	SharedInvalBackendInit(false);
 
-	ProcSignalInit(MyCancelKeyValid, MyCancelKey);
+	ProcSignalInit(MyCancelKey, MyCancelKeyLength);
 
 	/*
 	 * Also set up timeout handlers needed for backend operation.  We need

src/include/libpq/pqcomm.h

@@ -128,15 +128,20 @@ typedef uint32 AuthRequest;
  *
  * The cancel request code must not match any protocol version number
  * we're ever likely to use.  This random choice should do.
+ *
+ * Before PostgreSQL v17 and the protocol version bump from 3.0 to 3.1, the
+ * cancel key was always 4 bytes. Starting with v17, it's variable length.
  */
+
 #define CANCEL_REQUEST_CODE PG_PROTOCOL(1234,5678)
 
 typedef struct CancelRequestPacket
 {
 	/* Note that each field is stored in network byte order! */
 	MsgType		cancelRequestCode;	/* code to identify a cancel request */
 	uint32		backendPID;		/* PID of client's backend */
-	uint32		cancelAuthCode; /* secret key to authorize cancel */
+	char		cancelAuthCode[FLEXIBLE_ARRAY_MEMBER];	/* secret key to
+														 * authorize cancel */
 } CancelRequestPacket;
 
 /* Application-Layer Protocol Negotiation is required for direct connections

src/include/miscadmin.h

@@ -191,8 +191,8 @@ extern PGDLLIMPORT pg_time_t MyStartTime;
 extern PGDLLIMPORT TimestampTz MyStartTimestamp;
 extern PGDLLIMPORT struct Port *MyProcPort;
 extern PGDLLIMPORT struct Latch *MyLatch;
-extern PGDLLIMPORT bool MyCancelKeyValid;
-extern PGDLLIMPORT int32 MyCancelKey;
+extern PGDLLIMPORT char MyCancelKey[];
+extern PGDLLIMPORT uint8 MyCancelKeyLength;
 extern PGDLLIMPORT int MyPMChildSlot;
 
 extern PGDLLIMPORT char OutputFileName[];

src/include/storage/procsignal.h

@@ -56,16 +56,26 @@ typedef enum
 	PROCSIGNAL_BARRIER_SMGRRELEASE, /* ask smgr to close files */
 } ProcSignalBarrierType;
 
+/*
+ * Length of query cancel keys generated.
+ *
+ * Note that the protocol allows for longer keys, or shorter, but this is the
+ * length we actually generate.  Client code, and the server code that handles
+ * incoming cancellation packets from clients, cannot use this hardcoded
+ * length.
+ */
+#define MAX_CANCEL_KEY_LENGTH  32
+
 /*
  * prototypes for functions in procsignal.c
  */
 extern Size ProcSignalShmemSize(void);
 extern void ProcSignalShmemInit(void);
 
-extern void ProcSignalInit(bool cancel_key_valid, int32 cancel_key);
+extern void ProcSignalInit(char *cancel_key, int cancel_key_len);
 extern int	SendProcSignal(pid_t pid, ProcSignalReason reason,
 						   ProcNumber procNumber);
-extern void SendCancelRequest(int backendPID, int32 cancelAuthCode);
+extern void SendCancelRequest(int backendPID, char *cancel_key, int cancel_key_len);
 
 extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type);
 extern void WaitForProcSignalBarrier(uint64 generation);