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

Wrong userid for process sometimes returned on Linux #970

Closed
nsunderland1 opened this issue Apr 12, 2023 · 7 comments · Fixed by #977
Closed

Wrong userid for process sometimes returned on Linux #970

nsunderland1 opened this issue Apr 12, 2023 · 7 comments · Fixed by #977

Comments

@nsunderland1
Copy link

nsunderland1 commented Apr 12, 2023

Describe the bug

sysinfo version 0.28.3, platform is Amazon Linux 2 (also reproed on Ubuntu)

sysinfo always reports a userid of root on Linux for processes with the dumpable attribute set to a value other than 1. This is because it attempts to get the UID by stat-ing /proc/[pid]/status here and picking the owner of that file. The problem with this is that /proc/[pid]/status isn't always owned by the process's owner. From the docs for proc:

The files inside each /proc/[pid] directory are normally
owned by the effective user and effective group ID of the
process. However, as a security measure, the ownership is
made root:root if the process's "dumpable" attribute is
set to a value other than 1.

See here for docs on a few scenarios in which this flag can get unset.

Note that the /proc/[pid] directory itself is always owned by the process owner, so stat-ing that instead would be a good alternative.

To Reproduce

I put together a small repository with a repro https://github.com/nsunderland1/sysinfo_bug

The meat of it is in main.rs https://github.com/nsunderland1/sysinfo_bug/blob/main/src/main.rs

use std::ops::Deref;

use sysinfo::{ProcessExt, ProcessRefreshKind, RefreshKind, System, SystemExt};

fn main() {
    // The user passes a UID from the command line
    let my_new_uid: u32 = std::env::args().nth(1).unwrap().parse().unwrap();
    // It shouldn't match the current process's effective UID
    assert_ne!(nix::unistd::Uid::effective().as_raw(), my_new_uid);
    // Now use seteuid to change our effective UID to it
    nix::unistd::seteuid(nix::unistd::Uid::from_raw(my_new_uid)).unwrap();
    // Sanity check that it worked
    assert_eq!(nix::unistd::Uid::effective().as_raw(), my_new_uid);

    // Now check the effective UID using sysinfo
    let system = System::new_with_specifics(
        RefreshKind::new().with_processes(ProcessRefreshKind::new().with_user()),
    );

    let my_pid = sysinfo::get_current_pid().unwrap();
    let my_uid_but_wrong = system.process(my_pid).unwrap().user_id().unwrap();

    // This assertion will fail, as sysinfo will report a UID of 0 (root)
    assert_eq!(*my_uid_but_wrong.deref(), my_new_uid);
}
@GuillaumeGomez
Copy link
Owner

Thanks for the detailed report. Interested into sending a fix maybe?

@zeenix
Copy link
Contributor

zeenix commented May 4, 2023

So turns out on Linux, sysinfo gives you the effective UID and GID only, leaving behind real, saved and filesystem IDs. However, on FreeBSD it gives you real IDs instead. :( I actually don't know what the last ones are even so I'd ignore them (until and unless someone asks for them) but it would be good to:

  1. return the same ID for all OS. I think this should be the real ID (looking at all relevant APIs and proc entries on Linux, this seems like the right thing to do) so we should fix the Linux impl.
  2. add API to get the effective IDs (or real if we go with the opposite approach in 1).
  3. document clearly which IDs existing getters give you.

@GuillaumeGomez
Copy link
Owner

It should return real UIDs by default. So the current implementation on linux is wrong. The idea was to add methods to get effective_uid. And yes, making it explicit in the documentation would be really nice!

@zeenix
Copy link
Contributor

zeenix commented May 4, 2023

The idea was to add methods to get effective_uid

Glad we agree. I guess you'd want it to return an Option so on Windows it returns None? FWIW, I think on Windows it should just return the same as uid since it's not that Windows doesn't have effective UID but rather that it does not differentiates between the two. Up to you though.

@GuillaumeGomez
Copy link
Owner

Yes I'd prefer for the effective_uid method to return None on Windows to have one unified API. The documentation on the trait will mention it.

@zeenix
Copy link
Contributor

zeenix commented May 5, 2023

However, on FreeBSD it gives you real IDs instead

While working on this, I realized that I was wrong here. :( FreeBSD impl also returns effective IDs.

However, I still think the current API should return real IDs and we add getters for effective IDs.

@GuillaumeGomez
Copy link
Owner

Agreed once again. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants