public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Steele <[email protected]>
To: PostgreSQL Developers <[email protected]>
Subject: Allow root ownership of client certificate key
Date: Fri, 22 Oct 2021 11:41:21 -0400
Message-ID: <[email protected]> (raw)
Hackers,
I noticed recently that permissions checking is done differently for the
server certificate key than the client key. Specifically, on the server
the key can have 640 perms if it is owned by root.
On the server side this change was made in 9a83564c and I think the same
rational applies equally well to the client key. At the time managed
keys on the client may not have been common but they are now.
Attached is a patch to make this change.
I was able to this this manually by hacking 001_ssltests.pl like so:
- chmod 0640, "ssl/${key}_tmp.key"
+ chmod 0600, "ssl/${key}_tmp.key"
or die "failed to change permissions on ssl/${key}_tmp.key: $!";
- system_or_bail("sudo chown root ssl/${key}_tmp.key");
But this is clearly not going to work for general purpose testing. The
server keys also not tested for root ownership so perhaps we do not need
that here either.
I looked at trying to make this code common between the server and
client but due to the differences in error reporting it seemed like more
trouble than it was worth.
Regards,
--
-David
[email protected]
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 3a7cc8f774..285e772170 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1234,11 +1234,38 @@ initialize_SSL(PGconn *conn)
fnbuf);
return -1;
}
+
+ /*
+ * Refuse to load key files owned by users other than us or root.
+ *
+ * XXX surely we can check this on Windows somehow, too.
+ */
+#ifndef WIN32
+ 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;
+ }
+#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.
+ */
#ifndef WIN32
- if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))
+ 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;
}
Attachments:
[text/plain] client-key-perm-001.patch (1.8K, ../[email protected]/2-client-key-perm-001.patch)
download | inline diff:
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 3a7cc8f774..285e772170 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1234,11 +1234,38 @@ initialize_SSL(PGconn *conn)
fnbuf);
return -1;
}
+
+ /*
+ * Refuse to load key files owned by users other than us or root.
+ *
+ * XXX surely we can check this on Windows somehow, too.
+ */
+#ifndef WIN32
+ 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;
+ }
+#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.
+ */
#ifndef WIN32
- if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))
+ 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;
}
view thread (4+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Allow root ownership of client certificate key
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox