diff --git a/CHANGELOG.md b/CHANGELOG.md index 755f804d8..6a3d22a40 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Src/Common/InjectionGuardConstants.cs b/Src/Common/InjectionGuardConstants.cs index 9207562de..85b68ddd7 100644 --- a/Src/Common/InjectionGuardConstants.cs +++ b/Src/Common/InjectionGuardConstants.cs @@ -50,5 +50,10 @@ static class InjectionGuardConstants /// Max number of key value pairs in the tracestate header. /// public const int TraceStateMaxPairs = 32; + + /// + /// Max length of incoming Response Header value allowed. + /// + public const int QuickPulseResponseHeaderMaxLength = 1024; } } diff --git a/Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClient.cs b/Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClient.cs index 02a1784ad..9c25f1714 100644 --- a/Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClient.cs +++ b/Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClient.cs @@ -172,8 +172,7 @@ public void Dispose() { 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 @@ public void Dispose() 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 { diff --git a/Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClientHelpers.cs b/Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClientHelpers.cs index 3e986bf8e..d8c0eb87e 100644 --- a/Src/PerformanceCollector/Perf.Shared.NetStandard/Implementation/QuickPulse/QuickPulseServiceClientHelpers.cs +++ b/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(); - - public static IEnumerable GetValuesSafe(this HttpResponseHeaders headers, string name) + public static string GetValueSafe(this HttpHeaders headers, string name) { - IEnumerable 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; } } } diff --git a/Src/PerformanceCollector/Perf.Shared.Tests/Perf.Shared.Tests.projitems b/Src/PerformanceCollector/Perf.Shared.Tests/Perf.Shared.Tests.projitems index d7551760e..ce1ea9609 100644 --- a/Src/PerformanceCollector/Perf.Shared.Tests/Perf.Shared.Tests.projitems +++ b/Src/PerformanceCollector/Perf.Shared.Tests/Perf.Shared.Tests.projitems @@ -52,6 +52,9 @@ + + + PreserveNewest diff --git a/Src/PerformanceCollector/Perf.Shared.Tests/QuickPulse/QuickPulseServiceClientHelpersTests.cs b/Src/PerformanceCollector/Perf.Shared.Tests/QuickPulse/QuickPulseServiceClientHelpersTests.cs new file mode 100644 index 000000000..d42b874f2 --- /dev/null +++ b/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); + } + + /// + /// 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. + /// + private class TestHttpHeaders : HttpHeaders + { + } + } +}