TLS certificate permission checks

[Russ Allbery]

Julien ÉLIE <julien at trigofacile.com> writes:

> Shouldn't we also check that the key is readable?

This gets a bit tricky, since you can't figure that out solely by checking
ownership and file modes (extended ACLs, for instance).  But we could use
access(R_OK), which would correctly diagnose permission errors.

> I think it was the point of the initial commit in 2002:
>   https://inn.eyrie.org/trac/changeset/6037/trunk/nnrpd/tls.c
> where the check was:

>    !S_ISREG(buf.st_mode) || (buf.st_mode & 0077) != 0 || buf.st_uid != getuid()

> Otherwise, maybe the error appearing in the logs is not clear enough,
> if it does not say that there is a read access issue.

I checked my past email, and this appears to have been all of the original
commentary:

| This patch checks the ownership and permissions of the server's
| private key.  It is conventional for private keys to be regular
| files (not symlinks), owned by the running process, and without
| either group or world access.

> I agree that the new checks in 2011 were probably too restrictive
> for the use case you mention in your mail:
>   https://inn.eyrie.org/trac/changeset/9219/trunk/nnrpd/tls.c

Yeah, we were selectively weakening the check to allow for another common
use case.  But I think it may make sense to just weaken it further to
check for the obvious world-readable case and otherwise just try to open
the file.

How about this?

Index: tls.c
===================================================================
--- tls.c	(revision 10088)
+++ tls.c	(working copy)
@@ -391,15 +391,13 @@
 	    return (0);
 	}
 
-        /* Check that the key file is a real file, not readable by
-         * everyone.  If the mode is 440 or 640, make sure the group owner
-         * is the news group (to prevent the failure case of having news:users
-         * as the owner and group. */
-	if (!S_ISREG(buf.st_mode) || (buf.st_mode & 0137) != 0
-            || ((buf.st_mode & 0040) != 0 && buf.st_gid != getegid())) {
+        /* Check that the key file is a real file, isn't world-readable, and
+         * that we can read it. */
+	if (!S_ISREG(buf.st_mode) || (buf.st_mode & 0007) != 0
+            || access(key_file, R_OK) < 0) {
 	    syslog(L_ERROR, "bad ownership or permissions on private key"
-                   " '%s':  private key must be mode 640 at most, and readable"
-                   " by the news group only", key_file);
+                   " '%s': private key must be a regular file, readable by"
+                   " nnrpd, and not world-readable", key_file);
 	    return (0);
 	}
 
-- 
Russ Allbery (eagle at eyrie.org)              <http://www.eyrie.org/~eagle/>

    Please send questions to the list rather than mailing me directly.
     <http://www.eyrie.org/~eagle/faqs/questions.html> explains why.