Add more sanity checks in SASL exchanges

michaelpq · michaelpq · commit 44bd0126c70b · 2021-07-10T21:45:28.000+09:00
The following checks are added, to make the SASL infrastructure more aware of defects when implementing new mechanisms: - Detect that no output is generated by a mechanism if an exchange fails in the backend, failing if there is a message waiting to be sent. - Handle zero-length messages in the frontend. The backend handles that already, and SCRAM would complain if sending empty messages as this is not authorized for this mechanism, but other mechanisms may want this capability (the SASL specification allows that). - Make sure that a mechanism generates a message in the middle of the exchange in the frontend. SCRAM, as implemented, respects all these requirements already, and the recent refactoring of SASL done in 9fd8557 helps in documenting that in a cleaner way. Analyzed-by: Jacob Champion Author: Michael Paquier Reviewed-by: Jacob Champion Discussion: https://postgr.es/m/3d2a6f5d50e741117d6baf83eb67ebf1a8a35a11.camel@vmware.com
diff --git a/src/backend/libpq/auth-sasl.c b/src/backend/libpq/auth-sasl.c
@@ -171,6 +171,13 @@ CheckSASLAuth(const pg_be_sasl_mech *mech, Port *port, char *shadow_pass,
 
 		if (output)
 		{
+			/*
+			 * PG_SASL_EXCHANGE_FAILURE with some output is forbidden by SASL.
+			 * Make sure here that the mechanism used got that right.
+			 */
+			if (result == PG_SASL_EXCHANGE_FAILURE)
+				elog(ERROR, "output message found after SASL exchange failure");
+
 			/*
 			 * Negotiation generated data to be sent to the client.
 			 */
diff --git a/src/interfaces/libpq/fe-auth-sasl.h b/src/interfaces/libpq/fe-auth-sasl.h
@@ -78,11 +78,12 @@ typedef struct pg_fe_sasl_mech
 	 * Output parameters, to be set by the callback function:
 	 *
 	 *	output:	   A malloc'd buffer containing the client's response to
-	 *			   the server, or NULL if the exchange should be aborted.
-	 *			   (*success should be set to false in the latter case.)
+	 *			   the server (can be empty), or NULL if the exchange should
+	 *			   be aborted.  (*success should be set to false in the
+	 *			   latter case.)
 	 *
-	 *	outputlen: The length of the client response buffer, or zero if no
-	 *			   data should be sent due to an exchange failure
+	 *	outputlen: The length (0 or higher) of the client response buffer,
+	 *			   ignored if output is NULL.
 	 *
 	 *	done:      Set to true if the SASL exchange should not continue,
 	 *			   because the exchange is either complete or failed
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
@@ -674,7 +674,22 @@ pg_SASL_continue(PGconn *conn, int payloadlen, bool final)
 		return STATUS_ERROR;
 	}
 
-	if (outputlen != 0)
+	/*
+	 * If the exchange is not completed yet, we need to make sure that the
+	 * SASL mechanism has generated a message to send back.
+	 */
+	if (output == NULL && !done)
+	{
+		appendPQExpBufferStr(&conn->errorMessage,
+							 libpq_gettext("no client response found after SASL exchange success\n"));
+		return STATUS_ERROR;
+	}
+
+	/*
+	 * SASL allows zero-length responses, so this check uses "output" and not
+	 * "outputlen" to allow the case of an empty message.
+	 */
+	if (output)
 	{
 		/*
 		 * Send the SASL response to the server.

Original file line number	Diff line number	Diff line change
`@@ -78,11 +78,12 @@ typedef struct pg_fe_sasl_mech`
`78`	`78`	`* Output parameters, to be set by the callback function:`
`79`	`79`	`*`
`80`	`80`	`* output: A malloc'd buffer containing the client's response to`
`81`		`- * the server, or NULL if the exchange should be aborted.`
`82`		`- * (*success should be set to false in the latter case.)`
	`81`	`+ * the server (can be empty), or NULL if the exchange should`
	`82`	`+ * be aborted. (*success should be set to false in the`
	`83`	`+ * latter case.)`
`83`	`84`	`*`
`84`		`- * outputlen: The length of the client response buffer, or zero if no`
`85`		`- * data should be sent due to an exchange failure`
	`85`	`+ * outputlen: The length (0 or higher) of the client response buffer,`
	`86`	`+ * ignored if output is NULL.`
`86`	`87`	`*`
`87`	`88`	`* done: Set to true if the SASL exchange should not continue,`
`88`	`89`	`* because the exchange is either complete or failed`