Merge pull request #5715 from umbraco/v7/bugfix/AB-1085/antiforgery-per-form

Custom IAntiForgeryAdditionalDataProvider implementation

src/Umbraco.Tests/Security/UmbracoAntiForgeryAdditionalDataProviderTests.cs

@@ -0,0 +1,157 @@
+using System.Collections.Specialized;
+using System.Web;
+using System.Web.Helpers;
+using Moq;
+using Newtonsoft.Json;
+using NUnit.Framework;
+using Umbraco.Core;
+using Umbraco.Tests.TestHelpers;
+using Umbraco.Web.Mvc;
+using Umbraco.Web.Security;
+
+namespace Umbraco.Tests.Security
+{
+    [TestFixture]
+    public class UmbracoAntiForgeryAdditionalDataProviderTests
+    {
+        [Test]
+        public void Test_Wrapped_Non_BeginUmbracoForm()
+        {
+            var wrapped = Mock.Of<IAntiForgeryAdditionalDataProvider>(x => x.GetAdditionalData(It.IsAny<HttpContextBase>()) == "custom");
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(wrapped);
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            var data = provider.GetAdditionalData(httpContextFactory.HttpContext);
+
+            Assert.IsTrue(data.DetectIsJson());
+            var json = JsonConvert.DeserializeObject<UmbracoAntiForgeryAdditionalDataProvider.AdditionalData>(data);
+            Assert.AreEqual(null, json.Ufprt);
+            Assert.IsTrue(json.Stamp != default);
+            Assert.AreEqual("custom", json.WrappedValue);
+        }
+
+        [Test]
+        public void Null_Wrapped_Non_BeginUmbracoForm()
+        {
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(null);
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            var data = provider.GetAdditionalData(httpContextFactory.HttpContext);
+
+            Assert.IsTrue(data.DetectIsJson());
+            var json = JsonConvert.DeserializeObject<UmbracoAntiForgeryAdditionalDataProvider.AdditionalData>(data);
+            Assert.AreEqual(null, json.Ufprt);
+            Assert.IsTrue(json.Stamp != default);
+            Assert.AreEqual("default", json.WrappedValue);
+        }
+
+        [Test]
+        public void Validate_Non_Json()
+        {
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(null);
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "hello");
+
+            Assert.IsFalse(isValid);
+        }
+
+        [Test]
+        public void Validate_Invalid_Json()
+        {
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(null);
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '0'}");
+            Assert.IsFalse(isValid);
+
+            isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': ''}");
+            Assert.IsFalse(isValid);
+
+            isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'hello': 'world'}");
+            Assert.IsFalse(isValid);
+
+        }
+
+        [Test]
+        public void Validate_No_Request_Ufprt()
+        {
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(null);
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            //there is a ufprt in the additional data, but not in the request
+            var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': 'ASBVDFDFDFDF'}");
+            Assert.IsFalse(isValid);
+        }
+
+        [Test]
+        public void Validate_No_AdditionalData_Ufprt()
+        {
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(null);
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            var requestMock = Mock.Get(httpContextFactory.HttpContext.Request);
+            requestMock.SetupGet(x => x["ufprt"]).Returns("ABCDEFG");
+
+            //there is a ufprt in the additional data, but not in the request
+            var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': ''}");
+            Assert.IsFalse(isValid);
+        }
+
+        [Test]
+        public void Validate_No_AdditionalData_Or_Request_Ufprt()
+        {
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(null);
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            
+            //there is a ufprt in the additional data, but not in the request
+            var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': ''}");
+            Assert.IsTrue(isValid);
+        }
+
+        [Test]
+        public void Validate_Request_And_AdditionalData_Ufprt()
+        {
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(null);
+
+            var routeParams1 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco";
+            var routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco";
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            var requestMock = Mock.Get(httpContextFactory.HttpContext.Request);
+            requestMock.SetupGet(x => x["ufprt"]).Returns(routeParams1.EncryptWithMachineKey());
+
+            var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}");
+            Assert.IsTrue(isValid);
+
+            routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Invalid")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco";
+            isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}");
+            Assert.IsFalse(isValid);
+        }
+
+        [Test]
+        public void Validate_Wrapped_Request_And_AdditionalData_Ufprt()
+        {
+            var wrapped = Mock.Of<IAntiForgeryAdditionalDataProvider>(x => x.ValidateAdditionalData(It.IsAny<HttpContextBase>(), "custom") == true);
+            var provider = new UmbracoAntiForgeryAdditionalDataProvider(wrapped);
+
+            var routeParams1 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco";
+            var routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Test")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco";
+
+            var httpContextFactory = new FakeHttpContextFactory("/hello/world");
+            var requestMock = Mock.Get(httpContextFactory.HttpContext.Request);
+            requestMock.SetupGet(x => x["ufprt"]).Returns(routeParams1.EncryptWithMachineKey());
+
+            var isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}");
+            Assert.IsFalse(isValid);
+
+            isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'custom', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}");
+            Assert.IsTrue(isValid);
+
+            routeParams2 = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode("Invalid")}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode("Index")}&{RenderRouteHandler.ReservedAdditionalKeys.Area}=Umbraco";
+            isValid = provider.ValidateAdditionalData(httpContextFactory.HttpContext, "{'Stamp': '636970328040070330', 'WrappedValue': 'default', 'Ufprt': '" + routeParams2.EncryptWithMachineKey() + "'}");
+            Assert.IsFalse(isValid);
+        }
+    }
+}

src/Umbraco.Tests/Umbraco.Tests.csproj

@@ -197,6 +197,7 @@
     <Compile Include="Packaging\PackageExtractionTests.cs" />
     <Compile Include="Persistence\Repositories\SimilarNodeNameTests.cs" />
     <Compile Include="PublishedContent\StronglyTypedModels\Home.cs" />
+    <Compile Include="Security\UmbracoAntiForgeryAdditionalDataProviderTests.cs" />
     <Compile Include="Services\AuditServiceTests.cs" />
     <Compile Include="Services\ConsentServiceTests.cs" />
     <Compile Include="Services\MemberGroupServiceTests.cs" />

src/Umbraco.Web/Mvc/RenderRouteHandler.cs

@@ -17,7 +17,7 @@ namespace Umbraco.Web.Mvc
     public class RenderRouteHandler : IRouteHandler
     {
         // Define reserved dictionary keys for controller, action and area specified in route additional values data
-        private static class ReservedAdditionalKeys
+        internal static class ReservedAdditionalKeys
         {
             internal const string Controller = "c";
             internal const string Action = "a";
@@ -142,36 +142,7 @@ namespace Umbraco.Web.Mvc
                     return null;
             }
 
-
-            string decryptedString;
-            try
-            {
-                decryptedString = encodedVal.DecryptWithMachineKey();
-            }
-            catch (FormatException)
-            {
-                LogHelper.Warn<RenderRouteHandler>("A value was detected in the ufprt parameter but Umbraco could not decrypt the string");
-                return null;
-            }
-
-            var parsedQueryString = HttpUtility.ParseQueryString(decryptedString);
-            var decodedParts = new Dictionary<string, string>();
-
-            foreach (var key in parsedQueryString.AllKeys)
-            {
-                decodedParts[key] = parsedQueryString[key];
-            }
-
-            //validate all required keys exist
-
-            //the controller
-            if (decodedParts.All(x => x.Key != ReservedAdditionalKeys.Controller))
-                return null;
-            //the action
-            if (decodedParts.All(x => x.Key != ReservedAdditionalKeys.Action))
-                return null;
-            //the area
-            if (decodedParts.All(x => x.Key != ReservedAdditionalKeys.Area))
+            if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(encodedVal, out var decodedParts))
                 return null;
 
             foreach (var item in decodedParts.Where(x => new[] {
@@ -192,6 +163,8 @@ namespace Umbraco.Web.Mvc
             };
         }
 
+       
+
         /// <summary>
         /// Handles a posted form to an Umbraco Url and ensures the correct controller is routed to and that
         /// the right DataTokens are set.

src/Umbraco.Web/Security/UmbracoAntiForgeryAdditionalDataProvider.cs

@@ -0,0 +1,91 @@
+using System;
+using Umbraco.Web.Mvc;
+using Umbraco.Core;
+using System.Web.Helpers;
+using System.Web;
+using Newtonsoft.Json;
+
+namespace Umbraco.Web.Security
+{
+    /// <summary>
+    /// A custom <see cref="IAntiForgeryAdditionalDataProvider"/> to create a unique antiforgery token/validator per form created with BeginUmbracoForm
+    /// </summary>
+    public class UmbracoAntiForgeryAdditionalDataProvider : IAntiForgeryAdditionalDataProvider
+    {
+        private readonly IAntiForgeryAdditionalDataProvider _defaultProvider;
+
+        /// <summary>
+        /// Constructor, allows wrapping a default provider
+        /// </summary>
+        /// <param name="defaultProvider"></param>
+        public UmbracoAntiForgeryAdditionalDataProvider(IAntiForgeryAdditionalDataProvider defaultProvider)
+        {
+            _defaultProvider = defaultProvider;
+        }
+
+        public string GetAdditionalData(HttpContextBase context)
+        {
+            return JsonConvert.SerializeObject(new AdditionalData
+            {
+                Stamp = DateTime.UtcNow.Ticks,
+                //this value will be here if this is a BeginUmbracoForms form
+                Ufprt = context.Items["ufprt"]?.ToString(),
+                //if there was a wrapped provider, add it's value to the json, else just a static value
+                WrappedValue = _defaultProvider?.GetAdditionalData(context) ?? "default"
+            });
+        }
+
+        public bool ValidateAdditionalData(HttpContextBase context, string additionalData)
+        {
+            if (!additionalData.DetectIsJson())
+                return false; //must be json
+
+            AdditionalData json;
+            try
+            {
+                json = JsonConvert.DeserializeObject<AdditionalData>(additionalData);
+            }
+            catch
+            {
+                return false; //couldn't parse
+            }
+
+            if (json.Stamp == default) return false;
+
+            //if there was a wrapped provider, validate it, else validate the static value
+            var validateWrapped = _defaultProvider?.ValidateAdditionalData(context, json.WrappedValue) ?? json.WrappedValue == "default";
+            if (!validateWrapped)
+                return false;
+
+            var ufprtRequest = context.Request["ufprt"]?.ToString();
+
+            //if the custom BeginUmbracoForms route value is not there, then it's nothing more to validate
+            if (ufprtRequest.IsNullOrWhiteSpace() && json.Ufprt.IsNullOrWhiteSpace())
+                return true;
+
+            //if one or the other is null then something is wrong
+            if (!ufprtRequest.IsNullOrWhiteSpace() && json.Ufprt.IsNullOrWhiteSpace()) return false;
+            if (ufprtRequest.IsNullOrWhiteSpace() && !json.Ufprt.IsNullOrWhiteSpace()) return false;
+
+            if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(json.Ufprt, out var additionalDataParts))
+                return false;
+
+            if (!UmbracoHelper.DecryptAndValidateEncryptedRouteString(ufprtRequest, out var requestParts))
+                return false;
+
+            //ensure they all match
+            return additionalDataParts.Count == requestParts.Count
+                && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Controller] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Controller]
+                && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Action] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Action]
+                && additionalDataParts[RenderRouteHandler.ReservedAdditionalKeys.Area] == requestParts[RenderRouteHandler.ReservedAdditionalKeys.Area];
+        }
+
+        internal class AdditionalData
+        {
+            public string Ufprt { get; set; }
+            public long Stamp { get; set; }
+            public string WrappedValue { get; set; }
+        }
+
+    }
+}

