Thread: Unable to connect to PostgreSQL DB as root user when private key is owned by root with permission 640

"Suralkar, Yogendra (Yogendra)" <suralkary@avaya.com> writes:
> Recently we updated to PostgreSQL 13.7 (Please see list of rpms used below).
> After update we have noticed an issue when connecting to Database as 'root' user when private key file is owned by
rootand has permission 640. 

TBH, my immediate reaction is "what are you doing running database
accesses as root?".  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.

The repeat call of geteuid() is a waste of cycles anyway, so maybe better
like

        if (buf.st_uid != 0 ?
            buf.st_mode & (S_IRWXG | S_IRWXO) :
            buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO))

This feels kind of wrong, in that root's privacy check is now strictly
weaker than anyone else's, but root ought to know what she's doing anyway.

            regards, tom lane

I wrote:
> TBH, my immediate reaction is "what are you doing running database
> accesses as root?".  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.

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.

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 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.

            regards, tom lane

diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index 7a9de524db..2719ce1403 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -160,9 +160,10 @@ check_ssl_key_file_permissions(const char *ssl_key_file, bool isServerStart)
      * allow read access through either our gid or a supplementary gid that
      * allows us to read system-wide certificates.
      *
-     * Note that similar checks are performed in
+     * Note that roughly similar checks are performed in
      * src/interfaces/libpq/fe-secure-openssl.c so any changes here may need
-     * to be made there as well.
+     * to be made there as well.  The environment is different though; this
+     * code can assume that we're not running as root.
      *
      * Ideally we would do similar permissions checks on Windows, but it is
      * not clear how that would work since Unix-style permissions may not be
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 42d8d4616e..95e52690c6 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1373,31 +1373,31 @@ initialize_SSL(PGconn *conn)
         }

         /*
-         * Refuse to load key files owned by users other than us or root, and
-         * require no public access to the key file.  If the file is owned by
-         * us, require mode 0600 or less.  If owned by root, require 0640 or
-         * less to allow read access through either our gid or a supplementary
-         * gid that allows us to read system-wide certificates.
+         * Refuse to load world-readable key files.  We accept root-owned
+         * files with mode 0640 or less, so that we can access system-wide
+         * certificates if we have a supplementary group membership that
+         * allows us to read 'em.  For files with non-root ownership, require
+         * mode 0600 or less.  We need not check the file's ownership per se;
+         * if we're able to read it despite it having such restrictive
+         * permissions, it must have the right ownership.
          *
-         * Note that similar checks are performed in
+         * Note: be very careful about changing these rules.  Some people
+         * expect that a client process running as root should be able to use
+         * a non-root-owned key file.
+         *
+         * Note that roughly similar checks are performed in
          * src/backend/libpq/be-secure-common.c so any changes here may need
-         * to be made there as well.
+         * to be made there as well.  However, this code caters for the case
+         * of current user == root, while that code does not.
          *
          * Ideally we would do similar permissions checks on Windows, but it
          * is not clear how that would work since Unix-style permissions may
          * not be available.
          */
 #if !defined(WIN32) && !defined(__CYGWIN__)
-        if (buf.st_uid != geteuid() && buf.st_uid != 0)
-        {
-            appendPQExpBuffer(&conn->errorMessage,
-                              libpq_gettext("private key file \"%s\" must be owned by the current user or root\n"),
-                              fnbuf);
-            return -1;
-        }
-
-        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)))
+        if (buf.st_uid == 0 ?
+            buf.st_mode & (S_IWGRP | S_IXGRP | S_IRWXO) :
+            buf.st_mode & (S_IRWXG | S_IRWXO))
         {
             appendPQExpBuffer(&conn->errorMessage,
                               libpq_gettext("private key file \"%s\" has group or world access; file must have
permissionsu=rw (0600) or less if owned by the current user, or permissions u=rw,g=r (0640) or less if owned by
root\n"),

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

David Steele <david@pgmasters.net> writes:
> On 5/24/22 16:05, Tom Lane wrote:
>> 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.

Pushed that.

>> 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?

Yeah, I'm unenthused now about touching this in the back branches.
But do we want to do it in HEAD, or just leave well enough alone?

            regards, tom lane

On 5/26/22 2:16 PM, Tom Lane wrote:
> David Steele <david@pgmasters.net> writes:
>> On 5/24/22 16:05, Tom Lane wrote:
>>> 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.
> 
> Pushed that.

Excellent. Thank you!

>>> 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?
> 
> Yeah, I'm unenthused now about touching this in the back branches.
> But do we want to do it in HEAD, or just leave well enough alone?

After thinking on it for a bit I believe we should leave well enough 
alone. This code is rarely touched so I don't think it's a very big deal.

Regards,
-- 
-David
david@pgmasters.net

David Steele <david@pgmasters.net> writes:
> On 5/26/22 2:16 PM, Tom Lane wrote:
>>> * 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?

>> Yeah, I'm unenthused now about touching this in the back branches.
>> But do we want to do it in HEAD, or just leave well enough alone?

> After thinking on it for a bit I believe we should leave well enough 
> alone. This code is rarely touched so I don't think it's a very big deal.

Agreed.  We changed it because we thought the two chunks of code were
satisfying identical constraints, but evidently that's not so.  At least
the situation is better documented now.

            regards, tom lane