Thread: un-revert the MAINTAIN privilege and the pg_maintain predefined role
Thanks to Jeff's recent work with commits 2af07e2 and 59825d1, the issue that led to the revert of the MAINTAIN privilege and the pg_maintain predefined role (commit 151c22d) should now be resolved. Specifically, there was a concern that roles with the MAINTAIN privilege could use search_path tricks to run arbitrary code as the table owner. Jeff's work prevents this by restricting search_path to a known safe value when running maintenance commands. (This approach and others were discussed on the lists quite extensively, and it was also brought up at the developer meeting at FOSDEM [0] earlier this year.) Given this, I'd like to finally propose un-reverting MAINTAIN and pg_maintain. I created a commitfest entry for this [1] a few weeks ago and attached it to Jeff's search_path thread, but I figured it would be good to create a dedicated thread for this, too. The attached patch is a straight revert of commit 151c22d except for the following small changes: * The catversion bump has been removed for now. The catversion will need to be bumped appropriately if/when this is committed. * The OID for the pg_maintain predefined role needed to be changed. The original OID has been reused for something else since this feature was reverted. * The change in AdjustUpgrade.pm needed to be updated to check for "$old_version < 17" instead of "$old_version < 16". Thoughts? [0] https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2024_Developer_Meeting#The_Path_to_un-reverting_the_MAINTAIN_privilege [1] https://commitfest.postgresql.org/47/4836/ -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, Mar 05, 2024 at 10:12:35AM -0600, Nathan Bossart wrote: > Thanks to Jeff's recent work with commits 2af07e2 and 59825d1, the issue > that led to the revert of the MAINTAIN privilege and the pg_maintain > predefined role (commit 151c22d) should now be resolved. Specifically, > there was a concern that roles with the MAINTAIN privilege could use > search_path tricks to run arbitrary code as the table owner. Jeff's work > prevents this by restricting search_path to a known safe value when running > maintenance commands. (This approach and others were discussed on the > lists quite extensively, and it was also brought up at the developer > meeting at FOSDEM [0] earlier this year.) > > Given this, I'd like to finally propose un-reverting MAINTAIN and > pg_maintain. I created a commitfest entry for this [1] a few weeks ago and > attached it to Jeff's search_path thread, but I figured it would be good to > create a dedicated thread for this, too. The attached patch is a straight > revert of commit 151c22d except for the following small changes: > > * The catversion bump has been removed for now. The catversion will need > to be bumped appropriately if/when this is committed. > > * The OID for the pg_maintain predefined role needed to be changed. The > original OID has been reused for something else since this feature was > reverted. > > * The change in AdjustUpgrade.pm needed to be updated to check for > "$old_version < 17" instead of "$old_version < 16". Given all of this code was previously reviewed and committed, I am planning to forge ahead and commit this early next week, provided no objections or additional feedback materialize. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Thu, Mar 07, 2024 at 10:50:00AM -0600, Nathan Bossart wrote: > Given all of this code was previously reviewed and committed, I am planning > to forge ahead and commit this early next week, provided no objections or > additional feedback materialize. Jeff Davis and I spent some additional time looking at this patch. There are existing inconsistencies among the privilege checks for the various maintenance commands, and the MAINTAIN privilege just builds on the status quo, with one exception. In the v1 patch, I proposed skipping privilege checks when VACUUM recurses to TOAST tables, which means that a user may be able to process a TOAST table for which they've concurrent lost privileges on the main relation (since each table is vacuumed in a separate transaction). It's easy enough to resolve this inconsistency by sending down the parent OID when recursing to a TOAST table and using that for the privilege checks. AFAICT this avoids any kind of cache lookup hazards because we hold a session lock on the main relation in this case. I've done this in the attached v2. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On Tue, 2024-03-12 at 16:05 -0500, Nathan Bossart wrote:
> It's easy enough to resolve this inconsistency by sending
> down the parent OID when recursing to a TOAST table and using that
> for the
> privilege checks. AFAICT this avoids any kind of cache lookup
> hazards
> because we hold a session lock on the main relation in this case.
> I've
> done this in the attached v2.
Looks good to me. Thank you for expanding on the comment, as well.
Regards,
Jeff Davis
On Wed, Mar 13, 2024 at 09:49:26AM -0700, Jeff Davis wrote: > Looks good to me. Thank you for expanding on the comment, as well. Thanks for reviewing! Committed. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com