On 5/24/22 16:05, Tom Lane wrote:
> I wrote:
>> TBH, my immediate reaction is "what are you doing running database
>> accesses as root?".
That was my first thought as well. Kind of throws a lot of security out
the window anyway.
>> But given that you are, I see the problem: the test
>> is coded like
>> if ((buf.st_uid == geteuid() && buf.st_mode & (S_IRWXG | S_IRWXO)) ||
>> (buf.st_uid == 0 && buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO)))
>> which was copied verbatim from the equivalent test in the backend.
>> However, in the backend it's safe to assume that geteuid() != 0.
>> libpq apparently shouldn't assume that, meaning that the two arms
>> of the if aren't disjoint cases anymore, and it matters which one
>> we check first.
Indeed it does.
> After further poking at this, I see that we also have to drop the check of
> file ownership. That was already dropped once long ago (3405f2b9253), on
> the grounds that if the file has suitable permissions but its ownership
> isn't what we expect then our read attempt will fail, so we needn't check
> ownership explicitly. While I'd prefer a more explicit error than the
> "Permission denied" that you get with this approach, the intent of this
> patch was not to create any new failure modes, so I think we're stuck
> with that.
That makes sense. Seems I should have dug further into why the server
does this but the client does not.
> Open questions:
>
> * This puts us back into a situation where the frontend and server tests
> are not in sync. Do we want to relax the server's checks to match this,
> or just leave that side as it stands?
I'm inclined to leave it as is in the back branches to avoid any other
unintended consequences. Perhaps we could make the change for PG15?
> * I notice that while the code comments and error messages insist that
> we're checking for 0600/0640 or less, the actual test is not that:
> it doesn't look at S_IXUSR, so it will allow 0700 or 0740. Do we want
> to do anything about that? TBH I'm content to leave it as-is.
> Rejecting S_IXUSR doesn't seem like it buys any useful increment of
> security, and I'm now afraid that somebody will complain that it
> breaks their case that used to work. OTOH, updating the error messages
> and documentation to match the code reality is not terribly attractive
> either; it'll cause a lot of translation churn and probably add more
> confusion than it removes.
I'd rather leave this alone in the back branches as well. Another thing
to consider for PG15 but I agree it does not appear to be a security issue.
Regards,
-David