postgres.git / summary / log / commit / refs
commit a44bc8b8f421d8696eaa796884acb6ed6cc6c670
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu May 26 18:14:05 2022 +0000
First released in: 15.0, 10.22, 11.17, 12.12, 13.8, 14.4
Remove misguided SSL key file ownership check in libpq.
Commits a59c79564 et al. tried to sync libpq's SSL key file
permissions checks with what we've used for years in the backend.
We did not intend to create any new failure cases, but it turns out
we did: restricting the key file's ownership breaks cases where the
client is allowed to read a key file despite not having the identical
UID. In particular a client running as root used to be able to read
someone else's key file; and having seen that I suspect that there are
other, less-dubious use cases that this restriction breaks on some
platforms.
We don't really need an ownership check, since if we can read the key
file despite its having restricted permissions, it must have the right
ownership --- under normal conditions anyway, and the point of this
patch is that any additional corner cases where that works should be
deemed allowable, as they have been historically. Hence, just drop
the ownership check, and rearrange the permissions check to get rid
of its faulty assumption that geteuid() can't be zero. (Note that the
comparable backend-side code doesn't have to cater for geteuid() == 0,
since the server rejects that very early on.)
This does have the end result that the permissions safety check used
for a root user's private key file is weaker than that used for
anyone else's. While odd, root really ought to know what she's doing
with file permissions, so I think this is acceptable.
Per report from Yogendra Suralkar. Like the previous patch,
back-patch to all supported branches.
Discussion: https://postgr.es/m/MW3PR15MB3931DF96896DC36D21AFD47CA3D39@MW3PR15MB3931.namprd15.prod.outlook.com
src/backend/libpq/be-secure-common.c | 5 +++--
src/interfaces/libpq/fe-secure-openssl.c | 34 ++++++++++++++++----------------
2 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index 9af4ff15cd6..a48c38e64a7 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -159,9 +159,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 b5ac0210404..d81cb25a0cf 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1132,31 +1132,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 exactly;
+ * 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 tightening these rules. Some people
+ * expect, for example, 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)
- {
- printfPQExpBuffer(&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))
{
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("private key file \"%s\" 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\n"),
[parent: 0fab33916789]