Fix memory unsafety in unistd::getgrouplist

[vitalyd]

Fixes #1541

[vitalyd]

I'm not sure about other platforms so this is only for linux. I also skipped checking if ngroups is within ngroups_max in this case, to keep the code simple. The assumption is that linux is consistent between sysconf(NGROUPS_MAX) and its return value here.

[geofft]

It seems to be true on Solaris: https://docs.oracle.com/cd/E86824_01/html/E54766/getgrouplist-3c.html
and on musl but only since 2015: https://git.musl-libc.org/cgit/musl/commit/?id=2894a44b40e460fc4112988407818439f2e9672d

It might be better to detect this instead of trying to hard-code platforms, maybe something like

• store the old ngroups
• call getgrouplist
• if ngroups != old_ngroups, use that, otherwise use 2 * old_ngroups

[vitalyd]

Why don't I revert this portion of the change and save it for another day? I agree that a more principled approach might be better. For now we can focus on just the buffer overrun fix.

[Shnatsel]

Is this good to go?

There is an outstanding security advisory about this issue, but I don't want to alert people to a non-actionable advisory (i.e. with no fixed version to upgrade to) if that can be avoided.

[leifwalsh]

@Shnatsel we believe the change is correct. I'd feel more comfortable with a test, but we're not sure how to write one for this behavior. Do you think it would be reasonable to write a test like

nix/test/test_unistd.rs

Line 205 in 759b34a

fn test_setgroups() {

which adds many many groups before calling getgroups() again?

[leifwalsh]

I tried to reproduce this in a test and had some trouble:

• This test only runs as root
• It's getgrouplist, not getgroups, that has the unsafety
• I couldn't figure out how to temporarily grow the set of groups that getgrouplist ought to return within the scope of a unit test. setgroups didn't change the group list permanently enough that it would affect getgrouplist's behavior, I think setgroups just modifies the current process only, not the group database.

[leifwalsh]

Ok, I tested manually:

for i in `seq 1 64`; do groupadd mygroup$i && gpasswd -a root mygroup$i; done

Then, I wrote this test:

#[test]
// `getgroups()` and `setgroups()` do not behave as expected on Apple platforms
#[cfg(not(any(target_os = "ios", target_os = "macos", target_os = "redox", target_os = "fuchsia")))]
fn test_getgrouplist() {
    let _m = crate::GROUPS_MTX.lock().expect("Mutex got poisoned by another test");

    let username = User::from_uid(Uid::current()).unwrap().unwrap().name;
    let new_groups = getgrouplist(&CString::new(username).unwrap(), Gid::current()).unwrap();
    assert_eq!(new_groups.len(), 65);
}

I ran the test on master, and on master with this patch applied. Without the patch, I get this:

     Running test/test.rs (target/debug/deps/test-cf37cd1023390ea0)

running 1 test
munmap_chunk(): invalid pointer
error: test failed, to rerun pass '--test test'

Caused by:
  process didn't exit successfully: `/workspaces/nix/target/debug/deps/test-cf37cd1023390ea0 test_setgroups_many` (signal: 6, SIGABRT: process abort signal)

With the patch, the test passes.

I think this is ready, if you can think of a way to bundle that into a unit test that can go in CI, I'm open to suggestions, otherwise I recommend we just merge it.

[leifwalsh]

Oh, @Shnatsel sorry, I thought you were a nix maintainer, you came here from the security advisory. I think we need a maintainer to weigh in here.

[asomers]

Thanks for the fix. It looks good. And I agree that it's pretty hard to make an automatic test for this. Can you override /etc/groups by using Linux namespaces? While you mull that over, I'm going to hijack this PR to fix the CHANGELOG and merge.

[asomers]

Sigh, git confusingly pushed master instead of the branch I had checked out. And now github won't let me update the PR again, probably because the source branch is named master. I reopened this as #1545 .

[vitalyd]

@asomers sorry for the git issues. I tried to do this work via the GH webui (didn’t have access to personal computer at the time) and it went a bit sideways. Thanks for reopening and merging under #1545.

And yeah, I couldn’t think of a good automated test for this either. Maybe something can be done if we abstract away the libc call behind some trait, and then assert properties of the parameters passed to a mock/stub impl. But that wouldn’t be testing the real thing anyway.

While on this subject, it might be prudent to add assert!(ngroups <= groups.capacity()) right before the libc call is made. While it would look a bit superfluous with the patched code, given ngroups is set to groups.capacity() right above, it may guard against future refactoring that may violate this invariant. Just a thought.

[asomers]

If we're going to mock out libc, then we should just do it at compile time rather than use a trait. Mockall can do that, and I've thought about doing it some time. But for most libc functions the cost/benefit ratio isn't there.

[leifwalsh]

Possibly you could fake out /etc/groups with namespaces, but that requires a test harness outside cargo test.

One other option I thought of was to do everything that groupadd and gpasswd are doing in Rust code, but I don't know if that's possible.

[vitalyd]

If we're going to mock out libc, then we should just do it at compile time rather than use a trait.

The trait approach would still dispatch statically (via generics). Unit test would just supply a mock receiver for the call; real code would dispatch to libc based one. I’ve not used mockall but quick glance at its docs suggests it’s mocking traits. Just generates the mock for you. Or did I misunderstand you? Are you referring to having a #cfg(test) vs #cfg(not(test)) versions of the fn or something?

Anyway, agree the cost/benefit may not be there. However, perhaps some of the more “hairy” libc functions could benefit from it since one could craft special/infrequent scenarios more easily.

[asomers]

Mockall can work either by mocking traits, or by replacing structs via #[cfg(test)]. In this case, that's the approach I would prefer.