Update BCryptSetProperty to optionally take pbinput

[tofay]

Suggestion

Regarding https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Security/Cryptography/fn.BCryptSetProperty.html, for a HKDF operation I needed to set a property to a null pointer (I tried setting an empty array but it caused a CNG error).

I ended up just not using windows-rs for the one call in my crate that needed to set a null pointer: https://github.com/tofay/rustls-cng-crypto/blob/2f1de485a6856a8db4ccadb073e5ec3cf872d04f/src/hkdf.rs#L97.

[kennykerr]

From the header:

//  - Finally the key (or secret) is finalized via the
//  BCRYPT_HKDF_PRK_AND_FINALIZE property. In this case the input property
//  must be NULL since the PRK was already passed in.

Yes, I think we should be able to make this work - I'll see what I can do.

[tim-weis]

That's a bug, albeit not in the windows crate.

The bcrypt.h SDK header file declares BCryptSetProperty like this:

_Must_inspect_result_
NTSTATUS
WINAPI
BCryptSetProperty(
    _Inout_                 BCRYPT_HANDLE   hObject,
    _In_z_                  LPCWSTR pszProperty,
    _In_reads_bytes_(cbInput)    PUCHAR   pbInput,
    _In_                    ULONG   cbInput,
    _In_                    ULONG   dwFlags);

The win32metadata tooling faithfully transforms this into

public static extern NTSTATUS BCryptSetProperty(
  [In, Out] BCRYPT_HANDLE hObject,
  [Const, In] PWSTR pszProperty,
  [MemorySize(BytesParamIndex = 3), In] byte* pbInput,
  [In] uint cbInput,
  [In] uint dwFlags);

This gets picked up by the windows crate's code generator that collapses pbInput and cbInput into a slice. Rust mandates that even zero-length slices are constructed from (and represented with) a valid (i.e., non-null) pointer. All of this is very much in line with the API contract spelled out via the SAL annotations.

The interesting bit here is the _In_reads_bytes_(cbInput) SAL annotation that requires pbInput to be a valid pointer to an array of size cbInput (as opposed to _In_reads_bytes_opt_ that would accept a null pointer as well).

The bug could be either one of:

• The BCryptSetProperty implementation that exhibits different behavior for a zero-length input array depending on whether it is expressed through a null pointer or a non-null pointer.
• The SAL annotation for the function declaration that mandates a non-null pointer always (mind you, passing a null pointer won't pass static code analysis when called from C either).
• Client code using the API out-of-spec (the comment quoted by Kenny earlier suggests that this isn't the case).

Properly fixing this would necessitate a change to the header file (and possibly the implementation) of the BCrypt library. That'll take a hot minute to trickle down. What you can do in the meantime is taking advantage of the windows-targets crate's link! macro to generate the raw import binding, as the windows create is doing:

link!("bcrypt.dll" "system" fn BCryptSetProperty(hobject : BCRYPT_HANDLE, pszproperty : windows_core::PCWSTR, pbinput : *const u8, cbinput : u32, dwflags : u32) -> super::super::Foundation:: NTSTATUS);

[riverar]

Agree, looks like a bcrypt bug; makes you wonder how they satisfied the SAL tests.

We can take this to win32metadata and get the input marked as optional to support nullable properties.

[tim-weis]

[...] makes you wonder how they satisfied the SAL tests.

Checking an _In_ argument against a null pointer is valid, even if superfluous. Not checking an _In_opt_ pointer against null would fail, but BCryptSetProperty isn't using the _opt_-variant. The expectation here is that SAL testing will succeed, and that is valid (in face of an incorrect/inappropriate SAL annotation).

[tim-weis]

We can take this to win32metadata and get the input marked as optional to support nullable properties.

Please, don't.

This establishes a secondary API surface with unverified assumptions.

Do have the BCrypt owners fix their published API contract, whatever time/effort this takes. Under the assumption that the implementation is attributed as _Use_decl_annotations_ (or something to that effect), this ensures that there aren't any code paths that deref a null pointer in shipped binaries.

There's a place and time to be pragmatic.

Crypto isn't that.

[riverar]

This establishes a secondary API surface with unverified assumptions.

Does it? The above BCRYPT_HKDF_PRK_AND_FINALIZE use case is documented and requires this API to accept a null input.

The counterpart BCryptGetProperty returns the size of value for the requested property; callers are already expected to check this. So it's not like we're opening the gates to a stream of new nullable properties.

[tim-weis]

This establishes a secondary API surface with unverified assumptions.

Does it? The above BCRYPT_HKDF_PRK_AND_FINALIZE use case is documented and requires this API to accept a null input.

The contract is "documented" in a source code comment. Source code comments are (generally) ignored by static code analysis tools. This is in stark contrast to SAL annotations that are tool-verified at the call site and the implementation.

I'd very much appreciate if the win32metadata tooling didn't sidestep tool-based verification of API contracts.

[kennykerr]

@mikebattista can you chase down the API owner and see if they'd like to update the SAL annotation?