8 files changed, 165 insertions, 64 deletions

diff --git a/auth-rhosts.c b/auth-rhosts.c
index 06ae7f0b9..f20278797 100644
--- a/auth-rhosts.c
+++ b/auth-rhosts.c
@@ -256,8 +256,7 @@ auth_rhosts2_raw(struct passwd *pw, const char *client_user, const char *hostnam
                 return 0;
         }
         if (options.strict_modes &&
-            ((st.st_uid != 0 && st.st_uid != pw->pw_uid) ||
-            (st.st_mode & 022) != 0)) {
+            !secure_permissions(&st, pw->pw_uid)) {
                 logit("Rhosts authentication refused for %.100s: "
                     "bad ownership or modes for home directory.", pw->pw_name);
                 auth_debug_add("Rhosts authentication refused for %.100s: "
@@ -283,8 +282,7 @@ auth_rhosts2_raw(struct passwd *pw, const char *client_user, const char *hostnam
                  * allowing access to their account by anyone.
                  */
                 if (options.strict_modes &&
-                    ((st.st_uid != 0 && st.st_uid != pw->pw_uid) ||
-                    (st.st_mode & 022) != 0)) {
+                    !secure_permissions(&st, pw->pw_uid)) {
                         logit("Rhosts authentication refused for %.100s: bad modes for %.200s",
                             pw->pw_name, buf);
                         auth_debug_add("Bad file modes for %.200s", buf);
diff --git a/auth.c b/auth.c
index 54f4548f1..a188b891e 100644
--- a/auth.c
+++ b/auth.c
@@ -385,8 +385,7 @@ check_key_in_hostfiles(struct passwd *pw, Key *key, const char *host,
                 user_hostfile = tilde_expand_filename(userfile, pw->pw_uid);
                 if (options.strict_modes &&
                     (stat(user_hostfile, &st) == 0) &&
-                    ((st.st_uid != 0 && st.st_uid != pw->pw_uid) ||
-                    (st.st_mode & 022) != 0)) {
+                    !secure_permissions(&st, pw->pw_uid)) {
                         logit("Authentication refused for %.100s: "
                             "bad owner or modes for %.200s",
                             pw->pw_name, user_hostfile);
@@ -438,8 +437,7 @@ secure_filename(FILE *f, const char *file, struct passwd *pw,
 
         /* check the open file to avoid races */
         if (fstat(fileno(f), &st) < 0 ||
-            (st.st_uid != 0 && st.st_uid != uid) ||
-            (st.st_mode & 022) != 0) {
+            !secure_permissions(&st, uid)) {
                 snprintf(err, errlen, "bad ownership or modes for file %s",
                     buf);
                 return -1;
@@ -455,8 +453,7 @@ secure_filename(FILE *f, const char *file, struct passwd *pw,
 
                 debug3("secure_filename: checking '%s'", buf);
                 if (stat(buf, &st) < 0 ||
-                    (st.st_uid != 0 && st.st_uid != uid) ||
-                    (st.st_mode & 022) != 0) {
+                    !secure_permissions(&st, uid)) {
                         snprintf(err, errlen,
                             "bad ownership or modes for directory %s", buf);
                         return -1;
diff --git a/debian/changelog b/debian/changelog
index 5790f4643..a27f3b57f 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -4,6 +4,12 @@ openssh (1:5.5p1-4) UNRELEASED; urgency=low
   * Add powerpcspe to architecture list for libselinux1-dev build-dependency
     (closes: #579843).
 
+  [ Colin Watson ]
+  * Allow ~/.ssh/authorized_keys and other secure files to be
+    group-writable, provided that the group in question contains only the
+    file's owner; this extends a patch previously applied to ~/.ssh/config
+    (closes: #581919).
+
  -- Colin Watson <cjwatson@debian.org>  Tue, 04 May 2010 13:32:17 +0100
 
 openssh (1:5.5p1-3) unstable; urgency=low
diff --git a/debian/patches/debian-config.patch b/debian/patches/debian-config.patch
index a395d43a0..ac77919e6 100644
--- a/debian/patches/debian-config.patch
+++ b/debian/patches/debian-config.patch
@@ -24,7 +24,7 @@ Index: b/readconf.c
 ===================================================================
 --- a/readconf.c
 +++ b/readconf.c
-@@ -1152,7 +1152,7 @@
+@@ -1132,7 +1132,7 @@
         if (options->forward_x11 == -1)
                 options->forward_x11 = 0;
         if (options->forward_x11_trusted == -1)
diff --git a/debian/patches/user-group-modes.patch b/debian/patches/user-group-modes.patch
index 4d7ebe566..8cf862049 100644
--- a/debian/patches/user-group-modes.patch
+++ b/debian/patches/user-group-modes.patch
@@ -1,10 +1,11 @@
 Description: Allow harmless group-writability
- Allow ~/.ssh/config to be group-writable, provided that the group in
- question contains only the file's owner.  Rejected upstream for IMO
- incorrect reasons (e.g. a misunderstanding about the contents of
- gr->gr_mem).  Given that per-user groups and umask 002 are the default
- setup in Debian (for good reasons - this makes operating in setgid
- directories with other groups much easier), we need to permit this.
+ Allow secure files (~/.ssh/config, ~/.ssh/authorized_keys, etc.) to be
+ group-writable, provided that the group in question contains only the
+ file's owner.  Rejected upstream for IMO incorrect reasons (e.g. a
+ misunderstanding about the contents of gr->gr_mem).  Given that
+ per-user groups and umask 002 are the default setup in Debian (for good
+ reasons - this makes operating in setgid directories with other groups
+ much easier), we need to permit this by default.
 Author: Colin Watson <cjwatson@debian.org>
 Bug: https://bugzilla.mindrot.org/show_bug.cgi?id=1060
 Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=314347
@@ -23,36 +24,13 @@ Index: b/readconf.c
  
  #include "xmalloc.h"
  #include "ssh.h"
-@@ -1000,11 +1002,30 @@
- 
-        if (checkperm) {
-                struct stat sb;
-+               int bad_modes = 0;
+@@ -1003,8 +1005,7 @@
  
                 if (fstat(fileno(f), &sb) == -1)
                         fatal("fstat %s: %s", filename, strerror(errno));
 -               if (((sb.st_uid != 0 && sb.st_uid != getuid()) ||
 -                   (sb.st_mode & 022) != 0))
-+               if (sb.st_uid != 0 && sb.st_uid != getuid())
-+                       bad_modes = 1;
-+               if ((sb.st_mode & 020) != 0) {
-+                       /* If the file is group-writable, the group in
-+                        * question must have at most one member, namely the
-+                        * file's owner.
-+                        */
-+                       struct passwd *pw = getpwuid(sb.st_uid);
-+                       struct group *gr = getgrgid(sb.st_gid);
-+                       if (!pw || !gr)
-+                               bad_modes = 1;
-+                       else if (gr->gr_mem[0]) {
-+                               if (strcmp(pw->pw_name, gr->gr_mem[0]) ||
-+                                   gr->gr_mem[1])
-+                                       bad_modes = 1;
-+                       }
-+               }
-+               if ((sb.st_mode & 002) != 0)
-+                       bad_modes = 1;
-+               if (bad_modes)
++               if (!secure_permissions(&sb, getuid()))
                         fatal("Bad owner or permissions on %s", filename);
         }
  
@@ -82,3 +60,118 @@ Index: b/ssh_config.5
  .It Pa /etc/ssh/ssh_config
  Systemwide configuration file.
  This file provides defaults for those
+Index: b/auth.c
+===================================================================
+--- a/auth.c
++++ b/auth.c
+@@ -385,8 +385,7 @@
+                user_hostfile = tilde_expand_filename(userfile, pw->pw_uid);
+                if (options.strict_modes &&
+                    (stat(user_hostfile, &st) == 0) &&
+-                   ((st.st_uid != 0 && st.st_uid != pw->pw_uid) ||
+-                   (st.st_mode & 022) != 0)) {
++                   !secure_permissions(&st, pw->pw_uid)) {
+                        logit("Authentication refused for %.100s: "
+                            "bad owner or modes for %.200s",
+                            pw->pw_name, user_hostfile);
+@@ -438,8 +437,7 @@
+ 
+        /* check the open file to avoid races */
+        if (fstat(fileno(f), &st) < 0 ||
+-           (st.st_uid != 0 && st.st_uid != uid) ||
+-           (st.st_mode & 022) != 0) {
++           !secure_permissions(&st, uid)) {
+                snprintf(err, errlen, "bad ownership or modes for file %s",
+                    buf);
+                return -1;
+@@ -455,8 +453,7 @@
+ 
+                debug3("secure_filename: checking '%s'", buf);
+                if (stat(buf, &st) < 0 ||
+-                   (st.st_uid != 0 && st.st_uid != uid) ||
+-                   (st.st_mode & 022) != 0) {
++                   !secure_permissions(&st, uid)) {
+                        snprintf(err, errlen,
+                            "bad ownership or modes for directory %s", buf);
+                        return -1;
+Index: b/misc.c
+===================================================================
+--- a/misc.c
++++ b/misc.c
+@@ -45,8 +45,9 @@
+ #include <netdb.h>
+ #ifdef HAVE_PATHS_H
+ # include <paths.h>
+-#include <pwd.h>
+ #endif
++#include <pwd.h>
++#include <grp.h>
+ #ifdef SSH_TUN_OPENBSD
+ #include <net/if.h>
+ #endif
+@@ -638,6 +639,30 @@
+ }
+ 
+ int
++secure_permissions(struct stat *st, uid_t uid)
++{
++       if (st->st_uid != 0 && st->st_uid != uid)
++               return 0;
++       if ((st->st_mode & 020) != 0) {
++               /* If the file is group-writable, the group in question must
++                * have at most one member, namely the file's owner.
++                */
++               struct passwd *pw = getpwuid(st->st_uid);
++               struct group *gr = getgrgid(st->st_gid);
++               if (!pw || !gr)
++                       return 0;
++               else if (gr->gr_mem[0]) {
++                       if (strcmp(pw->pw_name, gr->gr_mem[0]) ||
++                           gr->gr_mem[1])
++                               return 0;
++               }
++       }
++       if ((st->st_mode & 002) != 0)
++               return 0;
++       return 1;
++}
++
++int
+ tun_open(int tun, int mode)
+ {
+ #if defined(CUSTOM_SYS_TUN_OPEN)
+Index: b/misc.h
+===================================================================
+--- a/misc.h
++++ b/misc.h
+@@ -91,4 +91,6 @@
+ int     ask_permission(const char *, ...) __attribute__((format(printf, 1, 2)));
+ int     read_keyfile_line(FILE *, const char *, char *, size_t, u_long *);
+ 
++int     secure_permissions(struct stat *st, uid_t uid);
++
+ #endif /* _MISC_H */
+Index: b/auth-rhosts.c
+===================================================================
+--- a/auth-rhosts.c
++++ b/auth-rhosts.c
+@@ -256,8 +256,7 @@
+                return 0;
+        }
+        if (options.strict_modes &&
+-           ((st.st_uid != 0 && st.st_uid != pw->pw_uid) ||
+-           (st.st_mode & 022) != 0)) {
++           !secure_permissions(&st, pw->pw_uid)) {
+                logit("Rhosts authentication refused for %.100s: "
+                    "bad ownership or modes for home directory.", pw->pw_name);
+                auth_debug_add("Rhosts authentication refused for %.100s: "
+@@ -283,8 +282,7 @@
+                 * allowing access to their account by anyone.
+                 */
+                if (options.strict_modes &&
+-                   ((st.st_uid != 0 && st.st_uid != pw->pw_uid) ||
+-                   (st.st_mode & 022) != 0)) {
++                   !secure_permissions(&st, pw->pw_uid)) {
+                        logit("Rhosts authentication refused for %.100s: bad modes for %.200s",
+                            pw->pw_name, buf);
+                        auth_debug_add("Bad file modes for %.200s", buf);
diff --git a/misc.c b/misc.c
index e1f723123..4ca318423 100644
--- a/misc.c
+++ b/misc.c
@@ -45,8 +45,9 @@
 #include <netdb.h>
 #ifdef HAVE_PATHS_H
 # include <paths.h>
-#include <pwd.h>
 #endif
+#include <pwd.h>
+#include <grp.h>
 #ifdef SSH_TUN_OPENBSD
 #include <net/if.h>
 #endif
@@ -638,6 +639,30 @@ read_keyfile_line(FILE *f, const char *filename, char *buf, size_t bufsz,
 }
 
 int
+secure_permissions(struct stat *st, uid_t uid)
+{
+        if (st->st_uid != 0 && st->st_uid != uid)
+                return 0;
+        if ((st->st_mode & 020) != 0) {
+                /* If the file is group-writable, the group in question must
+                 * have at most one member, namely the file's owner.
+                 */
+                struct passwd *pw = getpwuid(st->st_uid);
+                struct group *gr = getgrgid(st->st_gid);
+                if (!pw || !gr)
+                        return 0;
+                else if (gr->gr_mem[0]) {
+                        if (strcmp(pw->pw_name, gr->gr_mem[0]) ||
+                            gr->gr_mem[1])
+                                return 0;
+                }
+        }
+        if ((st->st_mode & 002) != 0)
+                return 0;
+        return 1;
+}
+
+int
 tun_open(int tun, int mode)
 {
 #if defined(CUSTOM_SYS_TUN_OPEN)
diff --git a/misc.h b/misc.h
index 32073acd4..8fbfc7985 100644
--- a/misc.h
+++ b/misc.h
@@ -91,4 +91,6 @@ char	*read_passphrase(const char *, int);
 int      ask_permission(const char *, ...) __attribute__((format(printf, 1, 2)));
 int      read_keyfile_line(FILE *, const char *, char *, size_t, u_long *);
 
+int      secure_permissions(struct stat *st, uid_t uid);
+
 #endif /* _MISC_H */
diff --git a/readconf.c b/readconf.c
index 487c3399b..2a5a706ab 100644
--- a/readconf.c
+++ b/readconf.c
@@ -1002,30 +1002,10 @@ read_config_file(const char *filename, const char *host, Options *options,
 
         if (checkperm) {
                 struct stat sb;
-                int bad_modes = 0;
 
                 if (fstat(fileno(f), &sb) == -1)
                         fatal("fstat %s: %s", filename, strerror(errno));
-                if (sb.st_uid != 0 && sb.st_uid != getuid())
-                        bad_modes = 1;
-                if ((sb.st_mode & 020) != 0) {
-                        /* If the file is group-writable, the group in
-                         * question must have at most one member, namely the
-                         * file's owner.
-                         */
-                        struct passwd *pw = getpwuid(sb.st_uid);
-                        struct group *gr = getgrgid(sb.st_gid);
-                        if (!pw || !gr)
-                                bad_modes = 1;
-                        else if (gr->gr_mem[0]) {
-                                if (strcmp(pw->pw_name, gr->gr_mem[0]) ||
-                                    gr->gr_mem[1])
-                                        bad_modes = 1;
-                        }
-                }
-                if ((sb.st_mode & 002) != 0)
-                        bad_modes = 1;
-                if (bad_modes)
+                if (!secure_permissions(&sb, getuid()))
                         fatal("Bad owner or permissions on %s", filename);
         }