Christoph Berg wrote:
> Re: To Tom Lane 2016-02-19 <20160219115334.gb26...@msg.df7cb.de>
> > Updated patch attached.
>
> *Blush* I though I had compile-tested the patch, but without
> --enable-openssl it wasn't built :(.
>
> The attached patch has successfully been included in the 9.6 Debian
> package, passed the regression tests there, and I've also done some
> chmod/chown tests on the filesystem to verify it indeed catches the
> cases laid out by Tom.
This looks like a pretty reasonable change to me.
For the sake of cleanliness, I propose splitting out the checks for
regular file and for owned-by-root-or-us from the actual chmod-level
checks at the same time. That way we can provide more specific error
messages for each case. (Furthermore, I'm pretty sure that the check
for superuserness could be applied on Windows also -- in the attached
patch it's still #ifdef'ed out, because I don't know how to write it.)
After doing that change I started to look at the details of the check
and found some mistakes:
* it said "g=w" instead of "g=r", in contradiction with the numeric
specification: g=w means 020 rather than 040. We want the file to be
group-readable, not group-writeable.
* it failed to check for S_IXUSR, so permissions 0700 were okay, in
contradiction with what the error message indicates. This is a
preexisting bug actually. Do we want to fix it by preventing a
user-executable file (possibly breaking compability with existing
executable key files), or do we want to document what the restriction
really is?
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 1e3dfb6..1330845 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -206,8 +206,30 @@ be_tls_init(void)
errmsg("could not access private key file \"%s\": %m",
ssl_key_file)));
+ if (!S_ISREG(buf.st_mode))
+ ereport(FATAL,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("private key file \"%s\" is not a regular file",
+ ssl_key_file)));
+
/*
- * Require no public access to key file.
+ * Refuse to load files owned by users other than us or root.
+ *
+ * XXX surely we can check this on Windows somehow, too.
+ */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+ if (buf.st_uid != geteuid() && buf.st_uid != 0)
+ ereport(FATAL,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("private key file \"%s\" must be owned by the database user or root",
+ ssl_key_file)));
+#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
@@ -215,12 +237,13 @@ be_tls_init(void)
* directory permission check in postmaster.c)
*/
#if !defined(WIN32) && !defined(__CYGWIN__)
- if (!S_ISREG(buf.st_mode) || buf.st_mode & (S_IRWXG | S_IRWXO))
+ if ((buf.st_uid == geteuid() && buf.st_mode & (S_IXUSR | S_IRWXG | S_IRWXO)) ||
+ (buf.st_uid == 0 && buf.st_mode & (S_IXUSR | S_IWGRP | S_IXGRP | S_IRWXO)))
ereport(FATAL,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("private key file \"%s\" has group or world access",
- ssl_key_file),
- errdetail("Permissions should be u=rw (0600) or less.")));
+ errmsg("private key file \"%s\" has group or world access",
+ ssl_key_file),
+ errdetail("File must have permissions u=rw (0600) or less if owned by the datbase user, or permissions u=rw,g=r (0640) or less if owned by root.")));
#endif
if (SSL_CTX_use_PrivateKey_file(SSL_context,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
