Horton

Thread

Commits

  1. Remove misguided SSL key file ownership check in libpq.

  1. 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> — 2022-05-20T05:53:11Z

    PostgreSQL Team,

We are using PostgreSQL 13.3 since last December. We are using SSL based connection to connect to PostgreSQL.

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 root and has permission 640.
psql: error: private key file "/swlibrary/keystore/data_store.pem" has group or world access; file must have permissions u=rw (0600) or less if owned by the current user, or permissions u=rw,g=r (0640) or less if owned by root

When using PostgreSQL 13.3, the file ownership is admin:admin with 600 permission. Most of the operations related to DB are performed by 'admin'. Some operations are performed by 'root' user. So, in 13.3 release both 'admin' and 'root' user were able to communicate with PostgreSQL with this configuration.

root >ls -l /swlibrary/keystore/data_store.pem
-rw-------. 1 admin admin 4600 May 20 10:03 /swlibrary/keystore/data_store.pem
root >export PGDATABASE=avmgmt; export PGUSER=avaya_system_data; export PGSSLCERT=/swlibrary/keystore/data_store.pem; export PGSSLKEY=/swlibrary/keystore/data_store.pem; export PGSSLMODE=verify-ca; export PGSSLROOTCERT=/swlibrary/keystore/default_truststore.pem; /usr/pgsql-13/bin/psql -q -h 127.0.0.1
avmgmt=> select version();
                                                version
--------------------------------------------------------------------------------------------------------
PostgreSQL 13.3 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 8.3.1 20191121 (Red Hat 8.3.1-5), 64-bit
(1 row)

avmgmt=>


After updating the binaries to 13.7, we first saw below error when connecting with root user.

root >ls -l /swlibrary/keystore/data_store.pem
-rw-------. 1 admin admin 4604 May 20 09:52 /swlibrary/keystore/data_store.pem
root >export PGDATABASE=avmgmt; export PGUSER=avaya_system_data; export PGSSLCERT=/swlibrary/keystore/data_store.pem; export PGSSLKEY=/swlibrary/keystore/data_store.pem; export PGSSLMODE=verify-ca; export PGSSLROOTCERT=/swlibrary/keystore/default_truststore.pem; /usr/pgsql-13/bin/psql -q -h 127.0.0.1
psql: error: private key file "/swlibrary/keystore/data_store.pem" must be owned by the current user or root
root >

So, we checked the 13.7 release notes (https://www.postgresql.org/docs/release/13.7/) and found one changelog.

  *   Make libpq accept root-owned SSL private key files (David Steele)

This change synchronizes libpq's rules for safe ownership and permissions of SSL key files with the rules the server has used since release 9.6. Namely, in addition to the current rules, allow the case where the key file is owned by root and has permissions rw-r----- or less. This is helpful for system-wide management of key files.
As per changelog, we should be able to set private key file ownership to root and set 640 permission. We tried this but we are getting below error.

root >ls -l /swlibrary/keystore/data_store.pem
-rw-r-----. 1 root admin 4604 May 20 09:52 /swlibrary/keystore/data_store.pem
root >export PGDATABASE=avmgmt; export PGUSER=avaya_system_data; export PGSSLCERT=/swlibrary/keystore/data_store.pem; export PGSSLKEY=/swlibrary/keystore/data_store.pem; export PGSSLMODE=verify-ca; export PGSSLROOTCERT=/swlibrary/keystore/default_truststore.pem; /usr/pgsql-13/bin/psql -q -h 127.0.0.1
psql: error: private key file "/swlibrary/keystore/data_store.pem" has group or world access; file must have permissions u=rw (0600) or less if owned by the current user, or permissions u=rw,g=r (0640) or less if owned by root
root >
The release notes clearly mention that if the file is owned by root with 640 permission, such use case will be allowed. Even the error says it.


The only way 'root' user can connect to PostgreSQL DB is when the file is owned by root and has permissions 600. But we cannot use this configuration as 'admin' user will not be able to access the private_key

root >ls -l /swlibrary/keystore/data_store.pem
-rw-------. 1 root admin 4604 May 20 09:52 /swlibrary/keystore/data_store.pem
root >export PGDATABASE=avmgmt; export PGUSER=avaya_system_data; export PGSSLCERT=/swlibrary/keystore/data_store.pem; export PGSSLKEY=/swlibrary/keystore/data_store.pem; export PGSSLMODE=verify-ca; export PGSSLROOTCERT=/swlibrary/keystore/default_truststore.pem; /usr/pgsql-13/bin/psql -q -h 127.0.0.1
avmgmt=> select version();
                                                 version
---------------------------------------------------------------------------------------------------------
PostgreSQL 13.7 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-10), 64-bit
(1 row)


The only log I see is below line. (I had set log_error_verbosity = verbose in postgresql.conf file)
May 20 11:12:56 smgr247 postgres[1712491]: [17-1] 2022-05-20 11:12:56.516 IST [1712491] LOG:  could not accept SSL connection: Success



13.7 rpm used
https://download.postgresql.org/pub/repos/yum/13/redhat/rhel-8-x86_64/postgresql13-13.7-1PGDG.rhel8.x86_64.rpm
https://download.postgresql.org/pub/repos/yum/13/redhat/rhel-8-x86_64/postgresql13-server-13.7-1PGDG.rhel8.x86_64.rpm
https://download.postgresql.org/pub/repos/yum/13/redhat/rhel-8-x86_64/postgresql13-contrib-13.7-1PGDG.rhel8.x86_64.rpm
https://download.postgresql.org/pub/repos/yum/13/redhat/rhel-8-x86_64/postgresql13-libs-13.7-1PGDG.rhel8.x86_64.rpm

Platform - Red Hat Enterprise Linux release 8.4 (Ootpa)

If you require any more information please do let us know.

P.S. - We have tried update to 13.6 release and we do not see this issue.

Regards,
Yogendra

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-20T14:49:46Z

    "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 root and 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-24T20:05:32Z

    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

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

    David Steele <david@pgmasters.net> — 2022-05-24T20:33:12Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-26T18:16:21Z

    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

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

    David Steele <david@pgmasters.net> — 2022-05-26T20:01:49Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-26T21:35:13Z

    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