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 ArgumentOutOfRangeException and 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.

[ghost]

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:

runtime/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/HKDF.cs

Lines 49 to 51 in a1f9226

	public static int Extract(HashAlgorithmName hashAlgorithmName, ReadOnlySpan<byte> ikm, ReadOnlySpan<byte> salt, Span<byte> prk)
	{
	int hashLength = HashLength(hashAlgorithmName);

and the tests are here:

runtime/src/libraries/System.Security.Cryptography.Algorithms/tests/HKDFTests.cs

Line 13 in a1f9226

public abstract class HKDFTests

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:

runtime/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RandomNumberGenerator.cs

Lines 143 to 144 in 1b491f6

	if (toExclusive <= 0)
	throw new ArgumentOutOfRangeException(nameof(toExclusive), SR.ArgumentOutOfRange_NeedPosNum);

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:

runtime/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DsaFamilySignatureFormatTests.cs

Lines 315 to 317 in 0ed3f33

	AssertExtensions.Throws<ArgumentOutOfRangeException>(
	"signatureFormat",
	() => SignData(key, empty, HashAlgorithmName.SHA1, SignatureFormat));

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

[vcsjones]

Hey @tonycimaglia - wanted to check in with you and see you were still willing to submit pull requests for these. If not, no worries. Let me know if you're running in to any issues.

[ADustyOldMuffin]

Since there hasn't been a response in 8 months I'd be interested in picking this up since I just finished one.

[bartonjs]

Go for it, @ADustyOldMuffin 😄

[vcsjones]

@ADustyOldMuffin Just in case you missed the in the scroll, fixing #42230 will likely address this one at the same time (but we should make sure tests cover both cases).