Possible thread safety issue with RequestValidator.cs causing System.Security.Cryptography.CryptographicException

[seannybgoode]

We have a transient Cryptography Exception being raised by RequestValidator.cs with the error:
"Hash not valid for use in specified state."

Described in detail by me here:
https://stackoverflow.com/questions/54699303/transient-system-security-cryptography-cryptographicexception-in-twiliorequestva

Doing some reading, and digging around in RequestValidator.cs, it looks like the private member _hmac, may not be thread safe. The attribute implementation as directed by Twilio documentation (linked in the stackoverflow post), is only initialized once. As such, the private member may be called by multiple threads concurrently, and possibly cause the failure under load.

My source for that is here: "https://stackoverflow.com/questions/26592596/why-does-sha1-computehash-fail-under-high-load-with-many-threads"

However the supporting documentation to that comment doesn't mention anything about thread safety, although HMACSHA1 does inherit from the same base-class.

Version:5.26

Code Snippet

See the stackoverflow post.

Exception/Log

System.Security.Cryptography.CryptographicException:
   at System.Security.Cryptography.CryptographicException.ThrowCryptographicException (mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)
   at System.Security.Cryptography.Utils.HashData (mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)
   at System.Security.Cryptography.SHA1CryptoServiceProvider.HashCore (mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)
   at System.Security.Cryptography.HashAlgorithm.TransformBlock (mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)
   at System.Security.Cryptography.HMAC.HashCore (mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)
   at System.Security.Cryptography.HashAlgorithm.ComputeHash (mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)
   at Twilio.Security.RequestValidator.GetValidationSignature (Twilio, Version=5.14.1.0, Culture=neutral, PublicKeyToken=null)
   at Misc.TwilioRequestValidator.ValidateTwilioRequestAttribute.IsValidRequest (RCHHRATool, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null)
   at Misc.TwilioRequestValidator.ValidateTwilioRequestAttribute.OnActionExecuting (RCHHRATool, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null)
   at System.Web.Mvc.Async.AsyncControllerActionInvoker+AsyncInvocationWithFilters.InvokeActionMethodFilterAsynchronouslyRecursive (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncControllerActionInvoker+AsyncInvocationWithFilters.InvokeActionMethodFilterAsynchronouslyRecursive (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncControllerActionInvoker+<>c__DisplayClass33.<BeginInvokeActionMethodWithFilters>b__31 (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncResultWrapper+WrappedAsyncResultBase`1.Begin (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncControllerActionInvoker.BeginInvokeActionMethodWithFilters (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncControllerActionInvoker+<>c__DisplayClass21.<BeginInvokeAction>b__19 (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncResultWrapper+WrappedAsyncResultBase`1.Begin (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncControllerActionInvoker.BeginInvokeAction (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Controller.<BeginExecuteCore>b__1c (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncResultWrapper+WrappedAsyncVoid`1.CallBeginDelegate (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncResultWrapper+WrappedAsyncResultBase`1.Begin (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Controller.BeginExecuteCore (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncResultWrapper+WrappedAsyncResultBase`1.Begin (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Controller.BeginExecute (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.MvcHandler.<BeginProcessRequest>b__4 (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncResultWrapper+WrappedAsyncVoid`1.CallBeginDelegate (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.Async.AsyncResultWrapper+WrappedAsyncResultBase`1.Begin (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.Mvc.MvcHandler.BeginProcessRequest (System.Web.Mvc, Version=5.2.3.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35)
   at System.Web.HttpApplication+CallHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute (System.Web, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a)
   at System.Web.HttpApplication+<>c__DisplayClass285_0.<ExecuteStepImpl>b__0 (System.Web, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a)
   at System.Web.HttpApplication.ExecuteStepImpl (System.Web, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a)
   at System.Web.HttpApplication.ExecuteStep (System.Web, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a)

Steps to Reproduce

1. Run a server under load with the RequestValidator running checks on Twilio Request
2. Wait (mine seems to be occurring in production only after several weeks of uptime)
3. Twilio requests begin failing in large numbers due to crypto-exceptions

[thinkingserious]

Hello @seannybgoode,

Thanks for taking the time to report this!

This issue has been added to our internal backlog to be prioritized. Pull requests and +1s on the issue summary will help it move up the backlog.

With best regards,

Elmer

[seannybgoode]

@childish-sambino

Here are my thoughts on a fix here:

The object disposed exceptions are most likely occurring when a validation flow is in-progress, an exception occurs, a re-init is triggered, and theres some garbage collection with causes the _hmac._disposed flag to flip to true. This is a bit of a red-herring though, I think these requests would fail anyway, as they're working on an the member that needed to be reinitialized anyway.

From what I can tell, the root cause here is that two threads could call ComputeHash on the same instance at the same time. This corrupts the members of the object irrecoverably, and causes all subsequent requests to fail.

Options for a fix:

1. This could be fixed by holding the HMACSHA1 object reference in local scope in the 'GetValidationSignature' body. This adds some overhead, in that we're calling 'new', possibly a lot. I ran some tests on HMACSHA1 performance, and we're (on average) doubling the cost of this method. My suspicion is that most of the cost is coming from moving memory to/from the heap repeatedly. This is by far the simplest fix, albeit with a cost.

2. We add some thread blocking logic inside RequestValidator, allowing the current thread to acquire a lock around the ComputeHash function. This is probably the best option, as it abstracts the reason for this logic inside the RequestValidator.

3. We do nothing to the code, add the thread blocking logic to the Twilio docs (linked here), and call it a day. Note: There are some other problems with this documentation, so it may be best to just apply the fix here, fix the other issues, and call it a day.

[childish-sambino]

What about option 1 and making the HMACSHA1 object (and the SHA256 object) thread-local? https://docs.microsoft.com/en-us/dotnet/api/system.threading.threadlocal-1

[seannybgoode]

@childish-sambino - I had considered that. However making an object thread local means that it will be initialized by reflection, which is expensive. Not knowing the cost, compared to the other options, I didn't mention it.

I'd need to put some time in to see if the benefit outweighs the cost.

I just tested 3, and it doesn't seem to cause any noticeable issues with our services on my development branch. The more I think about it, the more I like it, as it shortcuts any other unknown threading issues (although the SHA256 member seems unaffected, to my knowledge).

Failing that, we could try 2 like so:

lock(_hmac) { hashresult = _hmac.ComputeHash(...); //_hmac not threadsafe }

As far as I understand, C# can handle locking on any reference variable by that method.

As you said before though, we should test that somehow.

[childish-sambino]

Spent some time wrapping my head around action filters and threading. This helped: https://stackoverflow.com/a/8937793

I'm in favor of making the change in the ValidateRequestExample docs (options 3). Either this:

return new RequestValidator(_authToken).Validate(requestUrl, request.Form, signature);

or this:

lock(_requestValidator)
{
    return _requestValidator.Validate(requestUrl, request.Form, signature);
}

[seannybgoode]

@childish-sambino - The latter is exactly what I did in our systems.

In the ValidateRequestExample, there's also another problem in the 'isValidRequest' method, in that, sometimes the AbsoluteUri member of request.Url is http, sometimes it's https. It seems like the Twilio callbacks are coming back this way for some kinds of callbacks, and not others. This was causing some failures on our end, so we added a little method like so:

private string rewriteUri(string absoluteUri)        
{
            
//check to make sure we're not replacing 'https' with 'httpss'
            
    if (!absoluteUri.Contains("https"))
            
    {
                
        return Regex.Replace(absoluteUri, @"http", "https");
     
    }
            
    return absoluteUri;
        
}

Maybe we can kill two birds, if option 3 is the approach? Would like to help out the community by improving the documentation, if we go with option 3.

[childish-sambino]

The protocol could be related to what's proxying the requests. There was a similar issue in the Node helper lib where they were running an HTTP server but using an HTTPS heroku endpoint. The Twilio server-side signature calculation is based on HTTPS since that's what the webhook URL says, but it was being forwarded to the app by the proxy with HTTP as the protocol.

The Node request validator allows you to pass the expected protocol as an arg which will be used to do the calculation. Would this work?

[seannybgoode]

Given what you're saying about proxies, it could be that we're seeing HTTP sometimes due to testing with Ngrok in our dev environments. I'd need to dig in to see what's happening in production.

I think the trick here is that the expected protocol could be either, depending on the scenario.

[childish-sambino]

Not sure if the library should be doing the HTTP/S proto logic manipulation, but I'm open to discussing.

Regarding changing the example for threading reasons, can you submit a PR here? https://github.com/TwilioDevEd/api-snippets/blob/master/guides/request-validation-csharp/example-1/example-1.cs

[seannybgoode]

@childish-sambino - I'm going to get my colleague @elyahnora to have a look at the HTTP/HTTPS behavior, and get us more information.

Regarding the re-write of the RequestValidator attribute, I will submit a PR as soon as I get a chance.

[thinkingserious]

Since there has been no activity on this issue since March 1, 2020, we are closing this issue. Please feel free to reopen or create a new issue if you still require assistance. Thank you!

[benmccallum]

Bit of a shame this issue was closed without at least an update to the documentation which literally tells you to do the wrong thing and in many cases that won't fail until you hit production and experience enough concurrent load.

Here's a relevant conversation about these algos and their thread safety. It might be worth targeting net6 and using the new one-time hash methods in #if blocks.
dotnet/runtime#61417

Here's a modified version of the repro but with Twilio.Security.RequestValidator. Note, commenting out the thread2 stuff and this will run infinitely, but with it it'll fail with one of a few diff exceptions depending on where it blows up.

using Twilio.Security;

var validator = new RequestValidator("secret");
Thread thread1 = new Thread(static obj => {
    while (true)
    {
        ((RequestValidator)obj).Validate("https://foo.com", "123", "foo");
    }
});
Thread thread2 = new Thread(static obj => {
    while (true)
    {
        ((RequestValidator)obj).Validate("https://foo.com", "123", "foo");
    }
});

thread1.Start(validator);
thread2.Start(validator);
thread1.Join();
thread2.Join();

For now I new up a RequestValidator inside the attribute's OnActionExecuting method, instead of storing one locally and falling into the trap of a singleton attribute lifetime.

Aside
The validator should also dispose of the privately held HMACSHA1 and SHA256 instances too, right? As alluded to above. They're disposable.

[benmccallum]

Actually even new'ing up a RequestValidator each execution doesn't feel like a great fix due to the lack of disposal mentioned above... a lock might be better until those are being disposed.

Edit: Turns out this may just be for correctness, as these algos may not hold onto any unmanaged resources.
dotnet/aspnetcore#32871 Spun up a PR anyway.

[seannybgoode]

I don't disagree with you Ben that calling new on request validator isn't a great fix, but I don't think it should be the calling method's responsibility to guarantee thread safety. We should probably fix the underlying library, I'm just a little coder team at a non profit, I have not had time to fix this for Twilio 😆 😢

[benmccallum]

@seannybgoode , no worries mate. I wasn't giving you feedback 😉 Your investigation and suggestions were great and a huge help, Thanks for creating the issue!

I'll be happy with new instead of a lock if IDisposable is implemented. I created a PR for that.

I'm also happy to create a PR for thread safety ootb if they let me know that the code fix is the way to go, else it really needs to be documented as not thread safe! We hit this on production in our call centre 🙄

@thinkingserious, can you re-open this and let me know:

1. Code fix or doc fix?
2. If code fix, can I add a target net6? (So I can use one-time hash funcs)
3. What would be involved in terms of CI testing if I target net6? (edit to the GitHub action I guess? though that may be locked to me)

Out of interest, what do you think is the right way to go here @vcsjones? Are the one-time hash functions appropriate for a hot path like this? That'd remove the thread safety issue entirely for net6+

[vcsjones]

Out of interest, what do you think is the right way to go here @vcsjones? Are the one-time hash functions appropriate for a hot path like this? That'd remove the thread safety issue entirely for net6+

The one shot HMACs are meant to be an "always right" choice. They do not internally allocate, they are thread safe, and they will usually be the fastest.

If RequestValidator instances are supposed to be thread-safe, then this is problematic:

twilio-csharp/src/Twilio/Security/RequestValidator.cs

Lines 14 to 15 in 6b9ca50

	private readonly HMACSHA1 _hmac;
	private readonly SHA256 _sha;

Both SHA256 and HMACSHA1 are not thread safe. Converting them to the one shots would fix the thread safety.

That said:

twilio-csharp/src/Twilio/Twilio.csproj

Line 3 in 6b9ca50

<TargetFrameworks>netstandard1.4;netstandard2.0;net451;net35</TargetFrameworks>

Since this project appears to support some quite old .NET Framework releases, I think the "best" choice would be to just create and dispose of the algorithm instances:

main...vcsjones:example-thread-safe

I wouldn't say that's "bad" considering we still need to support .NET Framework 3.5. Let's write a benchmark.

The results:

Method	Mean	Error	StdDev	Gen 0	Allocated
HmacCreateDispose	700.8 ns	1.96 ns	1.74 ns	0.1945	408 B
HmacOneShot	319.9 ns	0.59 ns	0.55 ns	0.0229	48 B

So yes, the one shot is faster, and the 48 bytes that got allocated are only because we asked the HMAC to return a byte[].

But, we are still in "nanosecond" territory. Scaling that up to seconds, the HmacCreateDispose is 1,426,900 operations per second, and the one-shot is 3,126,000 operations per second. I don't have enough context to know if that throughput is "good" but it certainly seems reasonable to me.

So, if you want to ".NET 6-ify" to get access to the one shots, there is a benefit but I don't know that adding support for it is worth the trouble. If performance is paramount, then giving .NET 6 users the one shots seems worthwhile. It'd look something like:

byte[] mac;

#if NET6_0_OR_GREATER
mac = HMACSHA1.HashData(_hmacKey, _hmacData);
#else
using (HMACSHA1 hmac = new HMACSHA1(_hmacKey))
{
    mac = hmac.ComputeHash(_hmacData);
}
#endif

// Now do something with "mac" here

You would still need to add a net6 to the TFMs, but this would allow using the one shot, if it is available. How NET6_0_OR_GREATER works is described at https://docs.microsoft.com/en-us/dotnet/fundamentals/package-validation/compatible-framework-in-package-validator

I hope this has been helpful.

[vcsjones]

Edit: Turns out this may just be for correctness, as these algos may not hold onto any unmanaged resources.
dotnet/aspnetcore#32871 Spun up a PR anyway.

For what it's worth, the HMACSHA1 instances do use unmanaged resources. Here's Windows:

https://github.com/dotnet/runtime/blob/a5fda753afce966b9d31c76559636eb515bfa05a/src/libraries/Common/src/Internal/Cryptography/HashProviderCng.cs#L42

So we create at least one BCRYPT_HASH_HANDLE per HMAC instance.

Missing a dispose won't leak because safe handles are used, so the garbage collector and the finalizers should kick in. However, disposing of them when done with them is the correct thing to do.

The one shots use different APIs that do not require them to keep track of the lifetime of a native handle, except on Windows 7, I believe.

[seannybgoode]

@benmccallum - Thank you for the kind words, I'm glad we were able to help diagnose your call center issue. (yikes!)

Glad to see that this issue is getting some traction finally. Thanks all.

[benmccallum]

Thanks @vcsjones , that was very enlightening and well explained! I was thinking along the same lines given the old target frameworks, but I'm glad you backed that up with benchmarks!

Given the docs don't mention this class isn't thread-safe, my take on that is it should be OOTB, so the new approach for now is the way to go (with a note to move onto the one-shot methods in the future).

Certainly for our system when we've only got <30 call centre staff, I think we can safely say the throughput is enough 😄, and I doubt many people are running a system where 1,426,900 ops/sec of Twilio callback verifications is not enough to support their operations; certainly they'd be scaling out at that point anyway!

I'll spin up a PR.

[kevindqc]

I'm pretty disappointed that this hasn't been fixed after having been pointed out for almost 3 years now.. best case an exception is randomly thrown, worst case a segfault randomly kills the process!!! This is a huge issue IMO

We were using basically this implementation from the documentation (can still see it at https://web.archive.org/web/20220630075743/https://www.twilio.com/docs/usage/tutorials/how-to-secure-your-csharp-aspnet-core-app-by-validating-incoming-twilio-requests):

[AttributeUsage(AttributeTargets.Method)]
    public class ValidateTwilioRequestAttribute : ActionFilterAttribute
    {
        private readonly RequestValidator _requestValidator;

        private static IConfigurationRoot Configuration =>
            new ConfigurationBuilder()
                .SetBasePath(Directory.GetCurrentDirectory())
                .AddJsonFile("appsettings.json", true, true).Build();

        public ValidateTwilioRequestAttribute()
        {
            var authToken = Configuration["TwilioAuthToken"];
            _requestValidator = new RequestValidator(authToken);
        }

        public override void OnActionExecuting(ActionExecutingContext actionContext)
        {
            var context = actionContext.HttpContext;
            if (!IsValidRequest(context.Request))
            {
                actionContext.HttpContext.Response.StatusCode = StatusCodes.Status403Forbidden;
            }

            base.OnActionExecuting(actionContext);
        }

        private bool IsValidRequest(HttpRequest request) {
            var requestUrl = RequestRawUrl(request);
            var parameters = ToDictionary(request.Form);
            var signature = request.Headers["X-Twilio-Signature"];
            return _requestValidator.Validate(requestUrl, parameters, signature);
        }

        private static string RequestRawUrl(HttpRequest request)
        {
            return $"{request.Scheme}://{request.Host}{request.Path}{request.QueryString}";
        }

        private static IDictionary<string, string> ToDictionary(IFormCollection collection)
        {
            return collection.Keys
                .Select(key => new { Key = key, Value = collection[key] })
                .ToDictionary(p => p.Key, p => p.Value.ToString());
        }
    }

Which was recently updated to:

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
    public class ValidateTwilioRequestAttribute : TypeFilterAttribute
    {
        public ValidateTwilioRequestAttribute() : base(typeof(ValidateTwilioRequestFilter))
        {
        }
    }

    internal class ValidateTwilioRequestFilter : IAsyncActionFilter
    {
        private readonly RequestValidator _requestValidator;

        public ValidateTwilioRequestFilter(IConfiguration configuration)
        {
            var authToken = configuration["Twilio:AuthToken"] ?? throw new Exception("'Twilio:AuthToken' not configured.");
            _requestValidator = new RequestValidator(authToken);
        }

        public async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
        {
            var httpContext = context.HttpContext;
            var request = httpContext.Request;
        
            var requestUrl = $"{request.Scheme}://{request.Host}{request.Path}{request.QueryString}";
            Dictionary<string, string> parameters = null;
        
            if (request.HasFormContentType)
            {
                var form = await request.ReadFormAsync(httpContext.RequestAborted).ConfigureAwait(false);
                parameters = form.ToDictionary(p => p.Key, p => p.Value.ToString());
            }

            var signature = request.Headers["X-Twilio-Signature"];
            var isValid = _requestValidator.Validate(requestUrl, parameters, signature);
        
            if (!isValid)
            {
                httpContext.Response.StatusCode = StatusCodes.Status403Forbidden;
                return;
            }

            await next();
        }
    }

and it seems to fix the race condition at least.

[dtown123]

Not sure why this was closed - this issue still remains.

Try executing the following code which sort of simulates a load. Sometimes it works without issue and generates a valid response. Sometimes it doesn't. After reverse engineering the Validate() method I did notice that adding a lock seems to fix the problem.

using Twilio.Security;

const int Iterations = 10;

var authToken = "test";
var validator = new RequestValidator(authToken);

var url = "http://localhost:5000/twiliosmscallback";
var signature = "/2T2M82TLL2fOMfPxzvmjpVy5Tw=";
var validParameters = new Dictionary<string, string>
{
    {"ToCountry", "US"},
    {"From", "+12145551212"},
    {"ApiVersion", "2010-04-01"}
};

var tasks = new List<Task>();
var results = new ConcurrentBag<bool>();

for (var i = 0; i < Iterations; i++)
{
    tasks.Add(Task.Run(() =>
    {
        try
        {
            var result = validator.Validate(url, validParameters, signature);
            results.Add(result);
        }
        catch (Exception ex)
        {
            Console.WriteLine($"ERROR: {ex.Message}");
        }
    }));
}

await Task.WhenAll(tasks);

for (var i = 0; i < results.Count; i++)
{
    var result = results.ElementAt(i) ? "Valid" : "Invalid";
    Console.WriteLine($"Verifying result at index {i}: {result}");
}