Re: Avoid orphaned objects dependencies, take 3

Hi,

On Tue, Jun 03, 2025 at 01:27:29PM -0400, Robert Haas wrote:
> On Mon, Jun 2, 2025 at 10:02 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > So, we currently have 2 patterns:
> >
> > P1: permission checks on a referenced object is done before we call recordMultipleDependencies()
> > P2: permission checks on a referenced object is done after we call recordMultipleDependencies()
> >
> > So, if we add locking in recordMultipleDependencies() then I think that P2 is worst
> > than P1 (there is still the risk that the permissions changed by the time
> > we reach recordMultipleDependencies() though).
> 
> Hmm. I don't think I agree.

Thanks for sharing your thoughts!

> If I understand correctly, P2 only permits
> users to take a lock on an object they shouldn't be able to touch,
> permitting them to temporarily interfere with access to it. P1 permits
> users to potentially perform a permanent catalog modification that
> should have been blocked by the permissions system. To my knowledge,
> we've never formally classified the former type of problem as a
> security vulnerability, although maybe there's an argument that it is
> one. We've filed CVEs for problems of the latter sort.

What I meant to say is that P2 is worse "by design" because it's "always" wrong
to lock an object we don't have a permission on: so the permission should be
checked first.

So we have:

P1:

 * check_permissions()
 * permissions could change here
 * Lock in recordMultipleDependencies()

P2:

 * Lock in recordMultipleDependencies()
 * FWIW, permissions could change here too (for example, one could still "
revoke usage on schema myschema1 from user1" while there is an AccessShareLock
on schema myschema1)
 * check_permissions()

But P2 sequence of events is "wrong" by design (to lock an object we may not
have permissions on) that's what I meant.

Now if we look at it from a "pure" security angle (as you did) I agree that P1 is 
the worse because it could allow catalog change that should have been blocked by
the permission check.  P2 would prevent that. I agree that we should focus on
P1 then.

Let me try to list the P1 cases (instead of the P2 ones) so that we can focus on
/discuss those. 

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com