Horton

Re: new heapcheck contrib module

Mark Dilger <mark.dilger@enterprisedb.com>

View thread View on postgr.es
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Amul Sul <sulamul@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Peter Geoghegan <pg@bowt.ie>, Andres Freund <andres@anarazel.de>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2020-09-22T17:55:35Z
Lists: pgsql-hackers 

> On Sep 21, 2020, at 2:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> I think there should be a call to pg_class_aclcheck() here, just like
> the one in pg_prewarm, so that if the superuser does choose to grant
> access, users given access can check tables they anyway have
> permission to access, but not others. Maybe put that in
> check_relation_relkind_and_relam() and rename it. Might want to look
> at the pg_surgery precedent, too. 

In the presence of corruption, verify_heapam() reports to the user (in other words, leaks) metadata about the corrupted rows.  Reasoning about the attack vectors this creates is hard, but a conservative approach is to assume that an attacker can cause corruption in order to benefit from the leakage, and make sure the leakage does not violate any reasonable security expectations.

Basing the security decision on whether the user has access to read the table seems insufficient, as it ignores row level security.  Perhaps that is ok if row level security is not enabled for the table or if the user has been granted BYPASSRLS.  There is another problem, though.  There is no grantable privilege to read dead rows.  In the case of corruption, verify_heapam() may well report metadata about dead rows.

pg_surgery also appears to leak information about dead rows.  Owners of tables can probe whether supplied TIDs refer to dead rows.  If a table containing sensitive information has rows deleted prior to ownership being transferred, the new owner of the table could probe each page of deleted data to determine something of the content that was there.  Information about the number of deleted rows is already available through the pg_stat_* views, but those views don't give such a fine-grained approach to figuring out how large each deleted row was.  For a table with fixed content options, the content can sometimes be completely inferred from the length of the row.  (Consider a table with a single text column containing either "approved" or "denied".)

But pg_surgery is understood to be a collection of sharp tools only to be used under fairly exceptional conditions.  amcheck, on the other hand, is something that feels safer and more reasonable to use on a regular basis, perhaps from a cron job executed by a less trusted user.  Forcing the user to be superuser makes it clearer that this feeling of safety is not justified.

I am inclined to just restrict verify_heapam() to superusers and be done.  What do you think?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Commits

  1. Add pg_amcheck, a CLI for contrib/amcheck.

  2. Refactor and generalize the ParallelSlot machinery.

  3. Generalize parallel slot result handling.

  4. Move some code from src/bin/scripts to src/fe_utils to permit reuse.

  5. Factor pattern-construction logic out of processSQLNamePattern.

  6. Doc: clean up verify_heapam() documentation.

  7. Fix more portability issues in new amcheck code.

  8. Fix portability issues in new amcheck test.

  9. Try to avoid a compiler warning about using fxid uninitialized.

  10. Extend amcheck to check heap pages.

  11. Adjust walsender usage of xlogreader, simplify APIs

  12. Improve checking of child pages in contrib/amcheck.

  13. Sanitize line pointers within contrib/amcheck.

  14. Fix possible sorting error when aborting use of abbreviated keys.