windows-rs: UNICODE_STRING contains a PWSTR that points to a non-null-terminated string

Which crate is this about?

windows-sys

Crate version

0.48.0

Summary

Per UNICODE_STRING documentation on MSDN (emphasis added):

Buffer

Pointer to a wide-character string. Note that the strings returned by the various LSA functions might not be null-terminated.

Windows 7, Windows Server 2008, Windows Vista, Windows Server 2003 and Windows XP: When the Length structure member is zero and the MaximumLength structure member is 1, the Buffer structure member can be an empty string or contain solely a null character. This behavior changed beginning with Windows Server 2008 R2 and Windows 7 with SP1.

And STRING documentation on MSDN (emphasis added):

If the string is null-terminated, Length does not include the terminating NULL.

(ie: the value may not be null terminated)

At the moment, the UNICODE_STRING::Buffer member is declared as a PWSTR:

https://github.com/microsoft/windows-rs/blob/fa7b52473eebad0deb8e561bfc893df22f283999/crates/libs/sys/src/Windows/Win32/Foundation/mod.rs#L20342-L20345

And STRING::Buffer is a PSTR:

https://github.com/microsoft/windows-rs/blob/fa7b52473eebad0deb8e561bfc893df22f283999/crates/libs/sys/src/Windows/Win32/System/Kernel/mod.rs#L559-L562

PWSTR assumes it is pointing to a null-terminated array, and uses wcslen to find the length:

https://github.com/microsoft/windows-rs/blob/fa7b52473eebad0deb8e561bfc893df22f283999/crates/libs/core/src/strings/pwstr.rs#L34-L36

PSTR does the same but with strlen:

https://github.com/microsoft/windows-rs/blob/fa7b52473eebad0deb8e561bfc893df22f283999/crates/libs/core/src/strings/pstr.rs#L34-L36

Both of these functions cannot check the Length, so have buffer overrun issues:

Security Note These functions incur a potential threat brought about by a buffer overrun problem. Buffer overrun problems are a frequent method of system attack, resulting in an unwarranted elevation of privilege. For more information, see Avoiding buffer overruns.

Toolchain version/configuration

% rustup show Default host: x86_64-apple-darwin rustup home: ~/.rustup

installed toolchains

stable-x86_64-apple-darwin (default) nightly-x86_64-apple-darwin

installed targets for active toolchain

thumbv6m-none-eabi wasm32-unknown-unknown x86_64-apple-darwin x86_64-pc-windows-gnu <-- using this target x86_64-unknown-linux-gnu

active toolchain

stable-x86_64-apple-darwin (default) rustc 1.70.0 (90c541806 2023-05-31)

Reproducible example

use windows::core::{PSTR, PWSTR};
use windows::Win32::Foundation::UNICODE_STRING;
use windows::Win32::System::Kernel::STRING;

fn main() {
    let mut hello = [104u16, 101, 108, 108, 111, 33];
    let u = UNICODE_STRING {
        Length: 10,
        MaximumLength: 12,
        Buffer: PWSTR(hello.as_mut_ptr()),
    };

    let mut world = [119u8, 111, 114, 108, 100, 33];
    let s = STRING {
        Length: 5,
        MaximumLength: 6,
        Buffer: PSTR(world.as_mut_ptr()),
    };

    // These are unsafe conversions because they don't use the Length parameter
    println!("{}", unsafe { u.Buffer.to_string().unwrap() });
    println!("{}", unsafe { s.Buffer.to_string().unwrap() });

    // Use length parameter
    let u = String::from_utf16(unsafe { std::slice::from_raw_parts(u.Buffer.0, u.Length.into() / 2) })
        .unwrap();
    let s = std::str::from_utf8(unsafe { std::slice::from_raw_parts(s.Buffer.0, s.Length.into()) })
        .unwrap();
    println!("{u} {s}");
}

Crate manifest

[package]
name = "windows-strings"
version = "0.1.0"
edition = "2021"

[dependencies]
windows = { version = "0.48.0", features = ["Win32_System_Kernel", "Win32_Foundation"] }

Expected behavior