src/Umbraco.Web/UmbracoHelper.cs

@@ -10,9 +10,11 @@ using System.Xml.XPath;
 using Umbraco.Core;
 using Umbraco.Core.Dictionary;
 using Umbraco.Core.Dynamics;
+using Umbraco.Core.Logging;
 using Umbraco.Core.Models;
 using Umbraco.Core.Services;
 using Umbraco.Core.Xml;
+using Umbraco.Web.Mvc;
 using Umbraco.Web.Routing;
 using Umbraco.Web.Security;
 
@@ -1647,6 +1649,43 @@ namespace Umbraco.Web
 
         #endregion
 
+        internal static bool DecryptAndValidateEncryptedRouteString(string ufprt, out IDictionary<string, string> parts)
+        {
+            string decryptedString;
+            try
+            {
+                decryptedString = ufprt.DecryptWithMachineKey();
+            }
+            catch (FormatException)
+            {
+                LogHelper.Warn<UmbracoHelper>("A value was detected in the ufprt parameter but Umbraco could not decrypt the string");
+                parts = null;
+                return false;
+            }
+
+            var parsedQueryString = HttpUtility.ParseQueryString(decryptedString);
+            parts = new Dictionary<string, string>();
+
+            foreach (var key in parsedQueryString.AllKeys)
+            {
+                parts[key] = parsedQueryString[key];
+            }
+
+            //validate all required keys exist
+
+            //the controller
+            if (parts.All(x => x.Key != RenderRouteHandler.ReservedAdditionalKeys.Controller))
+                return false;
+            //the action
+            if (parts.All(x => x.Key != RenderRouteHandler.ReservedAdditionalKeys.Action))
+                return false;
+            //the area
+            if (parts.All(x => x.Key != RenderRouteHandler.ReservedAdditionalKeys.Area))
+                return false;
+
+            return true;
+        }
+
         /// <summary>
         /// This is used in methods like BeginUmbracoForm and SurfaceAction to generate an encrypted string which gets submitted in a request for which
         /// Umbraco can decrypt during the routing process in order to delegate the request to a specific MVC Controller.
