Check primary group memberships as well as supplementary group

memberships, and only allow group-writability by groups with exactly one
member, as zero-member groups are typically used by setgid binaries
rather than being user-private groups (closes: #581697).

3 files changed, 69 insertions, 15 deletions

diff --git a/debian/changelog b/debian/changelog
index a27f3b57f..60d985e2f 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -9,6 +9,10 @@ openssh (1:5.5p1-4) UNRELEASED; urgency=low
     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).
+  * Check primary group memberships as well as supplementary group
+    memberships, and only allow group-writability by groups with exactly one
+    member, as zero-member groups are typically used by setgid binaries
+    rather than being user-private groups (closes: #581697).
 
  -- Colin Watson <cjwatson@debian.org>  Tue, 04 May 2010 13:32:17 +0100
 
diff --git a/debian/patches/user-group-modes.patch b/debian/patches/user-group-modes.patch
index 8cf862049..164b8ec81 100644
--- a/debian/patches/user-group-modes.patch
+++ b/debian/patches/user-group-modes.patch
@@ -109,7 +109,7 @@ Index: b/misc.c
  #ifdef SSH_TUN_OPENBSD
  #include <net/if.h>
  #endif
-@@ -638,6 +639,30 @@
+@@ -638,6 +639,55 @@
  }
  
  int
@@ -117,22 +117,47 @@ Index: b/misc.c
 +{
 +       if (st->st_uid != 0 && st->st_uid != uid)
 +               return 0;
++       if ((st->st_mode & 002) != 0)
++               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.
++                * have exactly one member, namely the file's owner.
++                * (Zero-member groups are typically used by setgid
++                * binaries, and are unlikely to be suitable.)
 +                */
-+               struct passwd *pw = getpwuid(st->st_uid);
-+               struct group *gr = getgrgid(st->st_gid);
-+               if (!pw || !gr)
++               struct passwd *pw;
++               struct group *gr;
++               int members = 0;
++
++               gr = getgrgid(st->st_gid);
++               if (!gr)
++                       return 0;
++
++               /* Check primary group memberships. */
++               while ((pw = getpwent()) != NULL) {
++                       if (pw->pw_gid == gr->gr_gid) {
++                               ++members;
++                               if (pw->pw_uid != uid)
++                                       return 0;
++                       }
++               }
++               endpwent();
++
++               pw = getpwuid(st->st_uid);
++               if (!pw)
 +                       return 0;
-+               else if (gr->gr_mem[0]) {
++
++               /* Check supplementary group memberships. */
++               if (gr->gr_mem[0]) {
++                       ++members;
 +                       if (strcmp(pw->pw_name, gr->gr_mem[0]) ||
 +                           gr->gr_mem[1])
 +                               return 0;
 +               }
++
++               if (!members)
++                       return 0;
 +       }
-+       if ((st->st_mode & 002) != 0)
-+               return 0;
 +       return 1;
 +}
 +
diff --git a/misc.c b/misc.c
index 4ca318423..f9da621d6 100644
--- a/misc.c
+++ b/misc.c
@@ -643,22 +643,47 @@ secure_permissions(struct stat *st, uid_t uid)
 {
         if (st->st_uid != 0 && st->st_uid != uid)
                 return 0;
+        if ((st->st_mode & 002) != 0)
+                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.
+                 * have exactly one member, namely the file's owner.
+                 * (Zero-member groups are typically used by setgid
+                 * binaries, and are unlikely to be suitable.)
                  */
-                struct passwd *pw = getpwuid(st->st_uid);
-                struct group *gr = getgrgid(st->st_gid);
-                if (!pw || !gr)
+                struct passwd *pw;
+                struct group *gr;
+                int members = 0;
+
+                gr = getgrgid(st->st_gid);
+                if (!gr)
                         return 0;
-                else if (gr->gr_mem[0]) {
+
+                /* Check primary group memberships. */
+                while ((pw = getpwent()) != NULL) {
+                        if (pw->pw_gid == gr->gr_gid) {
+                                ++members;
+                                if (pw->pw_uid != uid)
+                                        return 0;
+                        }
+                }
+                endpwent();
+
+                pw = getpwuid(st->st_uid);
+                if (!pw)
+                        return 0;
+
+                /* Check supplementary group memberships. */
+                if (gr->gr_mem[0]) {
+                        ++members;
                         if (strcmp(pw->pw_name, gr->gr_mem[0]) ||
                             gr->gr_mem[1])
                                 return 0;
                 }
+
+                if (!members)
+                        return 0;
         }
-        if ((st->st_mode & 002) != 0)
-                return 0;
         return 1;
 }