Horton

Re: MAINTAIN privilege -- what do we need to un-revert it?

Noah Misch <noah@leadboat.com>

View thread View on postgr.es
From: Noah Misch <noah@leadboat.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Michael Paquier <michael@paquier.xyz>, Nathan Bossart <nathandbossart@gmail.com>, pgsql-hackers@postgresql.org, Joe Conway <mail@joeconway.com>, Robert Haas <robertmhaas@gmail.com>
Date: 2024-07-12T23:11:49Z
Lists: pgsql-hackers 
On Fri, Jul 12, 2024 at 02:50:52PM -0700, Jeff Davis wrote:
> On Thu, 2024-07-11 at 05:52 -0700, Noah Misch wrote:
> > > I could try to refactor it into two statements and execute them
> > > separately, or I could try to rewrite the statement to use a fully-
> > > qualified destination table before execution. Thoughts?
> > 
> > Those sound fine.  Also fine: just adding a comment on why creation
> > namespace
> > considerations led to not doing it there.
> 
> Attached. 0002 separates the CREATE MATERIALIZED VIEW ... WITH DATA
> into (effectively):
> 
>    CREATE MATERIALIZED VIEW ... WITH NO DATA;
>    REFRESH MATERIALIZED VIEW ...;
> 
> Using refresh also achieves the stated goal more directly: to (mostly)
> ensure that a subsequent REFRESH will succeed.
> 
> Note: the creation itself no longer executes in a security-restricted
> context, but I don't think that's a problem. The only reason it's using
> the security restricted context is so the following REFRESH will
> succeed, right?

Right, that's the only reason.

> @@ -346,13 +339,21 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
>  		PopActiveSnapshot();
>  	}
>  
> -	if (is_matview)
> +	/*
> +	 * For materialized views, use REFRESH, which locks down
> +	 * security-restricted operations and restricts the search_path.
> +	 * Otherwise, one could create a materialized view not possible to
> +	 * refresh.
> +	 */
> +	if (do_refresh)
>  	{
> -		/* Roll back any GUC changes */
> -		AtEOXact_GUC(false, save_nestlevel);
> +		RefreshMatViewStmt *refresh = makeNode(RefreshMatViewStmt);
>  
> -		/* Restore userid and security context */
> -		SetUserIdAndSecContext(save_userid, save_sec_context);
> +		refresh->relation = into->rel;
> +		ExecRefreshMatView(refresh, pstate->p_sourcetext, NULL, qc);
> +
> +		if (qc)
> +			qc->commandTag = CMDTAG_SELECT;
>  	}

Since refresh->relation is a RangeVar, this departs from the standard against
repeated name lookups, from CVE-2014-0062 (commit 5f17304).

Commits

  1. doc: Note that CREATE MATERIALIZED VIEW restricts search_path.

  2. Small refactoring around ExecCreateTableAs().

  3. Add is_create parameter to RefreshMatviewByOid().

  4. Remove unused ParamListInfo argument from ExecRefreshMatView.

  5. When creating materialized views, use REFRESH to load data.

  6. Add missing RestrictSearchPath() calls.

  7. Fix search_path to a safe value during maintenance operations.

  8. Revert MAINTAIN privilege and pg_maintain predefined role.

  9. Avoid repeated name lookups during table and index DDL.

  10. Add a materialized view relations.