Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oFIqj-0007d4-Rx for pgsql-hackers@arkaria.postgresql.org; Sat, 23 Jul 2022 17:23:34 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oFIqh-0001Jt-Fi for pgsql-hackers@arkaria.postgresql.org; Sat, 23 Jul 2022 17:23:31 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oFIqh-0001Jc-1n for pgsql-hackers@lists.postgresql.org; Sat, 23 Jul 2022 17:23:31 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oFIqe-00006t-Gt for pgsql-hackers@postgresql.org; Sat, 23 Jul 2022 17:23:30 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 26NHNOFN954937; Sat, 23 Jul 2022 13:23:24 -0400 From: Tom Lane To: Nathan Bossart cc: Kyotaro Horiguchi , michael@paquier.xyz, gurjeet@singh.im, pgsql-hackers@postgresql.org Subject: Re: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS In-reply-to: <20220722225615.GC3998906@nathanxps13> References: <20220722.151600.1094104349120879204.horikyota.ntt@gmail.com> <729330.1658516182@sss.pgh.pa.us> <20220722223359.GB3998906@nathanxps13> <763512.1658529844@sss.pgh.pa.us> <20220722225615.GC3998906@nathanxps13> Comments: In-reply-to Nathan Bossart message dated "Fri, 22 Jul 2022 15:56:15 -0700" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <954833.1658596910.0@sss.pgh.pa.us> Date: Sat, 23 Jul 2022 13:23:24 -0400 Message-ID: <954936.1658597004@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <954833.1658596910.1@sss.pgh.pa.us> Nathan Bossart writes: > On Fri, Jul 22, 2022 at 06:44:04PM -0400, Tom Lane wrote: >> Another idea is to add a "bool interactive" parameter to InitPostgres, >> thereby shoving the issue out to the call sites. Still wouldn't >> expose the am_walsender angle, but conceivably it'd be more >> future-proof anyway? > I hesitated to suggest this exactly because of the WAL sender problem, but > it does seem slightly more future-proof, so +1 for this approach. So about like this then. (I spent some effort on cleaning up the disjointed-to-nonexistent presentation of InitPostgres' parameters.) regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="move-process_session_preload_libraries-3.patch"; charset="us-ascii" Content-ID: <954833.1658596910.2@sss.pgh.pa.us> Content-Description: move-process_session_preload_libraries-3.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/boo= tstrap.c index 088556ab54..58752368e7 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -354,7 +354,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_o= nly) if (pg_link_canary_is_frontend()) elog(ERROR, "backend is incorrectly linked to frontend functions"); = - InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false); + InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL); = /* Initialize stuff for bootstrap-file processing */ for (i =3D 0; i < MAXATTR; i++) diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/= autovacuum.c index 2e146aac93..70a9176c54 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -475,7 +475,7 @@ AutoVacLauncherMain(int argc, char *argv[]) /* Early initialization */ BaseInit(); = - InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false); + InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL); = SetProcessingMode(NormalProcessing); = @@ -1694,12 +1694,13 @@ AutoVacWorkerMain(int argc, char *argv[]) pgstat_report_autovac(dbid); = /* - * Connect to the selected database + * Connect to the selected database, specifying no particular user * * Note: if we have selected a just-deleted database (due to using * stale stats info), we'll fail and exit here. */ - InitPostgres(NULL, dbid, NULL, InvalidOid, dbname, false); + InitPostgres(NULL, dbid, NULL, InvalidOid, false, false, + dbname); SetProcessingMode(NormalProcessing); set_ps_display(dbname); ereport(DEBUG1, diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/= postmaster.c index 1c25457526..e541b16bdb 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -5654,7 +5654,11 @@ BackgroundWorkerInitializeConnection(const char *db= name, const char *username, u (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("database connection requirement not indicated during registr= ation"))); = - InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL, (flags & BG= WORKER_BYPASS_ALLOWCONN) !=3D 0); + InitPostgres(dbname, InvalidOid, /* database to connect to */ + username, InvalidOid, /* role to connect as */ + false, /* never honor session_preload_libraries */ + (flags & BGWORKER_BYPASS_ALLOWCONN) !=3D 0, /* ignore datallowconn? = */ + NULL); /* no out_dbname */ = /* it had better not gotten out of "init" mode yet */ if (!IsInitProcessingMode()) @@ -5677,7 +5681,11 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid= , Oid useroid, uint32 flags) (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("database connection requirement not indicated during registr= ation"))); = - InitPostgres(NULL, dboid, NULL, useroid, NULL, (flags & BGWORKER_BYPASS_= ALLOWCONN) !=3D 0); + InitPostgres(NULL, dboid, /* database to connect to */ + NULL, useroid, /* role to connect as */ + false, /* never honor session_preload_libraries */ + (flags & BGWORKER_BYPASS_ALLOWCONN) !=3D 0, /* ignore datallowconn? = */ + NULL); /* no out_dbname */ = /* it had better not gotten out of "init" mode yet */ if (!IsInitProcessingMode()) diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 8ba1c170f0..3772329759 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -4076,7 +4076,11 @@ PostgresMain(const char *dbname, const char *userna= me) * it inside InitPostgres() instead. In particular, anything that * involves database access should be there, not here. */ - InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL, false); + InitPostgres(dbname, InvalidOid, /* database to connect to */ + username, InvalidOid, /* role to connect as */ + !am_walsender, /* honor session_preload_libraries? */ + false, /* don't ignore datallowconn */ + NULL); /* no out_dbname */ = /* * If the PostmasterContext is still around, recycle the space; we don't @@ -4112,12 +4116,6 @@ PostgresMain(const char *dbname, const char *userna= me) if (am_walsender) InitWalSender(); = - /* - * process any libraries that should be preloaded at backend start (this - * likewise can't be done until GUC settings are complete) - */ - process_session_preload_libraries(); - /* * Send this backend's cancellation info to the frontend. */ diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/po= stinit.c index a5c208a20a..de797c5933 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -622,29 +622,45 @@ BaseInit(void) * InitPostgres * Initialize POSTGRES. * + * Arguments: + * in_dbname, dboid: specify database to connect to, as described below + * username, useroid: specify role to connect as, as described below + * load_session_libraries: TRUE to honor session_preload_libraries + * override_allow_connections: TRUE to connect despite !datallowconn + * out_dbname: optional output parameter, see below; pass NULL if not use= d + * * The database can be specified by name, using the in_dbname parameter, = or by - * OID, using the dboid parameter. In the latter case, the actual databa= se + * OID, using the dboid parameter. Specify NULL or InvalidOid respective= ly + * for the unused parameter. If dboid is provided, the actual database * name can be returned to the caller in out_dbname. If out_dbname isn't * NULL, it must point to a buffer of size NAMEDATALEN. * - * Similarly, the username can be passed by name, using the username para= meter, + * Similarly, the role can be passed by name, using the username paramete= r, * or by OID using the useroid parameter. * - * In bootstrap mode no parameters are used. The autovacuum launcher pro= cess - * doesn't use any parameters either, because it only goes far enough to = be - * able to read pg_database; it doesn't connect to any particular databas= e. - * In walsender mode only username is used. + * In bootstrap mode the database and username parameters are NULL/Invali= dOid. + * The autovacuum launcher process doesn't specify these parameters eithe= r, + * because it only goes far enough to be able to read pg_database; it doe= sn't + * connect to any particular database. An autovacuum worker specifies a + * database but not a username; conversely, a physical walsender specifie= s + * username but not database. + * + * By convention, load_session_libraries should be passed as true in + * "interactive" sessions, false in background processes such as autovacu= um. * - * As of PostgreSQL 8.2, we expect InitProcess() was already called, so w= e - * already have a PGPROC struct ... but it's not completely filled in yet= . + * We expect that InitProcess() was already called, so we already have a + * PGPROC struct ... but it's not completely filled in yet. * * Note: * Be very careful with the order of calls in the InitPostgres function. * -------------------------------- */ void -InitPostgres(const char *in_dbname, Oid dboid, const char *username, - Oid useroid, char *out_dbname, bool override_allow_connections) +InitPostgres(const char *in_dbname, Oid dboid, + const char *username, Oid useroid, + bool load_session_libraries, + bool override_allow_connections, + char *out_dbname) { bool bootstrap =3D IsBootstrapProcessingMode(); bool am_superuser; @@ -1108,6 +1124,16 @@ InitPostgres(const char *in_dbname, Oid dboid, cons= t char *username, /* Initialize this backend's session state. */ InitializeSession(); = + /* + * If this is an interactive session, load any libraries that should be + * preloaded at backend start. Since those are determined by GUCs, this + * can't happen until GUC settings are complete, but we want it to happe= n + * during the initial transaction in case anything that requires databas= e + * access needs to be done. + */ + if (load_session_libraries) + process_session_preload_libraries(); + /* report this backend in the PgBackendStatus array */ if (!bootstrap) pgstat_bestart(); diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index ea9a56d395..067b729d5a 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -449,8 +449,11 @@ extern PGDLLIMPORT AuxProcType MyAuxProcType; /* in utils/init/postinit.c */ extern void pg_split_opts(char **argv, int *argcp, const char *optstr); extern void InitializeMaxBackends(void); -extern void InitPostgres(const char *in_dbname, Oid dboid, const char *us= ername, - Oid useroid, char *out_dbname, bool override_allow_connections); +extern void InitPostgres(const char *in_dbname, Oid dboid, + const char *username, Oid useroid, + bool load_session_libraries, + bool override_allow_connections, + char *out_dbname); extern void BaseInit(void); = /* in utils/init/miscinit.c */ ------- =_aaaaaaaaaa0--