Use a custom ReadOnlySpan marshaller to prevent null pointers when marshaling

[vcsjones]

Background

A common problem we have in System.Security.Cryptography is p/invoking native methods that accept data to work with, and sometimes that data can be an empty span.

Empty spans may be backed by a null ref, which when marshaled are done so as a nullptr / NULL.

Even though using a empty value may be legal and intended, some Win32 APIs, particularly in CNG, do not accept nullptrs - even if the corresponding buffer length is zero.

We have used a variety of tricks to try to address this.

1. Sometimes we allocate a buffer on the stack and pass the length as zero
2. We have a Helpers.GetNonNullPinnableReference that passes in a bogus value if the span is a null ref and the length is zero.

Proposal

This is a proposal to try doing a third, and hopefully better way: use a custom marshaller at the p/invoke declaration that takes care of it for us.

This pull request is an attempt at doing that, and for now is limited in scope to just BCryptGenerateSymmetricKey.

The hope is that if the idea is accepted, we can use ReadOnlySpan<T> are p/invoke declarations more and let the marshaller worry about null refs.

Help

The biggest problem is I am not well informed at writing a customer marshaller. This is largely pattern matching off the existing ReadOnlySpanMarshaller and reading some of the initial design documentation for the v2 marshaller.

/cc @bartonjs @jkoritzinsky

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull Request Overview

This PR implements a custom ReadOnlySpan marshaller to prevent null pointers during p/invoke calls in cryptographic operations. Key changes include adding a new test for empty spans, refactoring CNG key creation logic in multiple implementations to remove manual pointer handling, and introducing the NotNullReadOnlySpanMarshaller to ensure a non-null reference even for empty spans.

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/libraries/System.Security.Cryptography/tests/Rfc2898OneShotTests.cs	Added test Pbkdf2_Password_EmptySpans to verify empty span handling.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs	Refactored to remove unsafe pointer usage and adjust behavior for empty keys.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/Pbkdf2Implementation.Windows.cs	Updated usage of key material eliminating manual length variables and fixed blocks.
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs	Similar refactoring for safe key creation using ReadOnlySpan.
src/libraries/Common/src/System/Security/Cryptography/SP800108HmacCounterKdfImplementationCng.cs	Updated internal key creation to use ReadOnlySpan overload.
src/libraries/Common/src/System/Runtime/InteropServices/NotNullReadOnlySpanMarshaller.cs	Introduced the custom marshaller implementation for non-null ReadOnlySpan marshalling.
src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptGenerateSymmetricKey.cs	Adjusted native interop signatures to use the new custom marshaller.

Files not reviewed (1)

• src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj: Language not supported

Comments suppressed due to low confidence (1)

src/libraries/System.Security.Cryptography/tests/Rfc2898OneShotTests.cs:212

• Consider adding additional tests for scenarios with non-empty spans to ensure that the custom marshaller correctly handles both empty and populated inputs.

public static void Pbkdf2_Password_EmptySpans()

[jkotas]

Suggested change

	if (Unsafe.IsNullRef(ref MemoryMarshal.GetReference(managed)) && managed.IsEmpty)
	if (managed.IsEmpty)

This can be simpler

[bartonjs]

I had the same thought as Jan, then wondered if it was valueable to have the zero-length slice of some other array report a different pointer than the empty array, and didn't leave a comment. (His comment has "forced" me to).

I can't think of anywhere in the crypto use cases where the pointer is used as a lookup (and if it was we'd have to do a lot more pinning), so normalizing all emptys to the same non-null is probably fine.

[jkotas]

I doubt that you will ever use (or want to use) the ManagedToUnmanagedOut variant in libraries. You can certainly add it, but it is effectively dead code.

[vcsjones]

@jkotas @jkoritzinsky the bigger problem I am running in to seems to be that MarshalUsing is not available for downlevel platforms. Is there a way to get it working, similar to how LibraryImport works, or is using it downlevel completely off the table?

[vcsjones]

Closing, because I don't know the viability of this in OOBs and that's a bit of a blocker.