Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory unsafety in nix::unistd::getgrouplist #1541

Closed
vitalyd opened this issue Sep 27, 2021 · 0 comments · Fixed by vitalyd/nix#1 or #1545
Closed

Memory unsafety in nix::unistd::getgrouplist #1541

vitalyd opened this issue Sep 27, 2021 · 0 comments · Fixed by vitalyd/nix#1 or #1545

Comments

@vitalyd
Copy link
Contributor

vitalyd commented Sep 27, 2021

If libc::getgrouplist returns -1, indicating the supplied groups buffer is too short to hold all the user's groups, the current code will double the buffer and try again. Unfortunately, the ngroups value it passes to libc::getgrouplist doesn't reflect the len of the buffer. After the 1st iteration in this scenario, libc will set ngroups to the # of groups it found, which can be a larger # than the doubling of the group's capacity. The 2nd iteration of the loop will now have a mismatch between group's capacity and the ngroups value it supplies. This will lead to libc writing into unallocated memory beyond the buffer's allocation, leading to a segfault, allocator corruption (e.g. free on destroying the Vec<Gid> complaining of invalid size passed in), or more generally, UB behavior.

The simplest fix is to set ngroups to groups.capacity() on each loop iteration, i.e.:

...
let mut groups = Vec::<Gid>::with_capacity(min(ngroups_max, 8) as usize);
...
loop {
        let mut ngroups = groups.capacity() as i32;
        let ret = unsafe {
            libc::getgrouplist(user.as_ptr(),
                               gid as getgrouplist_group_t,
                               groups.as_mut_ptr() as *mut getgrouplist_group_t,
                               &mut ngroups)
        };
       ...

Separately, since Linux sets ngroups to be the # of groups found (when returning -1), it probably makes sense to reserve that amount in groups rather than doing the doubling capacity dance. But that's a separate issue.

cc @geofft

vitalyd added a commit to vitalyd/nix that referenced this issue Sep 27, 2021
@vitalyd vitalyd reopened this Sep 27, 2021
vitalyd added a commit to vitalyd/nix that referenced this issue Sep 28, 2021
asomers pushed a commit to asomers/nix that referenced this issue Sep 29, 2021
bors bot added a commit that referenced this issue Sep 29, 2021
1538: posix_fadvise doesn't return -1 as sentinel value r=asomers a=ocadaruma

## Summary
- `posix_fadvise(2)` does return error number directly (i.e. not through `errno`)
  * refs: https://man7.org/linux/man-pages/man2/posix_fadvise.2.html , https://man7.org/linux/man-pages/man2/posix_fadvise.2.html
- However `posix_fadvise`-binding uses `Errno::result` to translate the error now, which is mis-use.

1545: Fix memory unsafety in unistd::getgrouplist r=asomers a=asomers

Fixes #1541

1546: Revert "Expose SockAddr::from_raw_sockaddr" r=asomers a=asomers

This reverts commit ed43d2c.

As discussed in #1544 the API of this function needs to change.  For
now, revert the PR that made it public, because it has not yet been
included in any release.

Co-authored-by: Haruki Okada <ocadaruma@gmail.com>
Co-authored-by: vitalyd <vitalyd@gmail.com>
Co-authored-by: Alan Somers <asomers@gmail.com>
@bors bors bot closed this as completed in 1671edc Sep 29, 2021
asomers pushed a commit that referenced this issue Sep 29, 2021
asomers pushed a commit that referenced this issue Sep 29, 2021
asomers pushed a commit that referenced this issue Sep 29, 2021
geofft added a commit to twosigma/nsncd that referenced this issue Oct 7, 2021
geofft added a commit to twosigma/nsncd that referenced this issue Oct 7, 2021
geofft added a commit to twosigma/nsncd that referenced this issue Oct 7, 2021
geofft added a commit to twosigma/nsncd that referenced this issue Oct 7, 2021
geofft added a commit to twosigma/nsncd that referenced this issue Oct 7, 2021
geofft added a commit to twosigma/nsncd that referenced this issue Oct 8, 2021
geofft added a commit to twosigma/nsncd that referenced this issue Oct 8, 2021
geofft added a commit to twosigma/nsncd that referenced this issue Oct 8, 2021
FallenWarrior2k added a commit to FallenWarrior2k/xbgdump that referenced this issue Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant