SP800-108 CTR HMAC

[vcsjones]

This implements SP800-108 CTR HMAC as a package. In .NET 8, this is an inbox component. For down-level platforms, Microsoft.Bcl.Cryptography was introduced as a package for .NET Standard and .NET Framework.

Some implementation notes.

1. This at runtime decides if the platform is Windows 8+. If it is, you get the CNG implementation. If it isn't, you get the Managed implementation.
2. Both the CNG and Managed implementation support NetCoreApp and .NET Standard. This results in some backward desirable behavior. The .NET Standard implementation prefers working with byte[] for inputs because of IncrementalHash. .NET Core prefers working with spans. Because of this, we avoid the byte[] APIs deferring to {ReadOnly}Span<byte> APIs. Otherwise, we will go from byte[] -> span -> byte[]. This is true not just for public APIs that accept input, but also for places we need to do conversions, like from string to byte[]. In .NET Standard, we always rent from the pool and avoid doing work on the stack, whereas .NET Core can use the stack.
3. This class is thread safe for all implementations. This includes instance methods. Instances methods should either a) product the correct result or b) throw ObjectDisposedException. We get this for free with safe handles and the CNG implementation. For the managed implementation, the key is reference counted. When the reference count reaches zero, the key is zeroed and attempts to acquire the key afterward throw an ObjectDisposedException.
4. The test data draws from a few different places (explained in the tests) such as RFCs and other implementations like SymCrypt. While NIST has CAVP test vectors, those tests use 800-108 CTR HMAC in a mode that our public API does not support. All of the test vectors assume you have total control over the input into the PRF, whereas our public API forces the input to be label || 0x00 || context. So the NIST test vectors are unusable for tests.

Closes #65577

[ghost]

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

Issue Details

---

This implements SP800-108 HMAC CTR as a package. It is not inboxed.

Some implementation notes.

1. This at runtime decides if the platform is Windows 8+. If it is, you get the CNG implementation. If it isn't, you get the Managed implementation.
2. Both the CNG and Managed implementation support NetCoreApp and .NET Standard. This results in some backward desirable behavior. The .NET Standard implementation prefers working with byte[] for inputs because of IncrementalHash. .NET Core prefers working with spans. Because of this, we avoid the byte[] APIs deferring to {ReadOnly}Span<byte> APIs. Otherwise, we will go from byte[] -> span -> byte[]. This is true not just for public APIs that accept input, but also for places we need to do conversions, like from string to byte[]. In .NET Standard, we always rent from the pool and avoid doing work on the stack, whereas .NET Core can use the stack.
3. This class is thread safe for all implementations. This includes instance methods. Instances methods should either a) product the correct result or b) throw ObjectDisposedException. We get this for free with safe handles and the CNG implementation. For the managed implementation, the key is reference counted. When the reference count reaches zero, it is zeroed and attempts to acquire the key afterward throw an ObjectDisposedException.
4. The test data draws from a few different places (explained in the tests) such as RFCs and other implementations like SymCrypt. While NIST has CAVP test vectors, those tests use 800-108 CTR HMAC in a mode that our public API does not support. All of the test vectors assume you have total control over the input into the PRF, whereas our public API forces the input to be label || 0x00 || context. So the NIST test vectors are usable for tests.

Closes #65577

Author:	vcsjones
Assignees:	-
Labels:	area-System.Security
Milestone:	-

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[stephentoub]

Thanks.

We really need a new package/assembly for this one type? We can't, for example, just squint and say it's related to hashing so it can go in System.IO.Hashing?

[vcsjones]

We really need a new package/assembly for this one type?

There is internal (from Microsoft) interest in this capability for .NET Framework, and the API proposal suggested this as a package name.

We can't, for example, just squint and say it's related to hashing so it can go in System.IO.Hashing?

My 2c: that's... quite a bit of a squint. That package is specifically documented as "non cryptographic hashing" in its README. This API is 1. cryptographic and 2. doesn't do hashing, strictly speaking (though the PRF is HMAC in this one case).

Thinking about it more, I don't really see a good "home" for this type in an existing package, so a new package makes sense to me.

Though I will defer to y'all.

[stephentoub]

If we really lack a good existing package/assembly for it, then we should think hard about what to name this one so we don't need to create yet another the next time we need to ship another cryptographic algorithm downlevel.

[vcsjones]

we should think hard about what to name this one so we don't

@terrajobst @bartonjs @GrabYourPitchforks do y'all have any thoughts here?

Something like System.Security.Cryptography.Extensions?

[bartonjs]

If I recall correctly, I asked that same question either on the text portion of the proposal or during the API review, and there wasn't a good answer because we didn't know what the next thing would be.

We... could... do something like System.Security.Cryptography.NETStandard as the bucket for things that we're exposing to .NET Standard 2.0, and potentially decide that we should take the .NET (nee Core) variant of it and release it inbox out of System.Security.Cryptography.dll (with S.S.C.NETStandard typeforwarding for the net8 TFM). Then the NETStandard NuGet package is just for those things that we add to S.S.C that we're exposing back to .NET Standard. I don't remember if a super-generic bucket like that was proposed and shot down, or if I'm striking gold here.

Because, certainly, if this wasn't going to NS2.0 it'd just be in System.Security.Cryptography.dll, and we wouldn't bother with NuGet at all.

[vcsjones]

System.Security.Cryptography.NETStandard

I would probably avoid calling anything NETStandard, but I don't have a big preference for what the generic name is.

I do have a bit of a preference for using a generic name at all. One thing I don't really want is a big invitation / debate on what should be in NETStandard or whatever going forward. Let's say, of the sake of argument, HKDF doesn't in .NET today. Would we put it in this Extensions or NETStandard package? What about CertificateRevocationListBuilder?

I think there is clear evidence that SP800108 needs to be made available in downlevel targets. What's not clear to me is why everything else couldn't argue that criteria.

I don't remember if a super-generic bucket like that was proposed and shot down, or if I'm striking gold here.

It was discussed in the API review here. https://youtu.be/bDTLx5SXusY?t=5714 (1:35:20 in).

My take away from that discussion a slight preference to avoid a "dumping ground" package and "fine with another one off package".

[GrabYourPitchforks]

I've finished review of the prod logic - haven't gotten to tests yet. Some nits, and a race condition in ReleaseKey that we should address, but everything LGTM. Will try to get to the tests in a few days.

Thanks Kevin for getting to this! It's awesome to see it come together. :)

I think there is clear evidence that SP800108 needs to be made available in downlevel targets. What's not clear to me is why everything else couldn't argue that criteria.

Your line of reasoning is valid. I'm seeing some internal requests for AES-GCM (and maybe AES-CCM?) to be made available downlevel as well since folks are clamoring for them on net4x.

[vcsjones]

I'm seeing some internal requests for AES-GCM (and maybe AES-CCM?) to be made available downlevel as well since folks are clamoring for them on net4x.

That would probably have to be Windows-only or limited to .NETFramework. Supporting a true netstandard2.0 for GCM / CCM would be a huge multiplier in complexity.

[vcsjones]

@GrabYourPitchforks I believe I have addressed all of your feedback thus far, with most of it actioned, some of it "maybe not". I appreciate your time!

[GrabYourPitchforks]

That would probably have to be Windows-only or limited to .NETFramework. Supporting a true netstandard2.0 for GCM / CCM would be a huge multiplier in complexity.

Oh, it would definitely add complexity. But just yesterday I was in an email thread (which I think you were on as well!) where the writing on the wall looms ever larger.

Fortunately that's a problem for future us and not for this PR! 🫠

[jeffhandley]

I know we've still been waiting for a re-review from @GrabYourPitchforks, but from what I can tell all previous comments have been addressed. @bartonjs, if this looks good to you, I propose we go ahead and merge this ahead of Preview 1. We can revisit the packaging topic if another idea emerges there, and if there's post-merge feedback from @GrabYourPitchforks, we can address it in a separate PR.

[stephentoub]

We can revisit the packaging topic if another idea emerges there

If we rename it, won't that mean we'll have another dead-end package on nuget forevermore? It's not the end of the world, just, not ideal.

[jeffhandley]

We can revisit the packaging topic if another idea emerges there

If we rename it, won't that mean we'll have another dead-end package on nuget forevermore? It's not the end of the world, just, not ideal.

True. I rationalized it as: It'd only exist with -preview version(s) and we could unlist it while keeping it available for package restore).

Is System.Security.Cryptography.Extensions still a candidate name, or are there objections to that?

[vcsjones]

Is System.Security.Cryptography.Extensions still a candidate name, or are there objections to that?

My 2 cents, it's my preferred choice.

[jeffhandley]

Nit-picky: It looks like we always use lower-case .netstandard.cs and .netcoreapp.cs throughout the repo. Can you rename those files to lowercase the suffixes please?

[jeffhandley]

Is System.Security.Cryptography.Extensions still a candidate name, or are there objections to that?

My 2 cents, it's my preferred choice.

@bartonjs is going to re-review, so we'll give him a chance to weigh in on that again. In anticipation of that being OK, I'm tempted to get a commit ready with that restructuring. Would you like to do that, @vcsjones; or would you like me to? (Either's good with me; I have time today).

[vcsjones]

Would you like to do that, @vcsjones

I should be able to get that in over the next few hours.

[bartonjs]

If we want to propose a new package name, that needs to go back to API Review.

[bartonjs]

Since there's active discussion about renaming it, it should go back to API Review. If this gets merged in the next couple of hours, it makes it into Preview 1... which we don't want to do if we're going to rename it literally tomorrow. So, despite having signed off on the code, marked the PR as No Merge.

[vcsjones]

it should go back to API Review

Okay. I'll address some nits but hold off on pushing the commit that does the rename until it comes back from API review.

Significant changes from the API proposal.

[vcsjones]

@ViktorHofer added you to get input on the packaging for Microsoft.Bcl.Cryptography. The background is we added a new type called SP800108HmacCounterKdf. This is inbox in System.Security.Cryptography. For down level targets, that package was introduced to contain an implementation for .NET Framework and .NET Standard, and should type-forward for NetCoreApp.

[ericstj]

@vcsjones would this be interesting to add to dotnet/core#8134? If so please consider adding today before end of day. Sorry for the late notice.

[vcsjones]

would this be interesting to add to dotnet/core#8134?

Eh, I think it's cool, but I don't think it has broad enough appeal, and demonstrating it is tricky.