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

Crash on illumos due to different passwd struct definition #91

Closed
sunshowers opened this issue Feb 29, 2024 · 6 comments
Closed

Crash on illumos due to different passwd struct definition #91

sunshowers opened this issue Feb 29, 2024 · 6 comments
Labels
Milestone

Comments

@sunshowers
Copy link

Describe the bug

Hi there, and thanks for maintaining this library!

I'm trying to use a project that depends on whoami, on illumos, and I'm seeing memory corruption occur due to this.

This appears to be because the passwd struct is different on illumos than on other Unixes -- so the getpwuid_r call corrupts memory.

On illumos, the passwd struct is listed here (search for "passwd Structure"):

struct passwd {
    char *pw_name;      /* user's login name */
    char *pw_passwd;    /* no longer used */
    uid_t pw_uid;       /* user's uid */
    gid_t pw_gid;       /* user's gid */
    char *pw_age;       /* not used */
    char *pw_comment;   /* not used */
    char *pw_gecos;     /* typically user's full name */
    char *pw_dir;       /* user's home dir */
    char *pw_shell;     /* user's login shell */
};

But what whoami uses on illumos is defined here:

#[repr(C)]
struct PassWd {
    pw_name: *const c_void,
    pw_passwd: *const c_void,
    pw_uid: u32,
    pw_gid: u32,
    pw_gecos: *const c_void,
    pw_dir: *const c_void,
    pw_shell: *const c_void,
}

To Reproduce

On an illumos system, run:

#[test]
fn test_names() {
    println!("{:?}", whoami::username());
}

Then, run cargo test --release -- --nocapture.

This fails with:

     Running tests/basic.rs (target/release/deps/basic-3cc3589bf34767da)

running 1 test
"unknown"
error: test failed, to rerun pass `--test basic`

Caused by:
  process didn't exit successfully: `/home/rain/dev/whoami/target/release/deps/basic-3cc3589bf34767da --nocapture` (signal: 6, SIGABRT: process abort signal)

Expected behavior

No crash occurs.

Additional context

It looks like the nix library implements this call safely, using the passwd definition in libc here.

Would it be okay to switch the Unix impl of this library to using nix rather than doing this call by hand? That would make it so that whoami would work on any platforms supported by nix and libc (which is roughly all of them).

I'm happy to do the work here if you're okay with it. Thanks!

@AldaronLau
Copy link
Member

@sunshowers Thanks for opening this issue!

Unfortunately, nix has an MSRV of Rust 1.69, which would break whoami's MSRV of Rust 1.40, so it is not an option for this project.

I'd be happy to review and merge a PR if you wanted to do the work fix it.

@sunshowers
Copy link
Author