@@ -1663,10 +1702,7 @@ namespace Umbraco.Web
             Mandate.ParameterNotNull(area, "area");
 
             //need to create a params string as Base64 to put into our hidden field to use during the routes
-            var surfaceRouteParams = string.Format("c={0}&a={1}&ar={2}",
-                                                      HttpUtility.UrlEncode(controllerName),
-                                                      HttpUtility.UrlEncode(controllerAction),
-                                                      area);
+            var surfaceRouteParams = $"{RenderRouteHandler.ReservedAdditionalKeys.Controller}={HttpUtility.UrlEncode(controllerName)}&{RenderRouteHandler.ReservedAdditionalKeys.Action}={HttpUtility.UrlEncode(controllerAction)}&{RenderRouteHandler.ReservedAdditionalKeys.Area}={area}";
 
             //checking if the additional route values is already a dictionary and convert to querystring
             string additionalRouteValsAsQuery;

src/Umbraco.Web/WebBootManager.cs

@@ -47,10 +47,12 @@ using Umbraco.Web.Profiling;
 using Umbraco.Web.Search;
 using GlobalSettings = Umbraco.Core.Configuration.GlobalSettings;
 using ProfilingViewEngine = Umbraco.Core.Profiling.ProfilingViewEngine;
-
+using System.Web.Helpers;
+using Umbraco.Web.Controllers;
 
 namespace Umbraco.Web
 {
+
     /// <summary>
     /// A bootstrapper for the Umbraco application which initializes all objects including the Web portion of the application
     /// </summary>
@@ -188,6 +190,8 @@ namespace Umbraco.Web
 
             base.Complete(afterComplete);
 
+            AntiForgeryConfig.AdditionalDataProvider = new UmbracoAntiForgeryAdditionalDataProvider(AntiForgeryConfig.AdditionalDataProvider);
+
             //Now, startup all of our legacy startup handler
             ApplicationEventsResolver.Current.InstantiateLegacyStartupHandlers();
 
@@ -281,7 +285,7 @@ namespace Umbraco.Web
                     // to worker A again, in theory the %temp%  folder should already be empty but we really want to make sure that its not
                     // utilizing an old path
                     appDomainHash);
-                
+
                 //set the file map and composite file default location to the %temp% location
                 BaseCompositeFileProcessingProvider.CompositeFilePathDefaultFolder
                     = XmlFileMapper.FileMapDefaultFolder