twilio-csharp: Possible thread safety issue with RequestValidator.cs causing System.Security.Cryptography.CryptographicException

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

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

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 9
Comments: 20 (6 by maintainers)

Most upvoted comments

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.

kevindqc on Jan 26, 2023

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

thinkingserious on Oct 21, 2019

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. https://github.com/dotnet/runtime/issues/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 on Jan 31, 2022