Better handling when /etc/group misses an entry for the primary group of the user (#950)

Author: squell

src/common/resolve.rs

@@ -112,7 +112,7 @@ pub(crate) fn resolve_target_user_and_group(
         // when -u is specified but -g is not specified, default -g to the primary group of the specified user
         (Some(_), None) => {
             if let Some(user) = &target_user {
-                target_group = Group::from_gid(user.gid)?;
+                target_group = Some(user.primary_group()?);
             }
         }
         // when no -u or -g is specified, default to root:root
@@ -279,7 +279,7 @@ mod tests {
         // fallback to root
         let (user, group) = resolve_target_user_and_group(&None, &None, &current_user).unwrap();
         assert_eq!(user.name, "root");
-        assert_eq!(group.name, ROOT_GROUP_NAME);
+        assert_eq!(group.name.unwrap(), ROOT_GROUP_NAME);
 
         // unknown user
         let result =
@@ -296,7 +296,7 @@ mod tests {
             resolve_target_user_and_group(&None, &Some(ROOT_GROUP_NAME.into()), &current_user)
                 .unwrap();
         assert_eq!(user.name, current_user.name);
-        assert_eq!(group.name, ROOT_GROUP_NAME);
+        assert_eq!(group.name.unwrap(), ROOT_GROUP_NAME);
 
         // fallback to current users group when no group specified
         let (user, group) =

src/su/context.rs

@@ -99,8 +99,7 @@ impl SuContext {
         }
 
         // resolve target group
-        let mut group =
-            Group::from_gid(user.gid)?.ok_or_else(|| Error::GroupNotFound(user.gid.to_string()))?;
+        let mut group = user.primary_group()?;
 
         if !options.supp_group.is_empty() || !options.group.is_empty() {
             user.groups.clear();

src/sudo/env/tests.rs

@@ -96,7 +96,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
 
     let current_group = Group {
         gid: GroupId::new(1000),
-        name: "test".to_string(),
+        name: Some("test".to_string()),
     };
 
     let root_user = User {
@@ -112,7 +112,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context {
 
     let root_group = Group {
         gid: GroupId::new(0),
-        name: "root".to_string(),
+        name: Some("root".to_string()),
     };
 
     Context {

src/system/interface.rs

@@ -160,7 +160,7 @@ impl UnixGroup for super::Group {
         self.gid
     }
     fn try_as_name(&self) -> Option<&str> {
-        Some(&self.name)
+        self.name.as_deref()
     }
 }
 

src/system/mod.rs

@@ -479,6 +479,12 @@ impl User {
         Self::from_uid(Self::real_uid())
     }
 
+    pub fn primary_group(&self) -> std::io::Result<Group> {
+        // Use from_gid_unchecked here to ensure that we can still resolve when
+        // the /etc/group entry for the primary group is missing.
+        Group::from_gid_unchecked(self.gid)
+    }
+
     pub fn from_name(name_c: &CStr) -> Result<Option<User>, Error> {
         let max_pw_size = sysconf(libc::_SC_GETPW_R_SIZE_MAX).unwrap_or(16_384);
         let mut buf = vec![0; max_pw_size as usize];
@@ -511,7 +517,7 @@ impl User {
 #[cfg_attr(test, derive(PartialEq))]
 pub struct Group {
     pub gid: GroupId,
-    pub name: String,
+    pub name: Option<String>,
 }
 
 impl Group {
@@ -525,11 +531,12 @@ impl Group {
         let name = unsafe { string_from_ptr(grp.gr_name) };
         Group {
             gid: GroupId::new(grp.gr_gid),
-            name,
+            name: Some(name),
         }
     }
 
-    pub fn from_gid(gid: GroupId) -> std::io::Result<Option<Group>> {
+    /// Lookup group for gid without returning an error when a /etc/group entry is missing.
+    fn from_gid_unchecked(gid: GroupId) -> std::io::Result<Group> {
         let max_gr_size = sysconf(libc::_SC_GETGR_R_SIZE_MAX).unwrap_or(16_384);
         let mut buf = vec![0; max_gr_size as usize];
         let mut grp = MaybeUninit::uninit();
@@ -545,13 +552,23 @@ impl Group {
             )
         })?;
         if grp_ptr.is_null() {
-            Ok(None)
+            Ok(Group { gid, name: None })
         } else {
             // SAFETY: grp_ptr was not null, and getgrgid_r succeeded, so we have assurances that
             // the `grp` structure was written to by getgrgid_r
             let grp = unsafe { grp.assume_init() };
             // SAFETY: `pwd` was obtained by a call to getgrXXX_r, as required.
-            Ok(Some(unsafe { Group::from_libc(&grp) }))
+            Ok(unsafe { Group::from_libc(&grp) })
+        }
+    }
+
+    pub fn from_gid(gid: GroupId) -> std::io::Result<Option<Group>> {
+        let group = Self::from_gid_unchecked(gid)?;
+        if group.name.is_none() {
+            // No entry in /etc/group
+            Ok(None)
+        } else {
+            Ok(Some(group))
         }
     }
 
@@ -947,7 +964,7 @@ mod tests {
         for &(id, name) in fixed_groups {
             let root = Group::from_gid(id).unwrap().unwrap();
             assert_eq!(root.gid, id);
-            assert_eq!(root.name, name);
+            assert_eq!(root.name.unwrap(), name);
         }
     }
 
@@ -978,7 +995,7 @@ mod tests {
                     }
                 },
                 Group {
-                    name: name.to_string(),
+                    name: Some(name.to_string()),
                     gid: GroupId::new(gid),
                 }
             )

test-framework/sudo-compliance-tests/src/sudo/misc.rs

@@ -217,13 +217,33 @@ fn does_not_panic_on_io_errors_cli_error() -> Result<()> {
 }
 
 #[test]
-#[ignore = "gh771"]
 fn long_username() -> Result<()> {
     // `useradd` limits usernames to 32 characters
     // directly write to `/etc/passwd` to work around this limitation
     let username = "a".repeat(33);
     let env = Env(SUDOERS_ALL_ALL_NOPASSWD).build()?;
 
+    Command::new("sh")
+        .arg("-c")
+        .arg(format!(
+            "echo {username}:x:1000:1000::/tmp:/bin/sh >> /etc/passwd && echo {username}:x:1000: >> /etc/group"
+        ))
+        .output(&env)?
+        .assert_success()?;
+
+    Command::new("sudo")
+        .arg("-u")
+        .arg(username)
+        .arg("true")
+        .output(&env)?
+        .assert_success()
+}
+
+#[test]
+fn missing_primary_group() -> Result<()> {
+    let username = "user";
+    let env = Env(SUDOERS_ALL_ALL_NOPASSWD).build()?;
+
     Command::new("sh")
         .arg("-c")
         .arg(format!(