Validate Windows sandbox token groups before copying SIDs (#45182)

## Why Logon SID lookup previously walked token group entries and SID pointers without checking that they fit within the returned buffer. ## What changed Add a shared `token_groups` helper with a caller-supplied size limit. Validate the group layout, SID bounds, revision, and length before copying SIDs into owned `TokenGroup` values, preserving group order, duplicates, and attributes. Use this helper for logon SID lookup. ## Testing Add Windows tests for owned SID storage, preserved order and attributes, truncated layouts, malformed and out-of-buffer SIDs, query size limits, and current-token logon SID lookup. GitOrigin-RevId: f51ff6aa5266b6c73fe07968015e462c888a95e4
2026-10-30 19:18:49 +00:00 · 2026-09-13 06:43:42 +00:00
parent cfde11a24c
commit e61f381900
2 changed files with 217 additions and 39 deletions
@@ -1,6 +1,7 @@
 use crate::winutil::to_wide;
 use anyhow::Result;
 use anyhow::anyhow;
+use anyhow::ensure;
 use std::ffi::c_void;
 use windows_sys::Win32::Foundation::CloseHandle;
 use windows_sys::Win32::Foundation::ERROR_SUCCESS;
@@ -21,6 +22,7 @@ use windows_sys::Win32::Security::CreateRestrictedToken;
 use windows_sys::Win32::Security::CreateWellKnownSid;
 use windows_sys::Win32::Security::GetLengthSid;
 use windows_sys::Win32::Security::GetTokenInformation;
+use windows_sys::Win32::Security::IsValidSid;
 use windows_sys::Win32::Security::LookupPrivilegeValueW;
 use windows_sys::Win32::Security::SetTokenInformation;

@@ -31,6 +33,7 @@ use windows_sys::Win32::Security::TOKEN_ADJUST_PRIVILEGES;
 use windows_sys::Win32::Security::TOKEN_ADJUST_SESSIONID;
 use windows_sys::Win32::Security::TOKEN_ASSIGN_PRIMARY;
 use windows_sys::Win32::Security::TOKEN_DUPLICATE;
+use windows_sys::Win32::Security::TOKEN_GROUPS;
 use windows_sys::Win32::Security::TOKEN_PRIVILEGES;
 use windows_sys::Win32::Security::TOKEN_QUERY;
 use windows_sys::Win32::Security::TOKEN_USER;
@@ -191,47 +194,100 @@ pub unsafe fn get_current_token_for_restriction() -> Result<HANDLE> {
    Ok(h)
 }

+/// An owned token group, including attributes such as enabled and deny-only.
+#[derive(Debug, PartialEq, Eq)]
+pub(crate) struct TokenGroup {
+    pub sid: Vec<u8>,
+    pub attributes: u32,
+}
+
+/// Queries groups without filtering membership or retaining pointers into Windows' buffer.
+///
+/// # Safety
+/// `token` must remain a valid token handle with `TOKEN_QUERY` access during this call.
+pub(crate) unsafe fn token_groups(token: HANDLE, max_bytes: u32) -> Result<Vec<TokenGroup>> {
+    let mut needed = 0;
+    GetTokenInformation(token, TokenGroups, std::ptr::null_mut(), 0, &mut needed);
+    ensure!(
+        needed > 0 && needed <= max_bytes,
+        "invalid token group size"
+    );
+    let mut buffer = vec![0u8; needed as usize];
+    if GetTokenInformation(
+        token,
+        TokenGroups,
+        buffer.as_mut_ptr().cast(),
+        needed,
+        &mut needed,
+    ) == 0
+    {
+        return Err(anyhow!(
+            "GetTokenInformation(TokenGroups) failed: {}",
+            GetLastError()
+        ));
+    }
+    ensure!(
+        needed as usize <= buffer.len(),
+        "invalid token group result size"
+    );
+    decode_token_groups(&buffer[..needed as usize])
+}
+
+fn decode_token_groups(buffer: &[u8]) -> Result<Vec<TokenGroup>> {
+    let offset = std::mem::offset_of!(TOKEN_GROUPS, Groups);
+    let stride = std::mem::size_of::<SID_AND_ATTRIBUTES>();
+    ensure!(buffer.len() >= offset, "truncated token group header");
+    let count = unsafe { std::ptr::read_unaligned(buffer.as_ptr().cast::<u32>()) } as usize;
+    ensure!(
+        count <= (buffer.len() - offset) / stride,
+        "truncated token groups"
+    );
+    let mut groups = Vec::with_capacity(count);
+    for index in 0..count {
+        let entry = unsafe {
+            std::ptr::read_unaligned(
+                buffer
+                    .as_ptr()
+                    .add(offset + index * stride)
+                    .cast::<SID_AND_ATTRIBUTES>(),
+            )
+        };
+        // Bound the header and every subauthority before calling a SID API.
+        let sid_offset = (entry.Sid as usize).wrapping_sub(buffer.as_ptr() as usize);
+        ensure!(
+            sid_offset <= buffer.len().saturating_sub(8),
+            "invalid token group SID pointer"
+        );
+        ensure!(buffer[sid_offset] == 1, "invalid token group SID revision");
+        let sid_len = 8 + usize::from(buffer[sid_offset + 1]) * 4;
+        ensure!(
+            sid_len <= 68 && sid_len <= buffer.len() - sid_offset,
+            "invalid token group SID size"
+        );
+        ensure!(
+            unsafe { IsValidSid(entry.Sid) } != 0,
+            "invalid token group SID"
+        );
+        let mut sid = vec![0u8; sid_len];
+        ensure!(
+            unsafe { CopySid(sid_len as u32, sid.as_mut_ptr().cast(), entry.Sid) } != 0,
+            "invalid token group SID"
+        );
+        groups.push(TokenGroup {
+            sid,
+            attributes: entry.Attributes,
+        });
+    }
+    Ok(groups)
+}
+
 pub unsafe fn get_logon_sid_bytes(h_token: HANDLE) -> Result<Vec<u8>> {
    unsafe fn scan_token_groups_for_logon(h: HANDLE) -> Option<Vec<u8>> {
-        let mut needed: u32 = 0;
-        GetTokenInformation(h, TokenGroups, std::ptr::null_mut(), 0, &mut needed);
-        if needed == 0 {
-            return None;
-        }
-        let mut buf: Vec<u8> = vec![0u8; needed as usize];
-        let ok = GetTokenInformation(
-            h,
-            TokenGroups,
-            buf.as_mut_ptr() as *mut c_void,
-            needed,
-            &mut needed,
-        );
-        if ok == 0 || (needed as usize) < std::mem::size_of::<u32>() {
-            return None;
-        }
-        let group_count = std::ptr::read_unaligned(buf.as_ptr() as *const u32) as usize;
-        // TOKEN_GROUPS layout is: DWORD GroupCount; SID_AND_ATTRIBUTES Groups[];
-        // On 64-bit, Groups is aligned to pointer alignment after 4-byte GroupCount.
-        let after_count = unsafe { buf.as_ptr().add(std::mem::size_of::<u32>()) } as usize;
-        let align = std::mem::align_of::<SID_AND_ATTRIBUTES>();
-        let aligned = (after_count + (align - 1)) & !(align - 1);
-        let groups_ptr = aligned as *const SID_AND_ATTRIBUTES;
-        for i in 0..group_count {
-            let entry: SID_AND_ATTRIBUTES = std::ptr::read_unaligned(groups_ptr.add(i));
-            if (entry.Attributes & SE_GROUP_LOGON_ID) == SE_GROUP_LOGON_ID {
-                let sid = entry.Sid;
-                let sid_len = GetLengthSid(sid);
-                if sid_len == 0 {
-                    return None;
-                }
-                let mut out = vec![0u8; sid_len as usize];
-                if CopySid(sid_len, out.as_mut_ptr() as *mut c_void, sid) == 0 {
-                    return None;
-                }
-                return Some(out);
-            }
-        }
-        None
+        token_groups(h, u32::MAX)
+            .ok()?
+            .into_iter()
+            .find(|group| group.attributes & SE_GROUP_LOGON_ID == SE_GROUP_LOGON_ID)
+            .map(|group| group.sid)
    }

    if let Some(v) = scan_token_groups_for_logon(h_token) {
@@ -508,3 +564,7 @@ unsafe fn create_token_with_caps_from(
 #[cfg(test)]
 #[path = "token_tests.rs"]
 mod tests;
+
+#[cfg(test)]
+#[path = "token_groups_tests.rs"]
+mod token_groups_tests;
@@ -0,0 +1,118 @@
+//! Exercises owned group decoding and bounded queries without changing token privileges.
+
+use super::*;
+use pretty_assertions::assert_eq;
+use std::os::windows::io::FromRawHandle;
+use std::os::windows::io::OwnedHandle;
+use windows_sys::Win32::System::Threading::OpenProcessToken;
+
+fn group_buffer(groups: &[TokenGroup]) -> Vec<u8> {
+    let offset = std::mem::offset_of!(TOKEN_GROUPS, Groups);
+    let stride = std::mem::size_of::<SID_AND_ATTRIBUTES>();
+    let mut sid_offset = offset + groups.len() * stride;
+    let mut buffer =
+        vec![0u8; sid_offset + groups.iter().map(|group| group.sid.len()).sum::<usize>()];
+    unsafe { std::ptr::write_unaligned(buffer.as_mut_ptr().cast::<u32>(), groups.len() as u32) };
+    for (index, group) in groups.iter().enumerate() {
+        buffer[sid_offset..sid_offset + group.sid.len()].copy_from_slice(&group.sid);
+        let entry = SID_AND_ATTRIBUTES {
+            Sid: unsafe { buffer.as_mut_ptr().add(sid_offset).cast() },
+            Attributes: group.attributes,
+        };
+        unsafe {
+            std::ptr::write_unaligned(
+                buffer
+                    .as_mut_ptr()
+                    .add(offset + index * stride)
+                    .cast::<SID_AND_ATTRIBUTES>(),
+                entry,
+            )
+        };
+        sid_offset += group.sid.len();
+    }
+    buffer
+}
+
+#[test]
+fn owns_sids_and_preserves_duplicate_order_and_attributes() -> Result<()> {
+    let expected = [0x4, 0x10, SE_GROUP_LOGON_ID]
+        .into_iter()
+        .map(|attributes| TokenGroup {
+            sid: unsafe { world_sid().expect("world SID") },
+            attributes,
+        })
+        .collect::<Vec<_>>();
+    let actual = {
+        let buffer = group_buffer(&expected);
+        decode_token_groups(&buffer)?
+    };
+    assert_eq!(actual, expected);
+    Ok(())
+}
+
+#[test]
+fn rejects_truncated_group_layout() {
+    assert!(decode_token_groups(&[]).is_err());
+    let mut buffer = group_buffer(&[]);
+    assert_eq!(
+        decode_token_groups(&buffer).unwrap(),
+        Vec::<TokenGroup>::new()
+    );
+    unsafe { std::ptr::write_unaligned(buffer.as_mut_ptr().cast::<u32>(), u32::MAX) };
+    assert!(decode_token_groups(&buffer).is_err());
+}
+
+#[test]
+fn rejects_malformed_and_out_of_buffer_sids() {
+    let group = TokenGroup {
+        sid: unsafe { world_sid().expect("world SID") },
+        attributes: 4,
+    };
+    let mut buffer = group_buffer(&[group]);
+    let offset = std::mem::offset_of!(TOKEN_GROUPS, Groups);
+    let entry = unsafe {
+        std::ptr::read_unaligned(buffer.as_ptr().add(offset).cast::<SID_AND_ATTRIBUTES>())
+    };
+    let sid_offset = (entry.Sid as usize) - (buffer.as_ptr() as usize);
+    buffer[sid_offset] = 2;
+    assert!(decode_token_groups(&buffer).is_err());
+    buffer[sid_offset] = 1;
+    buffer[sid_offset + 1] = 15;
+    assert!(decode_token_groups(&buffer).is_err());
+    for sid in [std::ptr::null_mut(), unsafe {
+        buffer.as_mut_ptr().add(buffer.len() - 1).cast()
+    }] {
+        unsafe {
+            std::ptr::write_unaligned(
+                buffer.as_mut_ptr().add(offset).cast::<SID_AND_ATTRIBUTES>(),
+                SID_AND_ATTRIBUTES {
+                    Sid: sid,
+                    Attributes: 4,
+                },
+            )
+        };
+        assert!(decode_token_groups(&buffer).is_err());
+    }
+}
+
+#[test]
+fn queries_current_token_with_a_caller_size_limit() -> Result<()> {
+    let mut raw = 0;
+    ensure!(unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut raw) } != 0);
+    let _token = unsafe { OwnedHandle::from_raw_handle(raw as _) };
+    assert!(
+        unsafe {
+            token_groups(raw, /*max_bytes*/ 0)
+        }
+        .is_err()
+    );
+    let groups = unsafe { token_groups(raw, u32::MAX) }?;
+    assert!(!groups.is_empty());
+    if let Some(logon) = groups
+        .iter()
+        .find(|group| group.attributes & SE_GROUP_LOGON_ID == SE_GROUP_LOGON_ID)
+    {
+        assert_eq!(unsafe { get_logon_sid_bytes(raw) }?, logon.sid);
+    }
+    Ok(())
+}