Re: Unable to connect to PostgreSQL DB as root user when private key is owned by root with permission 640 - Mailing list pgsql-bugs

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"),