Thanks for the response. Would you be open to changing the MSRV policy at all? The general ecosystem consensus is that changing the MSRV can be done with a minor version bump (so as part of 1.5.0 in whoami's case).

Thee advantage of that would be that this crate has less unsafe code, instead relying on the already-audited unsafe code in nix. To my mind, that advantage far outweighs MSRV considerations, especially against ancient versions of Rust.

This comment on Zulip may be helpful. Assuming that whoami's usage follows the general patterns of overall crate usage, an MSRV of 1.69 would result in well over 90% of overall fetches being satisfied. And it's quite likely that whoami's largest dependencies have a much more modern MSRV. I briefly looked through the top couple of reverse dependencies and found that they don't actually have an MSRV policy at all, meaning they're only guaranteed to build on the latest version of stable Rust.

I've written a series of patches at https://github.com/oxidecomputer/whoami which switches whoami to using nix. This works very well, and massively reduces the amount of unsafe in whoami.


Another option is to use the libc crate. Currently, libc 0.2 has a fixed MSRV of 1.13. The MSRV policy for the next version of libc, 1.0, is actively being discussed here.

However, while that would fix the immediate issue, it still results in this crate containing a great deal of unsafe code.


In any case, once this issue is addressed we will likely need to put out a RUSTSEC advisory, since this is a stack buffer overflow. I'm happy to work with you on that!

@AldaronLau
Copy link
Member

Thanks for the response. Would you be open to changing the MSRV policy at all? The general ecosystem consensus is that changing the MSRV can be done with a minor version bump (so as part of 1.5.0 in whoami's case).

Considering that I have advertized whoami 1.0 to not break MSRV without a major version bump, any MSRV policy changes would have to be accompanied with the 2.0 release. When whoami 1.0 was released, there was not a general ecosystem consensus on when to change MSRV, as advertizing and supporting an MSRV was just becoming popular.

Thee advantage of that would be that this crate has less unsafe code, instead relying on the already-audited unsafe code in nix. To my mind, that advantage far outweighs MSRV considerations, especially against ancient versions of Rust.

This comment on Zulip may be helpful. Assuming that whoami's usage follows the general patterns of overall crate usage, an MSRV of 1.69 would result in well over 90% of overall fetches being satisfied. And it's quite likely that whoami's largest dependencies have a much more modern MSRV. I briefly looked through the top couple of reverse dependencies and found that they don't actually have an MSRV policy at all, meaning they're only guaranteed to build on the latest version of stable Rust.

I've written a series of patches at https://github.com/oxidecomputer/whoami which switches whoami to using nix. This works very well, and massively reduces the amount of unsafe in whoami.

Makes sense to me. I think it's time to create the v2 branch, and if you open a PR I'll review it for inclusion there.

Another option is to use the libc crate. Currently, libc 0.2 has a fixed MSRV of 1.13. The MSRV policy for the next version of libc, 1.0, is actively being discussed here.

However, while that would fix the immediate issue, it still results in this crate containing a great deal of unsafe code.

I think this could be an option for whoami 1.0 release track, depending on what the policy ends up being.

In any case, once this issue is addressed we will likely need to put out a RUSTSEC advisory, since this is a stack buffer overflow. I'm happy to work with you on that!

Help would be appreciated on that, thank you!

@AldaronLau AldaronLau added the bug label Mar 3, 2024
@AldaronLau AldaronLau added this to the v1.5.0 milestone Mar 3, 2024
@sunshowers
Copy link
Author

Considering that I have advertized whoami 1.0 to not break MSRV without a major version bump, any MSRV policy changes would have to be accompanied with the 2.0 release. When whoami 1.0 was released, there was not a general ecosystem consensus on when to change MSRV, as advertizing and supporting an MSRV was just becoming popular.

Sounds good. Happy to put up a PR once the v2 branch is out. (I'd also have a few other suggestions for that branch, such as making the fallible methods the default ones, and maybe removing the infallible ones while providing an easy way to get defaults like unknown).

It would also be great to figure out an MSRV policy as part of the v2 release. What do you think of something like:

  • best effort, but will occasionally bump if dependencies change
  • a minimum of 6 months of support -- this would easily cross the 90% threshold, based on the data in the Zulip comment. (Rust 1.69 was released on 2023-04-20, so if we bump to that we'll get 10+ months of support.)
  • minor version number must be bumped

And once again, thank you for maintaining this crate. As I was telling my coworker, whoami does something nothing else does -- provide a cross-platform wrapper to data that's generally pretty useful!

@AldaronLau
Copy link
Member

Created different issues for follow-up work based on the discussion here. Closing the issue, since it's fixed on the v1 branch. Release will be tracked at #87

@AldaronLau
Copy link
Member

whoami 1.5.0 just released to crates.io with this fix.

longbowlu added a commit to MystenLabs/sui that referenced this issue Mar 5, 2024
## Description 

```
1380 │ whoami 1.3.0 registry+https://github.com/rust-lang/crates.io-index
     │ ------------------------------------------------------------------ security vulnerability detected
     │
     = ID: RUSTSEC-2024-0020
     = Advisory: https://rustsec.org/advisories/RUSTSEC-2024-0020
     = With older versions of the whoami crate, calling the `username` function leads to an immediate stack
       buffer overflow on illumos and Solaris. Denial of service and data corruption have both been
       observed in the wild, and the issue is possibly exploitable as well.

       This also affects any other Unix platforms that aren't any of: `linux`, `macos`, `freebsd`,
       `dragonfly`, `bitrig`, `openbsd`, `netbsd`.

       This issue has been addressed in whoami 1.5.0.

       For more information, see [this GitHub issue](ardaku/whoami#91).
     = Announcement: ardaku/whoami#91
     = Solution: Upgrade to >=1.5.0 (try `cargo update -p whoami`)
```
## Test Plan 

How did you test the new or updated feature?

---
If your changes are not user-facing and do not break anything, you can
skip the following section. Otherwise, please briefly describe what has
changed under the Release Notes section.

### Type of Change (Check all that apply)

- [ ] protocol change
- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
amnn pushed a commit to MystenLabs/sui that referenced this issue Mar 11, 2024
## Description

```
1380 │ whoami 1.3.0 registry+https://github.com/rust-lang/crates.io-index
     │ ------------------------------------------------------------------ security vulnerability detected
     │
     = ID: RUSTSEC-2024-0020
     = Advisory: https://rustsec.org/advisories/RUSTSEC-2024-0020
     = With older versions of the whoami crate, calling the `username` function leads to an immediate stack
       buffer overflow on illumos and Solaris. Denial of service and data corruption have both been
       observed in the wild, and the issue is possibly exploitable as well.

       This also affects any other Unix platforms that aren't any of: `linux`, `macos`, `freebsd`,
       `dragonfly`, `bitrig`, `openbsd`, `netbsd`.

       This issue has been addressed in whoami 1.5.0.

       For more information, see [this GitHub issue](ardaku/whoami#91).
     = Announcement: ardaku/whoami#91
     = Solution: Upgrade to >=1.5.0 (try `cargo update -p whoami`)
```
## Test Plan

How did you test the new or updated feature?

---
If your changes are not user-facing and do not break anything, you can
skip the following section. Otherwise, please briefly describe what has
changed under the Release Notes section.

### Type of Change (Check all that apply)

- [ ] protocol change
- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
tx-tomcat pushed a commit to tx-tomcat/sui-network that referenced this issue May 30, 2024
## Description 

```
1380 │ whoami 1.3.0 registry+https://github.com/rust-lang/crates.io-index
     │ ------------------------------------------------------------------ security vulnerability detected
     │
     = ID: RUSTSEC-2024-0020
     = Advisory: https://rustsec.org/advisories/RUSTSEC-2024-0020
     = With older versions of the whoami crate, calling the `username` function leads to an immediate stack
       buffer overflow on illumos and Solaris. Denial of service and data corruption have both been
       observed in the wild, and the issue is possibly exploitable as well.

       This also affects any other Unix platforms that aren't any of: `linux`, `macos`, `freebsd`,
       `dragonfly`, `bitrig`, `openbsd`, `netbsd`.

       This issue has been addressed in whoami 1.5.0.

       For more information, see [this GitHub issue](ardaku/whoami#91).
     = Announcement: ardaku/whoami#91
     = Solution: Upgrade to >=1.5.0 (try `cargo update -p whoami`)
```
## Test Plan 

How did you test the new or updated feature?

---
If your changes are not user-facing and do not break anything, you can
skip the following section. Otherwise, please briefly describe what has
changed under the Release Notes section.

### Type of Change (Check all that apply)

- [ ] protocol change
- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants