Runtime: HKDF: inconsistency regarding output key material of zero length

[andreimilto]

There's an inconsistency in the way methods of the System.Security.Cryptography.HKDF class treat a request to derive an output key material (OKM) of zero length: one method throws and the other three methods work ok.
Example:

byte[] ikm = new byte[32];
byte[] salt = new byte[32];
byte[] prk = new byte[32];
byte[] info = new byte[32];
int outputLength = 0;
byte[] output = new byte[outputLength];


// Works fine:
HKDF.DeriveKey(HashAlgorithmName.SHA256, ikm, output, salt, info);

// Works fine:
HKDF.DeriveKey(HashAlgorithmName.SHA256, ikm, outputLength, salt, info);

// Works fine:
HKDF.Expand(HashAlgorithmName.SHA256, prk, output, info);

// Throws ArgumentOutOfRangeException because of invalid outputLength value:
HKDF.Expand(HashAlgorithmName.SHA256, prk, outputLength, info);

Instead there should be a clear policy on whether a request to derive an OKM of zero length is considered valid or not:

• if valid, all four methods should work (i.e., byte overload of Expand should be fixed to return an empty byte array instead of throwing an exception)
• otherwise, all four methods should throw an exception saying that the OKM's length can't be zero.

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]

Marking this initially as 5.0 since this shipped this release and it would be a breaking change to go from no throw -> throw.

I don't feel strongly opinionated if we should throw or not. This operation with length 0 doesn't make much sense. Can't think of scenario where it would make sense to want zero length key. Perhaps we should always throw? @bartonjs any thoughts?

[bartonjs]

Apparently there are two of these, this one for zero and the other for negative... and I answered on the other.

I don't see any way where zero makes sense; we should throw AOORE for any length < 1. (Yeah, we can make it be no-op/Array.Empty, but it's not providing value)

[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]

I was happy to help, @jeffhandley!

[krwq]

I unassigned myself from both bugs and will pick this up somewhere in 6.0 timeline

[vcsjones]

@krwq This and the other one would make a good [up-for-grabs] and [easy] tags if you wouldn't mind someone from the community picking these up.

[vcsjones]

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

[tonycimaglia]

@vcsjones I'll also take a look at this along with #42229

[tonycimaglia]

@bartonjs / @krwq could you assign this to me as well as #42229 ?

[krwq]

Thank you @tonycimaglia!