HMACSHA1.ComputeHash() thread-safety question

Question

i am asking myself if it would be dangerous on a asp.net page´s codebehind to use an static (Shared) variable which holds an HMACSHA1-instance. the problem is that the same HMACSHA1-instance would be used by all the asp.net worker-process´ threads when processing multiple simultaneous requests on the same asp.net-page. all (HMACSHA1)instance- and ComputeHash()-method variables which are used/modified by ComputeHash() would be shared (= could be modified) by all the threads?! is that assumption correct? as a result the return value of ComputeHash would not be guaranteed to be correct?!?! thus i am not allowed to use an static/Shared HMACSHA1-instance over all the asp.net-threads..

i am just wondering what you think about this problem.

the only solution to this would be sth like an critical path etc in the ComputeHash()-method. but that is "out of our reach"..

regards, kris

Answer 1

I just got an unknown cryptographic exception form the SHA256Cng.ComputeHash, when running 8 or 16 parallel tasks that amongst other things performed the hash computation, on a quad-core HT cpu.

Adding locking semantics around ComputeHash solved the issue - so it seems that at least the SHA256Cng version is not thread safe.

Answer 2

It worth to know that KeyedHashAlgorithm.ComputeHash() is not thread safe because it give non-deterministic result for the same KeyedHashAlgorithm.Key.

In my case, I want to cache KeyedHashAlgorithm since my KeyedHashAlgorithm.Key is always the same to verify the authenticity from client side. I realize that ComputeHash() is not consistent, probably it cache internal variable into the KeyedHashAlgorithm instance. I should cache the instance per thread ThreadStatic or ThreadLocal. This is the test:

Static KeyedHashAlgorithm gives inconsistent result:

var kha = KeyedHashAlgorithm.Create("HMACSHA256");
kha.Key = Encoding.UTF8.GetBytes("key");
Action comp = () =>
{
    var computed = kha.ComputeHash(Encoding.UTF8.GetBytes("message"));
    Console.WriteLine(Convert.ToBase64String(computed));
};
Parallel.Invoke(comp, comp, comp, comp, comp, comp, comp, comp);

Compared to KeyedHashAlgorithm per thread:

ThreadLocal<KeyedHashAlgorithm> tl= new ThreadLocal<KeyedHashAlgorithm>(() =>
{
    var kha = KeyedHashAlgorithm.Create("HMACSHA256");
    kha.Key = Encoding.UTF8.GetBytes("key");
    return kha;
});
Action comp = () =>
{
    var computed = tl.Value.ComputeHash(Encoding.UTF8.GetBytes("message"));
    Console.WriteLine(Convert.ToBase64String(computed));
};
Parallel.Invoke(comp, comp, comp, comp, comp, comp, comp, comp);

This code can be use to test other functions for 'thread safety' result. Hope this will help others.

Answer 3

Hashing algorithms are deterministic, they must return the same hash for a given input every time.

As long as you use the same key each time then there's no need to make them static. However if you're constructing the HMACSHA1 instance with the parameterless constructor then it generates a random key. You should be taking the random value from the KeyValue property and storing it with the hash.

It is definitely dangerous to use a static instance. If Thread1 sets the value to be calculated, and then Thread2 sets the value before Thread1 calls ComputerHash() then Thread1 is going to get the hash of Thread2's value. The same will happen if either thread is setting the key.

Answer 4

If you want thread safety without locking, you can use the ThreadStatic attribute to create a unique instance on each thread like this:

[ThreadStatic]
private static HMACSHA1 _hmacSha1;

public static HMACSHA1 HmacSha1
{
    get 
    {
        if (_hmacSha1 == null)
        {
            // this will happen once on each thread
            _hmacSha1 = new HMACSHA1(GetKeyBytes());
        }               

        return _hmacSha1;
    }
}

Now, two side notes:

1. Accessing a thread-static field takes significantly longer than accessing a normal static field. So the thread-static version may or may not be better for you.

2. If you're doing this once per page request, then the difference will be so minuscule that it won't matter which approach you choose. If you were doing this in a very tight loop, or the code in your lock section took a very long time, then the choice could be important.