libpq: Poison the v2 part of a v1 Bearer request

The new PGoauthBearerRequestV2 API (which has similarities to the
"subclass" pointer architecture in use by the backend, for Nodes)
carries the risk of a developer ignoring the type of hook in use and
just casting directly to the V2 struct. This will appear to work fine in
19, but crash (or worse) when speaking to libpq 18.

However, we're in a unique position to catch this problem, because we
have tight control over the struct. Add poisoning code to the v1 path
which does the following:

- masks the v2 request->issuer pointer, to hopefully point at nonsense
  memory
- abort()s if the v2 request->error is assigned by the hook
- attempts to cover both with VALGRIND_MAKE_MEM_NOACCESS for the
  duration of the callback (a potential AddressSanitizer implementation
  is left for future work)

The struct is unpoisoned after the call, so we can switch back to the v2
internal implementation when necessary.

Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/CAOYmi%2BnCg5upBVOo_UCSjMfO%3DYMkZXcSEsgaADKXqerr5wahZQ%40mail.gmail.com

diff --git a/src/interfaces/libpq/fe-auth-oauth.c b/src/interfaces/libpq/fe-auth-oauth.c
index f93184f04db1e813cc91d528b4319cba572a9655..10e995f2c4d0c9b046d4d0e145f76a95eb81dd46 100644 (file)
--- a/src/interfaces/libpq/fe-auth-oauth.c
+++ b/src/interfaces/libpq/fe-auth-oauth.c
@@ -27,6 +27,12 @@
 #include "fe-auth-oauth.h"
 #include "mb/pg_wchar.h"
 #include "pg_config_paths.h"
+#include "utils/memdebug.h"
+
+static PostgresPollingStatusType do_async(fe_oauth_state *state,
+                                                                                 PGoauthBearerRequestV2 *request);
+static void do_cleanup(fe_oauth_state *state, PGoauthBearerRequestV2 *request);
+static void poison_req_v2(PGoauthBearerRequestV2 *request, bool poison);
 
 /* The exported OAuth callback mechanism. */
 static void *oauth_init(PGconn *conn, const char *password,
@@ -741,9 +747,7 @@ run_oauth_flow(PGconn *conn)
                return PGRES_POLLING_FAILED;
        }
 
-       status = request->v1.async(conn,
-                                                          (PGoauthBearerRequest *) request,
-                                                          &conn->altsock);
+       status = do_async(state, request);
 
        if (status == PGRES_POLLING_FAILED)
        {
@@ -804,8 +808,7 @@ cleanup_oauth_flow(PGconn *conn)
 
        Assert(request);
 
-       if (request->v1.cleanup)
-               request->v1.cleanup(conn, (PGoauthBearerRequest *) request);
+       do_cleanup(state, request);
        conn->altsock = PGINVALID_SOCKET;
 
        free(request);
@@ -1019,7 +1022,14 @@ setup_token_request(PGconn *conn, fe_oauth_state *state)
         */
        res = PQauthDataHook(PQAUTHDATA_OAUTH_BEARER_TOKEN_V2, conn, &request);
        if (res == 0)
+       {
+               poison_req_v2(&request, true);
+
                res = PQauthDataHook(PQAUTHDATA_OAUTH_BEARER_TOKEN, conn, &request);
+               state->v1 = (res != 0);
+
+               poison_req_v2(&request, false);
+       }
        if (res == 0)
        {
                state->builtin = true;
@@ -1045,8 +1055,7 @@ setup_token_request(PGconn *conn, fe_oauth_state *state)
                        }
 
                        /* short-circuit */
-                       if (request.v1.cleanup)
-                               request.v1.cleanup(conn, (PGoauthBearerRequest *) &request);
+                       do_cleanup(state, &request);
                        return true;
                }
 
@@ -1076,8 +1085,7 @@ setup_token_request(PGconn *conn, fe_oauth_state *state)
                libpq_append_conn_error(conn, "no OAuth flows are available (try installing the libpq-oauth package)");
 
 fail:
-       if (request.v1.cleanup)
-               request.v1.cleanup(conn, (PGoauthBearerRequest *) &request);
+       do_cleanup(state, &request);
        return false;
 }
 
@@ -1428,3 +1436,119 @@ oauth_unsafe_debugging_enabled(void)
 
        return (env && strcmp(env, "UNSAFE") == 0);
 }
