diff --git a/CHANGELOG.md b/CHANGELOG.md index f543a77c4..7c5f0810c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # 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) @@ -7,6 +11,7 @@ **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) diff --git a/Src/Common/Common.projitems b/Src/Common/Common.projitems index a41c46e8e..40e933606 100644 --- a/Src/Common/Common.projitems +++ b/Src/Common/Common.projitems @@ -15,7 +15,9 @@ + + diff --git a/Src/Common/CorrelationIdLookupHelper.cs b/Src/Common/CorrelationIdLookupHelper.cs index 93241aa53..e21112e81 100644 --- a/Src/Common/CorrelationIdLookupHelper.cs +++ b/Src/Common/CorrelationIdLookupHelper.cs @@ -190,8 +190,12 @@ public bool IsFetchAppInProgress(string ikey) /// /// Instrumentation Key is expected to be a Guid string. /// 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. + /// To protect against injection attacks, AppId will be truncated to a maximum length. 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; diff --git a/Src/Common/HeadersUtilities.cs b/Src/Common/HeadersUtilities.cs index 93c595516..c14c3350f 100644 --- a/Src/Common/HeadersUtilities.cs +++ b/Src/Common/HeadersUtilities.cs @@ -25,7 +25,8 @@ public static string GetHeaderKeyValue(IEnumerable 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 GetHeaderDictionary(IEnumerable + /// 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. + /// + public static class InjectionGuardConstants + { + /// + /// Max length of AppId allowed in response from Breeze. + /// + public const int AppIdMaxLengeth = 50; + + /// + /// Max length of incoming Request Header value allowed. + /// + public const int RequestHeaderMaxLength = 1024; + } +} diff --git a/Src/Common/StringUtilities.cs b/Src/Common/StringUtilities.cs new file mode 100644 index 000000000..e7f685b0b --- /dev/null +++ b/Src/Common/StringUtilities.cs @@ -0,0 +1,26 @@ +namespace Microsoft.ApplicationInsights.Common +{ + using System.Diagnostics; + + /// + /// Generic functions to perform common operations on a string. + /// + public static class StringUtilities + { + /// + /// Check a strings length and trim to a max length if needed. + /// + 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; + } + } +} diff --git a/Src/DependencyCollector/Shared.Tests/CorrelationIdLookupHelperTests.cs b/Src/DependencyCollector/Shared.Tests/CorrelationIdLookupHelperTests.cs index cc5bc4a53..ca9ef845f 100644 --- a/Src/DependencyCollector/Shared.Tests/CorrelationIdLookupHelperTests.cs +++ b/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(); - 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)); } + + /// + /// Test that if an malicious value is returned, that value will be truncated. + /// + [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); + } } } diff --git a/Src/Web/Web.Net45/Implementation/HttpRequestExtensions.cs b/Src/Web/Web.Net45/Implementation/HttpRequestExtensions.cs index a9bfea93c..4839f0438 100644 --- a/Src/Web/Web.Net45/Implementation/HttpRequestExtensions.cs +++ b/Src/Web/Web.Net45/Implementation/HttpRequestExtensions.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Specialized; using System.Web; + using Microsoft.ApplicationInsights.Common; /// /// 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) diff --git a/Src/Web/Web.Shared.Net/RequestTrackingTelemetryModule.cs b/Src/Web/Web.Shared.Net/RequestTrackingTelemetryModule.cs index a2c2adbe7..3612ba8d3 100644 --- a/Src/Web/Web.Shared.Net/RequestTrackingTelemetryModule.cs +++ b/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))