[Cry for help by gitoxide]: Why can this code not determine that the file it just created is indeed owned by it?

[Byron]

As part of my work on gitoxide to incorporate the additional git security protocols I am trying to implement an ownership check for windows to assure that gitoxide will not fully trust repositories that aren't owned by the user executing the process.

This is the idea:

        pub fn is_path_owned_by_current_user(path: Cow<'_, Path>) -> std::io::Result<bool> {
            fn from_path(path: Cow<'_, Path>) -> std::io::Result<u32> {
                use std::os::unix::fs::MetadataExt;
                let meta = std::fs::symlink_metadata(path)?;
                Ok(meta.uid())
            }

            fn from_process() -> std::io::Result<u32> {
                // SAFETY: there is no documented possibility for failure
                #[allow(unsafe_code)]
                let uid = unsafe { libc::geteuid() };
                Ok(uid)
            }

            Ok(from_path(path)? == from_process()?)
        }

However, when trying to implement the same for windows very much similarly as git itself the following test does not succeed despite hours of trying.

let dir = tempfile::tempdir()?;
assert!(git_sec::identity::is_path_owned_by_current_user(dir.path().into())?);

I am reaching out in the hopes that someone can point me to the issue with the code causing the failure. It's terrible, but maybe there is a way to get it to work nonetheless.

Thanks for your consideration.

[riverar]

No more crying @Byron! Here's one way you can check if the current process effective token contains the SID matching the owner of the demo directory on disk.