+
+/*
+ * Hook v1 Poisoning
+ *
+ * Try to catch misuses of the v1 PQAUTHDATA_OAUTH_BEARER_TOKEN hook and its
+ * callbacks, which are not allowed to downcast their request argument to
+ * PGoauthBearerRequestV2. (Such clients may crash or worse when speaking to
+ * libpq 18.)
+ *
+ * This attempts to use Valgrind hooks, if present, to mark the extra members as
+ * inaccessible. For uninstrumented builds, it also munges request->issuer to
+ * try to crash clients that perform string operations, and it aborts if
+ * request->error is set.
+ */
+
+#define MASK_BITS ((uintptr_t) 0x55aa55aa55aa55aa)
+#define POISON_MASK(ptr) ((void *) (((uintptr_t) ptr) ^ MASK_BITS))
+
+/*
+ * Workhorse for v2 request poisoning. This must be called exactly twice: once
+ * to poison, once to unpoison.
+ *
+ * NB: Unpoisoning must restore the request to its original state, because we
+ * might still switch back to a v2 implementation internally. Don't do anything
+ * destructive during the poison operation.
+ */
+static void
+poison_req_v2(PGoauthBearerRequestV2 *request, bool poison)
+{
+#ifdef USE_VALGRIND
+       void       *const base = (char *) request + sizeof(request->v1);
+       const size_t len = sizeof(*request) - sizeof(request->v1);
+#endif
+
+       if (poison)
+       {
+               /* Poison request->issuer with a mask to help uninstrumented builds. */
+               request->issuer = POISON_MASK(request->issuer);
+
+               /*
+                * We'll check to make sure request->error wasn't assigned when
+                * unpoisoning, so it had better not be assigned now.
+                */
+               Assert(!request->error);
+
+               VALGRIND_MAKE_MEM_NOACCESS(base, len);
+       }
+       else
+       {
+               /*
+                * XXX Using DEFINED here is technically too lax; we might catch
+                * struct padding in the blast radius. But since this API has to
+                * poison stack addresses, and Valgrind can't track/manage undefined
+                * stack regions, we can't be any stricter without tracking the
+                * original state of the memory.
+                */
+               VALGRIND_MAKE_MEM_DEFINED(base, len);
+
+               /* Undo our mask. */
+               request->issuer = POISON_MASK(request->issuer);
+
+               /*
+                * For uninstrumented builds, make sure request->error wasn't touched.
+                */
+               if (request->error)
+               {
+                       fprintf(stderr,
+                                       "abort! out-of-bounds write to PGoauthBearerRequest by PQAUTHDATA_OAUTH_BEARER_TOKEN hook\n");
+                       abort();
+               }
+       }
+}
+
+/*
+ * Wrapper around PGoauthBearerRequest.async() which applies poison during the
+ * callback when necessary.
+ */
+static PostgresPollingStatusType
+do_async(fe_oauth_state *state, PGoauthBearerRequestV2 *request)
+{
+       PostgresPollingStatusType ret;
+       PGconn     *conn = state->conn;
+
+       Assert(request->v1.async);
+
+       if (state->v1)
+               poison_req_v2(request, true);
+
+       ret = request->v1.async(conn,
+                                                       (PGoauthBearerRequest *) request,
+                                                       &conn->altsock);
+
+       if (state->v1)
+               poison_req_v2(request, false);
+
+       return ret;
+}
+
+/*
+ * Similar wrapper for the optional PGoauthBearerRequest.cleanup() callback.
+ * Does nothing if one is not defined.
+ */
+static void
+do_cleanup(fe_oauth_state *state, PGoauthBearerRequestV2 *request)
+{
+       if (!request->v1.cleanup)
+               return;
+
+       if (state->v1)
+               poison_req_v2(request, true);
+
+       request->v1.cleanup(state->conn, (PGoauthBearerRequest *) request);
+
+       if (state->v1)
+               poison_req_v2(request, false);
+}

diff --git a/src/interfaces/libpq/fe-auth-oauth.h b/src/interfaces/libpq/fe-auth-oauth.h
index 511284614f779243d5d1c4a7719de614996e6724..2b22d23ffdeaf526cffc0ce06264a770bfff75ed 100644 (file)
--- a/src/interfaces/libpq/fe-auth-oauth.h
+++ b/src/interfaces/libpq/fe-auth-oauth.h
@@ -34,6 +34,7 @@ typedef struct
        PGconn     *conn;
        void       *async_ctx;
 
+       bool            v1;
        bool            builtin;
        void       *builtin_flow;
 } fe_oauth_state;