the Buffer field should be a *mut pointer, not a PSTR / PWSTR.
STRING and UNICODE_STRING should be wrapped similar to what this crate does for HSTRING to provide safe conversions to Rust String types

because using Buffer.to_string() with these is not correct!

Actual behavior

Additional comments

No response

EDITED: fixed code example’s UNICODE_STRING::Length to be a length in bytes, rather than UTF-16 codepoints, and extended MaximumLength. Added note about raw pointers.

About this issue

Original URL
State: closed
Created a year ago
Comments: 22 (6 by maintainers)

Most upvoted comments

@riverar Well, no. PWSTR’s have the _Null_terminated_ SAL annotation. They are always assumed to be null terminated. That’s in contrast to PWCH, a type that aliases the same underlying type, but doesn’t have the _Null_terminated_ attribute.

This is how (I presume) things are designed. Not going to comment on how things turn out in practice. YMMV, I guess.

On a more personal note, while I have your attention already: Love your contributions, here, and elsewhere. You’re an absolute animal! Much like Kenny, who’s someone/“something” I cannot name without falling victim to blasphemy.

Thank you both. You’ve provided me with powerful tools that make my life easier, and a feeling of confidence, that it’s not just me who “give[s] a damn”.

tim-weis on Jul 6, 2023

SetEnvironmentStringsW (and friends) are interesting in a number of ways:

The SDK header file processenv.h describes the argument using the _NullNull_terminated_ annotation. Ignoring the misnomer this is roughly correct.

The function signature in the metadata looks like this:

BOOL SetEnvironmentStringsW([In][NotNullTerminated][NullNullTerminated] PWSTR NewEnvironment);

It’s the metadata transformation that uncovers a few issues with the current approach. The obvious one up front: [NotNullTerminated] AND [NullNullTerminated] creates a conflict that cannot be resolved. One could argue that [NotNullTerminated] is to only mean “might not be null-terminated”, but really, things are wrong at a much more fundamental level.

Specifically, the implied assumption that PWSTR is null-terminated, but without a dedicated [NullTerminated] attribute, introduced the need for the awkward negative attribute [NotNullTerminated] for the PWCH type.

A more tractable scheme would be as follows:

Treat PWCH as the unconstrained character pointer type, and expose it.
Use the [NullTerminated] attribute for all PWSTRs.
Introduce the [NullNullTerminated] attribute, expose the PZZWSTR type with that attribute, or pick up the attribute where it happens to be spelled out in the function signature (e.g. SetEnvironmentStringsW).
Treat all attributes as strictly additive.

With those changes the transformed signature would look like this:

BOOL SetEnvironmentStringsW([In][NullNullTerminated] PWCH NewEnvironment);

Are there any issues with this proposed change, and would the win32metadata team be willing to act upon it (assuming that riddle isn’t going to make Windows.Win32.winmd obsolete at some point)?

tim-weis on Jul 17, 2023

@tim-weis I hope to share more about those plans as soon as I can. Thanks for your patience.

kennykerr on Jul 21, 2023

Kenny is currently heads down working on riddle to simplify WinRT and Win32 component authoring and code generation. We are actively exploring other potential areas where riddle could be used but we there’s nothing to share yet. (Literally. I’m not trying to be evasive.)

Regarding contributions to the metadata repositories: I’d say yes, please do continue. Anything done in those repositories will certainly serve as a baseline for any potential future work and benefits all projections, not just Rust.

riverar on Jul 20, 2023

Fair. PWSTR holds a pointer to a null-terminated string. It having unsafe utility functions is not a bug. UNICODE_STRING contains a PWSTR that points to a non-null-terminated string, breaking its contract.

Maybe a short term fix would be to pick up on [NotNullTerminated] annotated PWSTRs and replace them with *mut _.

On the metadata side, perhaps we can then extend the [NotNullTerminated] attribute to include a parameter that indicates which field member can be relied upon for the string length. (Or just re-use [NativeArrayInfo] or [MemorySize], it’s not clear yet.) This should aid codegen in determining whether to provide a nicer facility or to emit the raw pointer.

Some other APIs and structures that make use of non-null-terminated PWSTRs (from metadata’s perspective) that we need to keep in mind:

riverar on Jul 17, 2023