Tilee/fix sdl injection bug

CHANGELOG.md

@@ -1,12 +1,17 @@
 # Changelog
 
+## Version 2.6.0-beta2
+- [Added a max length restriction to values passed in through requests.](https://github.com/Microsoft/ApplicationInsights-dotnet-server/pull/810)
+
+
 ## Version 2.6.0-beta1
 - [Fix: Dependency Telemetry is not collected with DiagnosticSource when response does not have content.](https://github.com/Microsoft/ApplicationInsights-dotnet-server/issues/739)
 - [Improve DependencyCollectorEventSource.Log.CurrentActivityIsNull](https://github.com/Microsoft/ApplicationInsights-dotnet-server/issues/799)
 
 **Project**
 - A significant number of upgrades to our testing infrastructure.
 
+
 ## Version 2.5.0
 - [Fix: System.InvalidCastException for SQL Dependency](https://github.com/Microsoft/ApplicationInsights-dotnet-server/issues/782)
 

Src/Common/Common.projitems

@@ -15,7 +15,9 @@
     <Compile Include="$(MSBuildThisFileDirectory)ExceptionUtilities.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)HeadersUtilities.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)ICorrelationIdLookupHelper.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)InjectionGuardConstants.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)RequestResponseHeaders.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)StringUtilities.cs" />
   </ItemGroup>
   <ItemGroup Condition=" '$(TargetFramework)' != 'netstandard1.6' ">
     <Compile Include="$(MSBuildThisFileDirectory)WebHeaderCollectionExtensions.cs" />

Src/Common/CorrelationIdLookupHelper.cs

@@ -190,8 +190,12 @@ public bool IsFetchAppInProgress(string ikey)
         /// </summary>
         /// <param name="ikey">Instrumentation Key is expected to be a Guid string.</param>
         /// <param name="appId">Application Id is expected to be a Guid string. App Id needs to be Http Header safe, and all non-ASCII characters will be removed.</param>
+        /// <remarks>To protect against injection attacks, AppId will be truncated to a maximum length.</remarks>
         private void GenerateCorrelationIdAndAddToDictionary(string ikey, string appId)
         {
+            // Arbitrary maximum length to guard against injections.
+            appId = StringUtilities.EnforceMaxLength(appId, InjectionGuardConstants.AppIdMaxLengeth);
+
             if (string.IsNullOrWhiteSpace(appId))
             {
                 return;

Src/Common/HeadersUtilities.cs

@@ -25,7 +25,8 @@ public static string GetHeaderKeyValue(IEnumerable<string> headerValues, string
                     string[] keyNameValueParts = keyNameValue.Trim().Split('=');
                     if (keyNameValueParts.Length == 2 && keyNameValueParts[0].Trim() == keyName)
                     {
-                        return keyNameValueParts[1].Trim();
+                        string value = keyNameValueParts[1].Trim();
+                        return StringUtilities.EnforceMaxLength(value, InjectionGuardConstants.RequestHeaderMaxLength);
                     }
                 }
             }
@@ -47,7 +48,9 @@ public static IDictionary<string, string> GetHeaderDictionary(IEnumerable<string
                         string keyName = keyNameValueParts[0].Trim();
                         if (!result.ContainsKey(keyName))
                         {
-                            result.Add(keyName, keyNameValueParts[1].Trim());
+                            string value = keyNameValueParts[1].Trim();
+                            value = StringUtilities.EnforceMaxLength(value, InjectionGuardConstants.RequestHeaderMaxLength);
+                            result.Add(keyName, value);
                         }
                     }
                 }

Src/Common/InjectionGuardConstants.cs

@@ -0,0 +1,20 @@
+namespace Microsoft.ApplicationInsights.Common
+{
+    /// <summary>
+    /// These values are listed to guard against malicious injections by limiting the max size allowed in an HTTP Response.
+    /// These max limits are intentionally exaggerated to allow for unexpected responses, while still guarding against unreasonably large responses.
+    /// Example: While a 32 character response may be expected, 50 characters may be permitted while a 10,000 character response would be unreasonable and malicious.
+    /// </summary>
+    public static class InjectionGuardConstants
+    {
+        /// <summary>
+        /// Max length of AppId allowed in response from Breeze.
+        /// </summary>
+        public const int AppIdMaxLengeth = 50;
+
+        /// <summary>
+        /// Max length of incoming Request Header value allowed.
+        /// </summary>
+        public const int RequestHeaderMaxLength = 1024;
+    }
+}

Src/Common/StringUtilities.cs

@@ -0,0 +1,26 @@
+namespace Microsoft.ApplicationInsights.Common
+{
+    using System.Diagnostics;
+
+    /// <summary>
+    /// Generic functions to perform common operations on a string.
+    /// </summary>
+    public static class StringUtilities
+    {
+        /// <summary>
+        /// Check a strings length and trim to a max length if needed.
+        /// </summary>
+        public static string EnforceMaxLength(string input, int maxLength)
+        {
+            Debug.Assert(input != null, $"{nameof(input)} must not be null");
+            Debug.Assert(maxLength > 0, $"{nameof(maxLength)} must be greater than 0");
+
+            if (input != null && input.Length > maxLength)
+            {
+                input = input.Substring(0, maxLength);
+            }
+
+            return input;
+        }
+    }
+}

Src/DependencyCollector/Shared.Tests/CorrelationIdLookupHelperTests.cs

@@ -24,9 +24,7 @@ public void CorrelationIdLookupHelperReturnsAppIdOnSecondCall()
             var correlationIdLookupHelper = new CorrelationIdLookupHelper((ikey) =>
             {
                 // Pretend App Id is the same as Ikey
-                var tcs = new TaskCompletionSource<string>();
-                tcs.SetResult(ikey);
-                return tcs.Task;
+                return Task.FromResult(ikey);
             });
 
             string instrumenationKey = Guid.NewGuid().ToString();
@@ -44,5 +42,40 @@ public void CorrelationIdLookupHelperReturnsAppIdOnSecondCall()
             // Once fetch is complete, subsequent calls should return correlation id.
             Assert.IsTrue(correlationIdLookupHelper.TryGetXComponentCorrelationId(instrumenationKey, out cid));
         }
+
+        /// <summary>
+        /// Test that if an malicious value is returned, that value will be truncated.
+        /// </summary>
+        [TestMethod]
+        public void CorrelationIdLookupHelperTruncatesMaliciousValue()
+        {
+            // 50 character string.
+            var value = "a123456789b123546789c123456789d123456798e123456789";
+
+            // An arbitrary string that is expected to be truncated.
+            var malicious = "00000000000000000000000000000000000000000000000000000000000";
+
+            var cidPrefix = "cid-v1:";
+
+            var correlationIdLookupHelper = new CorrelationIdLookupHelper((ikey) =>
+            {
+                return Task.FromResult(value + malicious);
+            });
+
+            string instrumenationKey = Guid.NewGuid().ToString();
+
+            // first request fails because this will create the fetch task.
+            Assert.IsFalse(correlationIdLookupHelper.TryGetXComponentCorrelationId(instrumenationKey, out string ignore));
+
+            // Let's wait for the task to complete. It should be really quick (based on the test setup) but not immediate.
+            while (correlationIdLookupHelper.IsFetchAppInProgress(instrumenationKey))
+            {
+                Thread.Sleep(10); // wait 10 ms.
+            }
+
+            // Once fetch is complete, subsequent calls should return correlation id.
+            Assert.IsTrue(correlationIdLookupHelper.TryGetXComponentCorrelationId(instrumenationKey, out string cid));
+            Assert.AreEqual(cidPrefix + value, cid);
+        }
     }
 }

Src/Web/Web.Net45/Implementation/HttpRequestExtensions.cs

@@ -3,6 +3,7 @@
     using System;
     using System.Collections.Specialized;
     using System.Web;
+    using Microsoft.ApplicationInsights.Common;
 
     /// <summary>
     /// HttpRequest Extensions.
@@ -16,7 +17,8 @@ public static HttpCookie UnvalidatedGetCookie(this HttpRequest httpRequest, stri
 
         public static string UnvalidatedGetHeader(this HttpRequest httpRequest, string headerName)
         {
-            return httpRequest.Unvalidated.Headers[headerName];
+            string value = httpRequest.Unvalidated.Headers[headerName];
+            return StringUtilities.EnforceMaxLength(value, InjectionGuardConstants.RequestHeaderMaxLength);
         }
 
         public static Uri UnvalidatedGetUrl(this HttpRequest httpRequest)

Src/Web/Web.Shared.Net/RequestTrackingTelemetryModule.cs

@@ -475,6 +475,7 @@ internal bool OnEndRequest_ShouldLog(HttpContext context)
                 try
                 {
                     var rootRequestId = headers[HeaderRootRequestId];
+                    rootRequestId = StringUtilities.EnforceMaxLength(rootRequestId, InjectionGuardConstants.RequestHeaderMaxLength);
                     if (rootRequestId != null)
                     {
                         if (!this.IsRequestKnown(rootRequestId))