Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\))
Subject: Re: new heapcheck contrib module
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: 
 <CA+TgmoZbPtTgkQBGoco39uqKUWuv=0+TOkiNcCuqWWvUkJVuxQ@mail.gmail.com>
Date: Tue, 22 Sep 2020 10:55:35 -0700
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>
Content-Transfer-Encoding: quoted-printable
Message-Id: <107F3C9D-2598-4E77-A4D0-0084BD3CBA30@enterprisedb.com>
References: <12ED3DA8-25F0-4B68-937D-D907CFBF08E7@enterprisedb.com>
 <20200420194210.sevnpi4lyghoanxi@alap3.anarazel.de>
 <23E60C3C-C20A-4508-84AC-E0CEA59E784A@enterprisedb.com>
 <CA+TgmobAm8vB1MNQ1taO1et+HxTN8ULnQvEG+grNDqNQTBjbtA@mail.gmail.com>
 <F845AC50-A479-43F3-ADFB-861A4D2AD287@enterprisedb.com>
 <5F743835-3399-419C-8324-2D424237E999@enterprisedb.com>
 <CAFiTN-syyHc3jZoou51v0SR8z0POoNfktqEO6MaGig4YS8mosA@mail.gmail.com>
 <E6F4258A-B661-4F2C-8A35-278C9436930A@enterprisedb.com>
 <CAFiTN-s-VX3DLfqv7OOo0zGCTOZ3xVU9UZ6GgzBZdJTPNnYFNg@mail.gmail.com>
 <885D2EC9-7D06-4903-A17D-72C184A1E1C5@enterprisedb.com>
 <CAFiTN-uYg_PiZ854=5R-D9skratapAHKhaDAG7fhJv-Vn5UhaQ@mail.gmail.com>
 <5CC85F28-F210-47AC-8DA8-EC4E82AF3FD5@enterprisedb.com>
 <CAFiTN-sP5iFJGERTq4a4OLTCzjZKhLC5P6_k2bxQKXnuvdmzGA@mail.gmail.com>
 <CAFiTN-tTg0R5AHvXC1ro9XphroX1wUtc5t91hU_+7DeGTpE5VQ@mail.gmail.com>
 <0C0349EB-DD85-4DA8-82C7-DF70AB714592@enterprisedb.com>
 <CAFiTN-uD3WSeggU20yU6voGou6BDb-cQcfkE8B1rAzWKZVK-Bw@mail.gmail.com>
 <BA6E870F-241C-4DDB-B92D-DB39E679C7D4@enterprisedb.com>
 <CA+TgmobQUdfM2Q7WAR606RR2YehCZ5GHRKBc-VWfRj-1ZUKqDA@mail.gmail.com>
 <C4A3E5AE-CBB2-4E3F-8BAE-29EE216762C2@enterprisedb.com>
 <CAAJ_b96_Xr2a+D0j8xKbOnCzURcVQm0V2DS+T_pfc+wt3BpL8g@mail.gmail.com>
 <9DF93B83-FE46-4EC4-A99D-77A60729FD2D@enterprisedb.com>
 <CA+TgmoZT3kCOaKq6=m5wHGKh_9XQDbCC1LZXL6w_+NMB-n1+mw@mail.gmail.com>
 <CAAJ_b94hsrF4x7UKQj03416fAEXJorUpZ9WhsZZjqgv-4+Wg9A@mail.gmail.com>
 <50F6468F-5A5E-412A-86EC-DF24B549D733@enterprisedb.com>
 <CAAJ_b94+A1Gk5r5sBYbYM2K5hH_gyp_o=+gwJuJhn5ZEm5_rqw@mail.gmail.com>
 <CAAJ_b94Wh_7kM9JuJk7wnNsYSNPdSzz35cOEaE_ZRnPcqvupCg@mail.gmail.com>
 <7D603B53-8078-4AF0-851B-803FF4416501@enterprisedb.com>
 <CA+TgmoZbPtTgkQBGoco39uqKUWuv=0+TOkiNcCuqWWvUkJVuxQ@mail.gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Precedence: bulk


> On Sep 21, 2020, at 2:09 PM, Robert Haas <robertmhaas@gmail.com> =
wrote:
>=20
> 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.=20

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?

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company