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

Conversation

vdang-crabnebula
Copy link
Contributor

Fix SEC-279

Copy link

linear bot commented May 23, 2024

SEC-279 Fuzzer Windows, use ntdll library rather than win32 to monitor functions

ntdll is lower level than win32 and is expected to be present in all windows systems.
ziglang/zig#1840

Copy link
Contributor

@tillmann-crabnebula tillmann-crabnebula left a comment

Choose a reason for hiding this comment

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

Haven't tested on a windows machine yet but looks good from a code review.

As mentioned in a comment the only required change I would like to request would be around handling the windows unicode string handling to avoid security pitfalls and improve performance.

- 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.

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

@vdang-crabnebula vdang-crabnebula deleted the branch tauri-2.x May 27, 2024 08:56
@vdang-crabnebula vdang-crabnebula mentioned this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants