From c1e5640a656ddfe6d478cb54a30002de41c25180 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 21 Apr 2016 14:17:38 -0700 Subject: [PATCH 1/2] Don't allow response headers to contain control characters - For the purposes of this commit, control characters are everything < 0x20. - Immediately throw an InvalidOperationException when the header is added to the IHttpResponseFeature.Headers dictionary. --- .../Http/FrameHeaders.Generated.cs | 8 ++++ .../Http/FrameHeaders.cs | 37 +++++++++++++------ .../KnownHeaders.cs | 4 ++ 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameHeaders.Generated.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameHeaders.Generated.cs index 3d9ccbaf3c..bf47376156 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameHeaders.Generated.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameHeaders.Generated.cs @@ -2038,6 +2038,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } protected override void SetValueFast(string key, StringValues value) { + switch (key.Length) { case 13: @@ -2424,10 +2425,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } break; } + Unknown[key] = value; } protected override void AddValueFast(string key, StringValues value) { + switch (key.Length) { case 13: @@ -2990,6 +2993,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } break; } + Unknown.Add(key, value); } protected override bool RemoveFast(string key) @@ -7181,6 +7185,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } protected override void SetValueFast(string key, StringValues value) { + ValidateHeaderCharacters(value); switch (key.Length) { case 13: @@ -7512,10 +7517,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } break; } + ValidateHeaderCharacters(key); Unknown[key] = value; } protected override void AddValueFast(string key, StringValues value) { + ValidateHeaderCharacters(value); switch (key.Length) { case 13: @@ -7991,6 +7998,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http } break; } + ValidateHeaderCharacters(key); Unknown.Add(key, value); } protected override bool RemoveFast(string key) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameHeaders.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameHeaders.cs index 596b973575..4bba23dbd3 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameHeaders.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameHeaders.cs @@ -39,21 +39,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http { get { + // Unlike the IHeaderDictionary version, this getter will throw a KeyNotFoundException. return GetValueFast(key); } set { - if (_isReadOnly) - { - ThrowReadOnlyException(); - } - SetValueFast(key, value); + ((IHeaderDictionary)this)[key] = value; } } protected void ThrowReadOnlyException() { - throw new InvalidOperationException("Headers are readonly, reponse has already started."); + throw new InvalidOperationException("Headers are read-only, response has already started."); } protected void ThrowArgumentException() @@ -140,11 +137,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http void ICollection>.Add(KeyValuePair item) { - if (_isReadOnly) - { - ThrowReadOnlyException(); - } - AddValueFast(item.Key, item.Value); + ((IDictionary)this).Add(item.Key, item.Value); } void IDictionary.Add(string key, StringValues value) @@ -216,5 +209,27 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http { return TryGetValueFast(key, out value); } + + public static void ValidateHeaderCharacters(StringValues headerValues) + { + foreach (var value in headerValues) + { + ValidateHeaderCharacters(value); + } + } + + public static void ValidateHeaderCharacters(string headerCharacters) + { + if (headerCharacters != null) + { + foreach (var ch in headerCharacters) + { + if (ch < 0x20) + { + throw new InvalidOperationException(string.Format("Invalid control character in header: 0x{0:X2}", (byte)ch)); + } + } + } + } } } diff --git a/tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs b/tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs index 9f47fdfae0..a51039d540 100644 --- a/tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs +++ b/tools/Microsoft.AspNetCore.Server.Kestrel.GeneratedCode/KnownHeaders.cs @@ -299,6 +299,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http }} protected override void SetValueFast(string key, StringValues value) {{ + {(loop.ClassName == "FrameResponseHeaders" ? "ValidateHeaderCharacters(value);" : "")} switch (key.Length) {{{Each(loop.HeadersByLength, byLength => $@" case {byLength.Key}: @@ -313,10 +314,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http ")}}} break; ")}}} + {(loop.ClassName == "FrameResponseHeaders" ? "ValidateHeaderCharacters(key);" : "")} Unknown[key] = value; }} protected override void AddValueFast(string key, StringValues value) {{ + {(loop.ClassName == "FrameResponseHeaders" ? "ValidateHeaderCharacters(value);" : "")} switch (key.Length) {{{Each(loop.HeadersByLength, byLength => $@" case {byLength.Key}: @@ -335,6 +338,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Http ")}}} break; ")}}} + {(loop.ClassName == "FrameResponseHeaders" ? "ValidateHeaderCharacters(key);" : "")} Unknown.Add(key, value); }} protected override bool RemoveFast(string key) From faf81f11f540c5459e222d1fc49dee2209d03fe3 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 2 May 2016 16:55:41 -0700 Subject: [PATCH 2/2] Add response header validation tests --- .../FrameResponseHeadersTests.cs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameResponseHeadersTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameResponseHeadersTests.cs index 88ea59e93e..07be384d2c 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameResponseHeadersTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameResponseHeadersTests.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Server.Kestrel; using Microsoft.AspNetCore.Server.Kestrel.Http; using Microsoft.AspNetCore.Server.Kestrel.Infrastructure; @@ -69,5 +70,51 @@ namespace Microsoft.AspNetCore.Server.KestrelTests Assert.False(frame.ResponseHeaders.ContainsKey("Server")); Assert.False(frame.ResponseHeaders.ContainsKey("Date")); } + + [Theory] + [InlineData("Server", "\r\nData")] + [InlineData("Server", "\0Data")] + [InlineData("Server", "Data\r")] + [InlineData("Server", "Da\0ta")] + [InlineData("Server", "Da\u001Fta")] + [InlineData("Unknown-Header", "\r\nData")] + [InlineData("Unknown-Header", "\0Data")] + [InlineData("Unknown-Header", "Data\0")] + [InlineData("Unknown-Header", "Da\nta")] + [InlineData("\r\nServer", "Data")] + [InlineData("Server\r", "Data")] + [InlineData("Ser\0ver", "Data")] + [InlineData("Server\r\n", "Data")] + [InlineData("\u001FServer", "Data")] + [InlineData("Unknown-Header\r\n", "Data")] + [InlineData("\0Unknown-Header", "Data")] + [InlineData("Unknown\r-Header", "Data")] + [InlineData("Unk\nown-Header", "Data")] + public void AddingControlCharactersToHeadersThrows(string key, string value) + { + var responseHeaders = new FrameResponseHeaders(); + + Assert.Throws(() => { + ((IHeaderDictionary)responseHeaders)[key] = value; + }); + + Assert.Throws(() => { + ((IHeaderDictionary)responseHeaders)[key] = new StringValues(new[] { "valid", value }); + }); + + Assert.Throws(() => { + ((IDictionary)responseHeaders)[key] = value; + }); + + Assert.Throws(() => { + var kvp = new KeyValuePair(key, value); + ((ICollection>)responseHeaders).Add(kvp); + }); + + Assert.Throws(() => { + var kvp = new KeyValuePair(key, value); + ((IDictionary)responseHeaders).Add(key, value); + }); + } } }