[2.11.0-beta1] guard against malicious headers in quickpulse (#1191)

* guard against malicious headers in quickpulse

* remove unused variable

* using not needed

* fixing compile issues

* update

* changelog

CHANGELOG.md

@@ -8,6 +8,7 @@
  - [Defer populating RequestTelemetry properties.](https://github.com/Microsoft/ApplicationInsights-dotnet-server/issues/1173)
  - [Azure Web App for Windows Containers to use regular PerfCounter mechanism.](https://github.com/microsoft/ApplicationInsights-dotnet-server/pull/1167)
  - [Support for Process CPU and Process Memory perf counters in all platforms including Linux.](https://github.com/microsoft/ApplicationInsights-dotnet-server/issues/1189)
+ - SDL: [Guard against malicious headers in quickpulse](https://github.com/microsoft/ApplicationInsights-dotnet-server/pull/1191)
 
 ## Version 2.10.0
 - Updated Base SDK to 2.10.0

Src/Common/InjectionGuardConstants.cs

@@ -50,5 +50,10 @@ namespace Microsoft.ApplicationInsights.Common.Internal
         /// Max number of key value pairs in the tracestate header.
         /// </summary>
         public const int TraceStateMaxPairs = 32;
+
+        /// <summary>
+        /// Max length of incoming Response Header value allowed.
+        /// </summary>
+        public const int QuickPulseResponseHeaderMaxLength = 1024;
     }
 }

Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClient.cs

@@ -172,8 +172,7 @@
         {
             configurationInfo = null;
 
-            bool isSubscribed;
-            if (!bool.TryParse(response.Headers.GetValuesSafe(QuickPulseConstants.XMsQpsSubscribedHeaderName).FirstOrDefault(), out isSubscribed))
+            if (!bool.TryParse(response.Headers.GetValueSafe(QuickPulseConstants.XMsQpsSubscribedHeaderName), out bool isSubscribed))
             {
                 // could not parse the isSubscribed value
 
@@ -192,10 +191,10 @@
 
             foreach (string headerName in QuickPulseConstants.XMsQpsAuthOpaqueHeaderNames)
             {
-                this.authOpaqueHeaderValues[headerName] = response.Headers.GetValuesSafe(headerName).FirstOrDefault();
+                this.authOpaqueHeaderValues[headerName] = response.Headers.GetValueSafe(headerName);
             }
 
-            string configurationETagHeaderValue = response.Headers.GetValuesSafe(QuickPulseConstants.XMsQpsConfigurationETagHeaderName).FirstOrDefault();
+            string configurationETagHeaderValue = response.Headers.GetValueSafe(QuickPulseConstants.XMsQpsConfigurationETagHeaderName);
 
             try
             {

Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClientHelpers.cs

@@ -1,18 +1,22 @@
 namespace Microsoft.ApplicationInsights.Extensibility.PerfCounterCollector.Implementation.QuickPulse
 {
-    using System.Collections.Generic;
+    using System.Linq;
     using System.Net.Http.Headers;
-    using Microsoft.ApplicationInsights.Common;
+    using Microsoft.ApplicationInsights.Common.Internal;
 
     internal static class QuickPulseServiceClientHelpers
     {
-        private static readonly string[] emptyResult = ArrayExtensions.Empty<string>();
-
-        public static IEnumerable<string> GetValuesSafe(this HttpResponseHeaders headers, string name)
+        public static string GetValueSafe(this HttpHeaders headers, string name)
         {
-            IEnumerable<string> result = (headers?.Contains(name) ?? false) ? headers.GetValues(name) : emptyResult;
+            string value = default(string);
 
-            return result;
+            if (headers?.Contains(name) ?? false)
+            {
+                value = headers.GetValues(name).First();
+                value = StringUtilities.EnforceMaxLength(value, InjectionGuardConstants.QuickPulseResponseHeaderMaxLength);
+            }
+            
+            return value;
         }
     }
 }

Src/PerformanceCollector/Perf.Shared.Tests/Perf.Shared.Tests.projitems

@@ -52,6 +52,9 @@
     <Compile Include="$(MSBuildThisFileDirectory)WebAppPerformanceCollector\RatioCounterTests.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)WebAppPerformanceCollector\SumUpCountersGaugeTests.cs" />
   </ItemGroup>
+  <ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp1.0' OR '$(TargetFramework)' == 'netcoreapp2.0'">
+    <Compile Include="$(MSBuildThisFileDirectory)QuickPulse\QuickPulseServiceClientHelpersTests.cs" />
+  </ItemGroup>
   <ItemGroup>
     <None Include="$(MSBuildThisFileDirectory)WebAppPerformanceCollector\SampleFiles\RemoteEnvironmentVariablesAllSampleOne.json">
       <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>

Src/PerformanceCollector/Perf.Shared.Tests/QuickPulse/QuickPulseServiceClientHelpersTests.cs

@@ -0,0 +1,82 @@
+namespace Microsoft.ApplicationInsights.Extensibility.PerfCounterCollector.Tests.QuickPulse
+{
+    using System.Net.Http.Headers;
+    using Microsoft.ApplicationInsights.Common.Internal;
+    using Microsoft.ApplicationInsights.Extensibility.PerfCounterCollector.Implementation.QuickPulse;
+    using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+    [TestClass]
+    public class QuickPulseServiceClientHelpersTests
+    {
+        private const int QuickPulseResponseHeaderHeaderMaxLength = InjectionGuardConstants.QuickPulseResponseHeaderMaxLength;
+        private const string SecretString = "12345abcd";
+        private const string HeaderName = "myheader";
+        private const string FakeHeaderName = "fake";
+
+        [TestMethod]
+        public void VerifyHeaderGetValue()
+        {
+            // setup
+            var headers = new TestHttpHeaders();
+            foreach (string headerName in QuickPulseConstants.XMsQpsAuthOpaqueHeaderNames)
+            {
+                headers.Add(headerName, headerName + SecretString);
+            }
+
+            // assert
+            foreach (string headerName in QuickPulseConstants.XMsQpsAuthOpaqueHeaderNames)
+            {
+                var value = headers.GetValueSafe(headerName);
+                Assert.AreEqual(headerName + SecretString, value);
+            }
+        }
+
+        [TestMethod]
+        public void VerifyHeaderGetValues()
+        {
+            // setup
+            var headers = new TestHttpHeaders();
+            headers.Add(HeaderName, "one");
+            headers.Add(HeaderName, "two");
+            headers.Add(HeaderName, "three");
+
+            // assert
+            var result = headers.GetValueSafe(HeaderName);
+            Assert.AreEqual("one", result);
+
+            var result2 = headers.GetValueSafe(FakeHeaderName);
+            Assert.AreEqual(default(string), result2);
+        }
+
+        [TestMethod]
+        public void VerifyHeadersLengthIsProtected()
+        {
+            // setup
+            var headers = new TestHttpHeaders();
+            headers.Add(HeaderName, new string('*', QuickPulseResponseHeaderHeaderMaxLength));
+
+            // assert
+            var result = headers.GetValueSafe(HeaderName);
+            Assert.AreEqual(QuickPulseResponseHeaderHeaderMaxLength, result.Length);
+        }
+
+        [TestMethod]
+        public void VerifyInvalidHeadersLengthIsProtected()
+        {
+            // setup
+            var headers = new TestHttpHeaders();
+            headers.Add(HeaderName, new string('*', QuickPulseResponseHeaderHeaderMaxLength + 1));
+
+            // assert
+            var result = headers.GetValueSafe(HeaderName);
+            Assert.AreEqual(QuickPulseResponseHeaderHeaderMaxLength, result.Length);
+        }
+
+        /// <summary>
+        /// HttpHeaders is an abstract class. I need to initialize my own class for tests. The class I'm testing is built on HttpHeaders so this is fine.
+        /// </summary>
+        private class TestHttpHeaders : HttpHeaders
+        {
+        }
+    }
+}