Thanks for asking, as we identified a metadata bug along the way. (microsoft/win32metadata#884)

[package]
name = "app"
version = "0.0.0"
edition = "2021"
publish = false

[dependencies.windows]
version = "0.35"
features = [
    "alloc",
    "Win32_Foundation",
    "Win32_Security_Authorization",
    "Win32_Storage_FileSystem",
    "Win32_System_Memory",
    "Win32_System_Threading",
]

use windows::{
    core::{Error, PCSTR},
    Win32::{
        Foundation::{BOOL, ERROR_SUCCESS, HANDLE, PSID},
        Security::{
            Authorization::{GetNamedSecurityInfoA, SE_FILE_OBJECT},
            CheckTokenMembershipEx, OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR,
        },
        System::Memory::LocalFree,
    },
};

fn main() -> windows::core::Result<()> {
    unsafe {
        let demo_path = std::path::Path::new("demo");
        if !demo_path.exists() {
            std::fs::create_dir(demo_path).unwrap();
        }

        let mut psid = PSID::default();
        let mut pdescriptor = PSECURITY_DESCRIPTOR::default();
        let result = GetNamedSecurityInfoA(
            PCSTR(b".\\demo\0".as_ptr()),
            SE_FILE_OBJECT,
            OWNER_SECURITY_INFORMATION,
            &mut psid,
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            &mut pdescriptor,
        );

        // Workaround for https://github.com/microsoft/win32metadata/issues/884
        if result != ERROR_SUCCESS.0 {
            return Err(Error::from_win32());
        }

        let mut is_member = BOOL(0);
        CheckTokenMembershipEx(HANDLE::default(), psid, 0, &mut is_member).ok()?;
        if is_member.as_bool() {
            println!(r"User owns \demo directory.");
        } else {
            println!(r"User does not own \demo directory.");
        }

        LocalFree(pdescriptor.0 as isize);
        Ok(())
    }
}

[kennykerr]

@danielframpton / @sivadeilra may know if there's a reliable (e.g. works on Windows) way to do this with the Rust standard library. @jonwis may have additional API suggestions.

[jonwis]

@riverar 's answer is for the question "is the current token a member of a group that is the owner of the file." So if the ownership of a file was assigned to YourDomain\MuffinMakers (a group containing all the principals that make muffins for the domain) and was being run by YourDomain\Sally, the method returns "true" as long as Sally is a muffin maker.

If you were looking for a more precise answer, like "is this directory owned by the owner of this process", then you'd want to GetTokenInformation for TokenOwner and look at the TOKEN_OWNER structure's Owner field.

On rereading the motivation - should this user trust the files in this directory - the group membership check is probably right. A domain could have a rule saying that cloned repos must be owned by YourDomain\DevGroup5 and as long as Sally is also a member of DevGroup5, she should trust it.

(Note that "trust" is a word with a lot of baggage... what specific kinds of "trust" did you mean?)

[riverar]

@riverar 's answer is for the question "is the current token a member of a group that is the owner of the file." So if the ownership of a file was assigned to YourDomain\MuffinMakers (a group containing all the principals that make muffins for the domain) and was being run by YourDomain\Sally, the method returns "true" as long as Sally is a muffin maker.

I'm not seeing that behavior.

> dir /q
...
04/15/2022  10:33 AM    <DIR>          BUILTIN\Administrators demo
...

> cargo run
User does not own \demo directory.

(Probably a bad test as I'm using an unelevated token.)

[jonwis]

Is cargo run running with an elevated token? An un-elevated token has the Administrator group present, but disabled for purposes of "are you a muffin maker"

[riverar]

@jonwis Just realized my test was bad for that reason. Thanks!

[Byron]

wiping away the tears Thanks a lot for your swift help, @riverar, that does indeed work and CI is now green. The code is also much more readable what I had before (source code link for excerpt below)

pub fn is_path_owned_by_current_user(path: Cow<'_, Path>) -> std::io::Result<bool> {
            use windows::Win32::{
                Foundation::{CloseHandle, ERROR_SUCCESS, HANDLE, PSID},
                Security,
                Security::Authorization::SE_FILE_OBJECT,
                System::Threading,
            };
            let mut handle = HANDLE::default();
            let mut descriptor = Security::PSECURITY_DESCRIPTOR::default();
            let mut err_msg = None;
            let mut is_owned = false;

            #[allow(unsafe_code)]
            unsafe {
                Threading::OpenProcessToken(Threading::GetCurrentProcess(), Security::TOKEN_QUERY, &mut handle)
                    .ok()
                    .map_err(|_| err("Failed to open process token"))?;

                let mut len = 0_u32;
                if !Security::GetTokenInformation(handle, Security::TokenUser, std::ptr::null_mut(), 0, &mut len)
                    .as_bool()
                {
                    let mut info_buf = Vec::<u8>::new();
                    info_buf.reserve_exact(len as usize);
                    if Security::GetTokenInformation(
                        handle,
                        Security::TokenUser,
                        info_buf.as_mut_ptr() as *mut std::ffi::c_void,
                        len,
                        &mut len,
                    )
                    .as_bool()
                    {
                        // NOTE: we avoid to copy the sid or cache it in any way for now, even though it should be possible
                        //       with a custom allocation/vec/box and it's just very raw. Can the `windows` crate do better?
                        //       When/If yes, then let's improve this.
                        //       It should however be possible to create strings from SIDs, check this once more.
                        let info: *const Security::TOKEN_USER = std::mem::transmute(info_buf.as_ptr());
                        if Security::IsValidSid((*info).User.Sid).as_bool() {
                            let wide_path = to_wide_path(&path);
                            let mut path_sid = PSID::default();
                            let res = Security::Authorization::GetNamedSecurityInfoW(
                                windows::core::PCWSTR(wide_path.as_ptr()),
                                SE_FILE_OBJECT,
                                Security::OWNER_SECURITY_INFORMATION | Security::DACL_SECURITY_INFORMATION,
                                &mut path_sid as *mut _,
                                std::ptr::null_mut(),
                                std::ptr::null_mut(),
                                std::ptr::null_mut(),
                                &mut descriptor as *mut _,
                            );

                            if res == ERROR_SUCCESS.0 && Security::IsValidSid(path_sid).as_bool() {
                                is_owned = Security::EqualSid(path_sid, (*info).User.Sid).as_bool();
                                dbg!(is_owned, path.as_ref());
                            } else {
                                err_msg = format!("couldn't get owner for path or it wasn't valid: {}", res).into();
                            }
                        } else {
                            err_msg = String::from("owner id of current process wasn't set or valid").into();
                        }
                    } else {
                        err_msg = String::from("Could not get information about the token user").into();
                    }
                } else {
                    err_msg = String::from("Could not get token information for length of token user").into();
                }
                CloseHandle(handle);
                if !descriptor.is_invalid() {
                    windows::core::heap_free(descriptor.0);
                }
            }
            err_msg.map(|msg| Err(err(msg))).unwrap_or(Ok(is_owned))
        }

The intend of the above is indeed to compare actual ownership, which is how git does it. That's why ultimately I'd love to do it with the same semantics even though I certainly don't care how.

@jonwis already mentioned that the above would be more inline with the semantics that were originally intended, and I'd love to get that to work even though for some reason it didn't.

Now I am with this implementation which serves as a stepping stone towards the ultimate goal of being 'git-compliant' :D.

The question remains, is there a way to check for actual ownership and can this be made to work?

PS: Right now testing for me is a bit painful as it has to happen on CI, but I am trying to get a Windows for ARM instance running within Parallels which should speed up this kind of development for me.
PS2: In case you are interested, my initial experience with the windows crate was positive especially compared to winapi , but I'd love to see more documentation on the basics. Right now most of what I found in the README was TODO.

[riverar]

Do consider replacing my sample above with a more precise version soon then! We can get you a sample using GetTokenInformation but first we need to fix the metadata describing this function. It's a bit busted and makes working with it very difficult. Standby.

[Byron]

By the way, I just successfully ran the test on windows with the GNU toolchain even, neat! I highlight this because I was surprised that it was 'so easy', at least after the msvc Rust toolchain failed due to MSVC not being installed.

Edit: I just realized that it installed the x86_64 toolchain despite being on ARM, and it worked nonetheless. This must be an issue with Rustup which should detect it's an ARM environment.
Edit2: It seems like rustup.exe is an x86 application which is why it defaults to these architectures. There is an aarch64-pc-windows-msvc target, but no GNU version of it. All of this is tangential, maybe it's interesting to some.

[kennykerr]

Glad you got it working!

[Byron]

Is it not too soon to close this given that @riverar asked me to standby and wait for the precise solution?

[riverar]

@Byron We keep a tight ship around here. That is, if there's no work needed on the crate directly, the issue is closed. That doesn't prevent us from continuing the discussion!

[Byron]

Hehe, that's fair enough. I will be subscribing to the related issue and maybe learn from the example code how GetTokenInformation can be used.

Thanks a lot for the help :)!.

[riverar]

Hi @Byron, here's a sample that uses GetTokenInformation instead.

[dependencies.windows]
version = "0.35"
features = [
    "alloc",
    "Win32_Foundation",
    "Win32_Security_Authorization",
    "Win32_Storage_FileSystem",
    "Win32_System_Memory",
    "Win32_System_Threading",
]

use windows::{
    core::{Error, PCSTR},
    Win32::{
        Foundation::{ERROR_SUCCESS, HANDLE, PSID},
        Security::{
            Authorization::{GetNamedSecurityInfoA, SE_FILE_OBJECT},
            EqualSid, GetTokenInformation, TokenOwner, OWNER_SECURITY_INFORMATION,
            PSECURITY_DESCRIPTOR, TOKEN_OWNER, TOKEN_QUERY,
        },
        System::{
            Memory::LocalFree,
            Threading::{GetCurrentProcess, OpenProcessToken},
        },
    },
};

fn main() -> windows::core::Result<()> {
    unsafe {
        let demo_path = std::path::Path::new("demo");
        if !demo_path.exists() {
            std::fs::create_dir(demo_path).unwrap();
        }

        let mut folder_owner = PSID::default();
        let mut pdescriptor = PSECURITY_DESCRIPTOR::default();
        let result = GetNamedSecurityInfoA(
            PCSTR(b".\\demo\0".as_ptr()),
            SE_FILE_OBJECT,
            OWNER_SECURITY_INFORMATION,
            &mut folder_owner,
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            std::ptr::null_mut(),
            &mut pdescriptor,
        );

        // Workaround for https://github.com/microsoft/win32metadata/issues/884
        if result != ERROR_SUCCESS.0 {
            return Err(Error::from_win32());
        }

        let mut token = HANDLE::default();
        OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token).ok()?;

        let mut buffer_size = 0;
        let mut buffer = Vec::<u8>::new();
        let _ = GetTokenInformation(token, TokenOwner, std::ptr::null_mut(), 0, &mut buffer_size);
        if buffer_size == 0 {
            LocalFree(pdescriptor.0 as isize);
            return Err(Error::from_win32());
        }

        buffer.resize(buffer_size as usize, 0);
        GetTokenInformation(
            token,
            TokenOwner,
            buffer.as_mut_ptr() as _,
            buffer_size,
            &mut buffer_size,
        )
        .ok()?;

        let token_owner = buffer.as_ptr() as *const TOKEN_OWNER;
        let token_owner = (*token_owner).Owner;

        if EqualSid(folder_owner, token_owner).as_bool() {
            println!(r"User owns \demo directory.");
        } else {
            println!(r"User does not own \demo directory.");
        }

        LocalFree(pdescriptor.0 as isize);
        Ok(())
    }
}

[Byron]

Thanks a lot @riverar, this works perfectly!

Maybe that's good to have as example as well. Is it true or am I imagining that pdescriptor is leaked in case there is an error along the way causing an early exit? If that's indeed the case, I wonder how difficult it would be to provide some sort of 'auto-free' wrappers that do this on drop. It's more of a rhetorical question, I wouldn't want to hijack this thread which now is fully resolved as far as I am concerned.

Thanks again for the great support!

[riverar]

@Byron

Is it true or am I imagining that pdescriptor is leaked in case there is an error along the way causing an early exit?

Yep, my bad. I went ahead and lazily patched that up by adding a call to LocalFree in the early exit scenario. Perhaps you could move things around and get the token SID first, or impl some fancier Drop semantics to avoid this issue altogether.

I wonder how difficult it would be to provide some sort of 'auto-free' wrappers that do this on drop.

Upstream metadata is working on resolving some outstanding issues around annotating Win32 types with information about how they need to be destructed or dropped. Once that's resolved, you should see that trickle down to the the windows crate in the form of impl Drop.

[Byron]

Great to hear, I subscribed to the outstanding issue and will happily adjust my code once Drop semantics trickle down.