Skip to content

Use ntdll.dll rather than KERNEL32.dll to intercept windows library calls #51

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/LibAFL/libafl_frida/src/syscall_isolation_rt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ impl SyscallIsolationRuntime {
// println!("{:#?}", modules_info());
// let lib = Module::enumerate_modules()
// .into_iter()
// .find(|m| m.name.contains("KERNEL32"))
// .find(|m| m.name.contains("ntdll.dll"))
// .ok_or(Error::unknown(format!(
// "lib {} not found in modules",
// "kernel"
Expand Down
3 changes: 1 addition & 2 deletions crates/policies/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,4 @@ log = "0.4"
thiserror = "1.0"

[target.'cfg(target_env = "msvc")'.dependencies]
windows-sys = { version = "0.52", features = ["Win32", "Win32_Foundation"] }
windows = "0.54"
windows-sys = { version = "0.52", features = ["Win32", "Win32_Foundation", "Wdk", "Wdk_Foundation"] }
111 changes: 83 additions & 28 deletions crates/policies/src/policies/file_policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ mod file_policy_impl {
.iter()
.map(|f| {
let name: String = (*f).into();
let description = format!("Access to [{}] with write access is denied", f);
let description =
format!("Access to [{}] is only allowed with read-only access", f);
FunctionPolicy {
name,
lib: LIBC.into(),
Expand All @@ -61,6 +62,31 @@ mod file_policy_impl {
.collect()
}

// Check if the flag is WRITE_ONLY
fn is_write_only_flag(params: &[usize]) -> Result<bool, RuleError> {
let flag = params[1];
let res = (flag & ACCESS_MODE_MASK) == WRITE_ONLY_FLAG;
Ok(res)
}

pub fn write_only_access() -> FuzzPolicy {
MONITORED_FUNCTIONS
.iter()
.map(|f| {
let name: String = (*f).into();
let description =
format!("Access to [{}] is only allowed with write-only access", f);
FunctionPolicy {
name,
lib: LIBC.into(),
rule: Rule::OnEntry(is_write_only_flag),
description,
nb_parameters: 2,
}
})
.collect()
}

/// Checks if the filename contained in the first register is part of the blocked files
fn rule_no_access_to_filenames(registers: &[usize]) -> Result<bool, RuleError> {
let filename: &str;
Expand Down Expand Up @@ -101,59 +127,88 @@ mod file_policy_impl {

#[cfg(target_env = "msvc")]
mod file_policy_impl {
use core::slice;

use super::BLOCKED_FILENAMES;
use crate::engine::{FunctionPolicy, FuzzPolicy, Rule, RuleError};
use crate::policies::block_on_entry;
use windows::core::PCWSTR;
use windows_sys::Win32::Foundation::GENERIC_READ;
use windows_sys::Wdk::Foundation::OBJECT_ATTRIBUTES;
use windows_sys::Win32::Foundation::{GENERIC_READ, GENERIC_WRITE};

const FILE_CRT: &str = "KERNEL32";
// Doc to CreateFileW: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
const OPEN_FILE: &str = "CreateFileW";
const FILE_CRT: &str = "ntdll.dll";
// Doc to NtCreateFile: https://learn.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntcreatefile
const OPEN_FILE: &str = "NtCreateFile";

pub fn no_file_access() -> FuzzPolicy {
vec![FunctionPolicy {
name: OPEN_FILE.into(),
lib: FILE_CRT.into(),
rule: Rule::OnEntry(block_on_entry),
description: "Access to [CreateFileW] denied".into(),
nb_parameters: 7,
description: format!("Access to [{FILE_CRT}::{OPEN_FILE}] denied"),
nb_parameters: 11,
}]
}

/// Policy where file access can only be read_only
pub fn read_only_access() -> FuzzPolicy {
vec![FunctionPolicy {
name: OPEN_FILE.into(),
lib: FILE_CRT.into(),
rule: Rule::OnEntry(is_read_only_flag),
description: format!("Access to [{FILE_CRT}::{OPEN_FILE}] restricted to read-only"),
nb_parameters: 11,
}]
}

const GENERIC_MASK: u32 = 0xf0000000;
// Checks if the flag is READ_ONLY
fn read_only_flag(params: &[usize]) -> Result<bool, RuleError> {
let flag = params[1];
let res = flag == GENERIC_READ as usize;
// NOTE: flag values seems to differ from the documentation:
// https://learn.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntcreatefile
// Refer to the diary for more details
fn is_read_only_flag(params: &[usize]) -> Result<bool, RuleError> {
let flag = params[1] as u32;
let res = (flag & GENERIC_MASK) == GENERIC_READ;
Ok(res)
}

pub fn read_only_access() -> FuzzPolicy {
/// Policy where file access can only be write_only
pub fn write_only_access() -> FuzzPolicy {
vec![FunctionPolicy {
name: OPEN_FILE.into(),
lib: FILE_CRT.into(),
rule: Rule::OnEntry(read_only_flag),
description: "Access to [CreateFileW] restricted to read-only".into(),
nb_parameters: 7,
rule: Rule::OnEntry(is_write_only_flag),
description: format!("Access to [{FILE_CRT}::{OPEN_FILE}] restricted to read-only"),
nb_parameters: 11,
}]
}

// Checks if the flag is WRITE_ONLY
// NOTE: flag values seems to differ from the documentation. Refer to the diary for more
// details
fn is_write_only_flag(params: &[usize]) -> Result<bool, RuleError> {
let flag = params[1] as u32;
let res = (flag & GENERIC_MASK) == GENERIC_WRITE;
Ok(res)
}

/// Checks if the filename contained in the first register is part of the blocked files
fn rule_no_access_to_filenames(registers: &[usize]) -> Result<bool, RuleError> {
// the first register should contain a pointer to the name of the file being accessed
let name_ptr = registers[0] as *const u16;

let filename: String;
// This is unsafe because pointers stored in the first register needs to be valid
let obj_attr_ptr = registers[2] as *const OBJECT_ATTRIBUTES;
unsafe {
filename = PCWSTR::from_raw(name_ptr)
.to_string()
.expect("Failed filename conversion to String type");
let obj_attr: OBJECT_ATTRIBUTES = *obj_attr_ptr;

// Convert win32 UNICODE_STRING to a rust String
let unicode_data = (*obj_attr.ObjectName).Buffer;

// NOTE: `unicode.Length` gives the wrong size of the string
// Instead we manually calculate the size of the string by searching the first null byte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is not ideal as it introduces possibly out of bound reads/overflows and does not take advantage of the idea of the UNICODE_STRING approach.

I think we should rely on nt-string crate to handle this conversion for us, as we will likely use this in multiple occasions when interacting with windows strings.

https://colinfinck.de/posts/nt-string-the-missing-windows-string-types-for-rust/ for reference

Copy link
Contributor Author

@vdang-crabnebula vdang-crabnebula May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good advice 👍. I will look into it

let len = (0..).take_while(|&i| *unicode_data.offset(i) != 0).count();
let buffer: &[u16] = slice::from_raw_parts(unicode_data, len);
let file_path = String::from_utf16_lossy(buffer);
Ok(!BLOCKED_FILENAMES
.iter()
.any(|blocked_filename| file_path.ends_with(blocked_filename)))
}

Ok(!BLOCKED_FILENAMES
.iter()
.any(|blocked_filename| filename.ends_with(blocked_filename)))
}

/// Block access to file with name [`filename`].
Expand All @@ -163,7 +218,7 @@ mod file_policy_impl {
lib: FILE_CRT.into(),
rule: Rule::OnEntry(rule_no_access_to_filenames),
description: format!("Access to files {:?} denied", BLOCKED_FILENAMES),
nb_parameters: 2,
nb_parameters: 11,
}]
}
}
15 changes: 15 additions & 0 deletions docs/src/diary.md
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,21 @@ InvokeRequest {
- to be honest I don't really understand what's happening precisely and I don't want to dig further.
But I'm happy to have found a solution quickly but I expect this to bite me back in the future

#### NtCreateFile use flags different from the doc

- doc: https://learn.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntcreatefile
- from the doc `NtCreateFile` is supposed to use flags such as:
- FILE_GENERIC_READ: 0x00120089
- FILE_GENERIC_WRITE: 0x00120116
- FILE_READ_DATA: 0x00000001
- from the experimentations we get values such as:
- open file in read only: 0x80100080
- open file in write only: 0x40100080
- this matches other known windows constants that exist are:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some references for the future:
https://learn.microsoft.com/en-us/windows/win32/secauthz/access-mask-format
https://referencesource.microsoft.com/#System.Runtime.Remoting/channels/ipc/win32namedpipes.cs,78

So makes sense that these values are in different from the supposed flag values.

- GENERIC_READ: 0x80000000
- GENERIC_WRITE: 0x40000000
- we will use these flags eventhough this is different from what described from the doc

### Docker on Windows

- Docker daemon can be started by launching Docker desktop
Expand Down
3 changes: 2 additions & 1 deletion examples/mini-app/src-tauri/assets/foo.txt
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
foo
/ABKj
8F!qT,x-p��vL^5tM.z
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,13 @@ mod test {
ExitKind::Ok
};
unsafe {
let _ = fuzzer::fuzz_test(
assert!(fuzzer::fuzz_test(
harness,
&options.into(),
COMMAND_PTR as usize,
policies::file_policy::read_only_access(),
)
.is_ok();
.is_ok(),)
}
}
}
20 changes: 20 additions & 0 deletions examples/mini-app/src-tauri/fuzz/fuzz_targets/write_foo_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,24 @@ mod test {
assert_eq!(Some(134), status.code());
}
}

#[test]
fn write_foo_accepted_by_writeonly_policy() {
let w = setup_mock();
let options =
fuzzer::SimpleFuzzerConfig::from_toml(fuzz_config(), COMMAND_NAME, fuzz_dir()).into();
let harness = |input: &BytesInput| {
invoke_command_minimal(w.clone(), create_request(input.bytes()));
ExitKind::Ok
};
unsafe {
assert!(fuzzer::fuzz_test(
harness,
&options,
COMMAND_PTR as usize,
policies::file_policy::write_only_access(),
)
.is_ok(),)
}
}
}