-
Notifications
You must be signed in to change notification settings - Fork 486
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
[Cry for help by gitoxide]: Why can this code not determine that the file it just created is indeed owned by it? #1697
Comments
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 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(())
}
} |
@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. |
@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 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 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 (Note that "trust" is a word with a lot of baggage... what specific kinds of "trust" did you mean?) |
I'm not seeing that behavior.
(Probably a bad test as I'm using an unelevated token.) |
Is |
@jonwis Just realized my test was bad for that reason. Thanks! |
Inspired by this suggestion. microsoft/windows-rs#1697
Inspired by this suggestion. microsoft/windows-rs#1697
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. |
Do consider replacing my sample above with a more precise version soon then! We can get you a sample using |
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. |
Glad you got it working! |
Is it not too soon to close this given that @riverar asked me to standby and wait for the precise solution? |
@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! |
Hehe, that's fair enough. I will be subscribing to the related issue and maybe learn from the example code how Thanks a lot for the help :)!. |
Hi @Byron, here's a sample that uses [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(())
}
} |
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 Thanks again for the great support! |
Thanks microsoft/windows-rs#1697 (comment) for figuring this out.
Yep, my bad. I went ahead and lazily patched that up by adding a call to
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 |
Great to hear, I subscribed to the outstanding issue and will happily adjust my code once |
As part of my work on
gitoxide
to incorporate the additionalgit
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:
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.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.
The text was updated successfully, but these errors were encountered: