Runtime: HKDF: Expand and DeriveKey throw invalid exceptions when outputLength is negative

[andreimilto]

Methods Expand and DeriveKey of the System.Security.Cryptography.HKDF class throw invalid exceptions when the argument outputLength has negative value.

In this example Expand throws ArgumentOutOfRangeException with the message Output keying material length can be at most 8160 bytes (255 * hash length).:

HKDF.Expand(HashAlgorithmName.SHA256, prk: new byte[32], outputLength: -1);

Instead the exception message should say that outputLength can't be negative (or that it must be positive - depends on whether 0 is considered a valid input).

Here DeriveKey throws OverflowException with the message Arithmetic operation resulted in an overflow.:

HKDF.DeriveKey(HashAlgorithmName.SHA256, ikm: new byte[32], outputLength: -1);

Instead the type of exception should be ArgumentOutOfRangeExceptionand the message should say that the outputLength can't be negative (or that it must be positive - depends on whether 0 is considered a valid input).

Windows 10 x64 Pro, dotnet 5.0.0-preview.8.

[msftbot[bot]]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.

[krwq]

It's not blocking release but I think we should fix at least fix OverflowException to correctly throw ArgumentOutOfRangeException for 5.0 (the call if it will be included in 5.0 doesn't belong to me but will make a patch and see what happens)

[bartonjs]

Zero doesn't really make sense, I'm fine with making it throw AOORE.

[jeffhandley]

I did a preemptive bar check on this and determined it won't meet the bar for 5.0.0 (since there are workarounds); moving to 6.0.0. Thanks for reporting this, @andreimilto!

[andreimilto]

You're welcome, @jeffhandley!

[vcsjones]

Note to anyone picking this up: it probably makes sense to fix #42230 in the same pull request.

[tonycimaglia]

Hi, I am a first-time contributor and I would love to take a shot at this.

[vcsjones]

@tonycimaglia Excellent!

Have a look at the Workflow Guide for instructions about getting dotnet/runtime building and running locally, the OS table has a link to building requirements. If everything is set up correctly, you should be able to do build.cmd -rc Release -s clr+libs at the repository root. (Substitute build.cmd for build.sh if you are on macOS / Linux). You need to do this build from the command line once before the Visual Studio solution files will work.

If you plan to develop and test in Visual Studio, see the Visual Studio Workflow for some info on getting tests running.

Also, take a look at the Building Libraries for more info about building and testing libraries from the Command Line.

The HKDF implementation is here:

https://github.com/dotnet/runtime/blob/a1f9226e7b7689aa153861b7bb5011e6f272ccc2/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/HKDF.cs#L49-L51

and the tests are here:

https://github.com/dotnet/runtime/blob/a1f9226e7b7689aa153861b7bb5011e6f272ccc2/src/libraries/System.Security.Cryptography.Algorithms/tests/HKDFTests.cs#L13

Before you start making changes it'd be a good idea to make sure that all of the tests in 'System.Security.Cryptography.Algorithms' are green.

[tonycimaglia]

@vcsjones do you mind assigning these issues to me so no one else picks them up?

[vcsjones]

@bartonjs / @krwq (I don't have assign rights)

[vcsjones]

Here's an example of another method checking that a parameter is positive:

https://github.com/dotnet/runtime/blob/1b491f603275a0d943d613da1e914502b9102e0f/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RandomNumberGenerator.cs#L143-L144

For testing ArgumentExceptions and derived exceptions, we have a custom assertion helper that should be used that also asserts the ParamName of the exception. Here is an example:

https://github.com/dotnet/runtime/blob/0ed3f3311e4d07cf6840b78c32055ab1b16d092c/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DsaFamilySignatureFormatTests.cs#L315-L317

It's new-ish so it isn't used consistently in all of the unit tests, but it should be used going forward.