Hi Tom,
On 1/18/22 14:41, Tom Lane wrote:
David Steele <da...@pgmasters.net> writes:
[ client-key-perm-002.patch ]
I took a quick look at this and agree with the proposed behavior
change, but also with your self-criticisms:
We may want to do the same on the server side to make the code blocks
look more similar.
Also, on the server side the S_ISREG() check gets its own error and that
might be a good idea on the client side as well. As it is, the error
message on the client is going to be pretty confusing in this case.
Particularly, I think the S_ISREG check should happen before any
ownership/permissions checks; it just seems saner that way.
The two blocks of code now look pretty much identical except for error
handling and the reference to the other file. Also, the indentation for
the comment on the server side is less but I kept the comment formatting
the same to make it easier to copy the comment back and forth.
The only other nitpick I have is that I'd make the cross-references be
to the two file names, ie like "Note that similar checks are performed
in fe-secure-openssl.c ..." References to the specific functions seem
likely to bit-rot in the face of future code rearrangements.
I suppose filename references could become obsolete too, but it
seems less likely.
Updated these to reference the file instead of the function.
I still don't think we can commit the test for root ownership, but
testing it manually got a whole lot easier after the refactor in
c3b34a0f. Before that you had to hack up the source tree, which is a
pain depending on how it is mounted (I'm testing in a container).
So, to test the new functionality, just add this snippet on line 57 of
001_ssltests.pl:
chmod 0640, "$cert_tempdir/client.key"
or die "failed to change permissions on $cert_tempdir/client.key: $!";
system_or_bail("sudo chown root $cert_tempdir/client.key");
If you can think of a way to add this to the tests I'm all ears. Perhaps
we could add these lines commented out and explain what they are for?
Regards,
-David
diff --git a/src/backend/libpq/be-secure-common.c
b/src/backend/libpq/be-secure-common.c
index 7e9a64d08e..3f00040efe 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -143,6 +143,7 @@ check_ssl_key_file_permissions(const char *ssl_key_file,
bool isServerStart)
return false;
}
+ /* Key file must be a regular file */
if (!S_ISREG(buf.st_mode))
{
ereport(loglevel,
@@ -153,10 +154,20 @@ check_ssl_key_file_permissions(const char *ssl_key_file,
bool isServerStart)
}
/*
- * Refuse to load key files owned by users other than us or root.
- *
- * XXX surely we can check this on Windows somehow, too.
- */
+ * 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 our gid, or a supplementary gid that allows
+ * us to read system-wide certificates.
+ *
+ * Note that similar checks are performed in
+ * src/interfaces/libpq/fe-secure-openssl.c so any changes here may need
to
+ * be made there as well.
+ *
+ * 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)
{
@@ -166,20 +177,7 @@ check_ssl_key_file_permissions(const char *ssl_key_file,
bool isServerStart)
ssl_key_file)));
return false;
}
-#endif
- /*
- * Require no public access to 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 our gid, or a supplementary gid that allows
- * to read system-wide certificates.
- *
- * XXX temporarily suppress check when on Windows, because there may not
- * be proper support for Unix-y file permissions. Need to think of a
- * reasonable check to apply on Windows. (See also the data directory
- * permission check in postmaster.c)
- */
-#if !defined(WIN32) && !defined(__CYGWIN__)
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)))
{
diff --git a/src/interfaces/libpq/fe-secure-openssl.c
b/src/interfaces/libpq/fe-secure-openssl.c
index f6e563a2e5..59e3224f97 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1245,11 +1245,45 @@ initialize_SSL(PGconn *conn)
fnbuf);
return -1;
}
+
+ /* Key file must be a regular file */
+ if (!S_ISREG(buf.st_mode))
+ {
+ appendPQExpBuffer(&conn->errorMessage,
+
libpq_gettext("private key file \"%s\" is not a regular file"),
+ fnbuf);
+ return -1;
+ }
+
+ /*
+ * 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 our gid, or a supplementary gid
that allows
+ * us to read system-wide certificates.
+ *
+ * Note that similar checks are performed in
+ * src/backend/libpq/be-secure-common.c so any changes here may
need to
+ * be made there as well.
+ *
+ * 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.
+ */
#ifndef WIN32
- if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))
+ 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)))
{
appendPQExpBuffer(&conn->errorMessage,
-
libpq_gettext("private key file \"%s\" has group or world access; permissions
should be u=rw (0600) or less\n"),
+
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"),
fnbuf);
return -1;
}
