Timing vulnerability in sample code?

[Danghor]

https://github.com/dotnet/dotnet-api-docs/blob/2d0e5e61b8635b57bdb26b42d879382252edfc62/snippets/csharp/System.Security.Cryptography/HMACSHA256/Overview/hmacsha256.cs#L110C17-L116C18

storedHash is directly taken from the source file, i.e. the potentially manipulated file. Would this loop not cause a timing vulnerability? The attacker could guess the first byte of the MAC, send it together with the manipulated file to the server and observe for which guess the server takes slightly longer to process. This guess can be assumed to be the correct first key MAC byte. Then the attacker can proceed to the next byte etc., essentially brute forcing the MAC one byte after another.

I think creating a hash value of both the expected and the actual MAC value and then comparing the hash value could potentially solve this.

Am I missing something?

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[colejohnson66]

The timing attack you are describing is one where the first failed comparison breaks out of the loop:

public static bool CheckHash(byte[] fromUser, byte[] expected)
{
    // validation elided
    for (int i = 0; i < fromUser.Length; i++)
    {
        if (fromUser[i] != expected[i])
            return false;
    }
    return true;
}

If you look at that snipped, you can see that it doesn't do that. It instead sets a flag, then continues to check each and every byte before exiting the loop.

dotnet-api-docs/snippets/csharp/System.Security.Cryptography/HMACSHA256/Overview/hmacsha256.cs

Lines 110 to 128 in 2d0e5e6

	for (int i = 0; i < storedHash.Length; i++)
	{
	if (computedHash[i] != storedHash[i])
	{
	err = true;
	}
	}
	}
	}
	if (err)
	{
	Console.WriteLine("Hash values differ! Signed file has been tampered with!");
	return false;
	}
	else
	{
	Console.WriteLine("Hash values agree -- no tampering occurred.");
	return true;
	}

This is the proper way to do a hash comparison short of constant-time algorithms.

Regardless, documentation examples are not supposed to be industry-standards. After all, they're examples.

[Danghor]

I see, the loop indeed compares all the bytes of the hashes. However, when only guessing the first byte, wouldn't the function run slightly faster for the correct guess, since one assignment is omitted (while the other bytes of the MAC stay the same)?

[colejohnson66]

Yes. A more "constant time" algorithm would look something like:

for (int i = 0; i < storedHash.Length; i++)
    err |= computedHash[i] != storedHash[i];

Then the value is updated for each byte, not just when it's wrong.

[ManickaP]

@dotnet/area-system-security

[bartonjs]

the correct thing for modern code would be to call CryptographicOperations.FixedTimeEquals, but I don't know that we can update the sample to that; so it might have to just get the inlined version.

[colejohnson66]

Why would the sample not be able to use FixedTimeEquals?

[bartonjs]

It's not available for .NET Framework / .NET Standard 2.0. So it would require bifurcating the sample, which is.... frustrating 😄