Merged PR 20748: [6.0] MSRC 69432 - ASP.NET Core - Denial of service in ASP.NET Core via FormPipeReader

# [6.0] MSRC 69432 - ASP.NET Core - Denial of service in ASP.NET Core via FormPipeReader

Fixes a bug in FormPipeReader where data without a delimiter will be buffered indefinitely, beyond configured limits.

## Description

When chunked data without a delimiter is sent to FormPipeReader, FormPipeReader will read the entire stream of data, starting from the beginning each time, without honoring configured length limits. This is because, after each read, it checks if `SequenceReader.Consumed` is greater than the configured limit, but `SequenceReader.Consumed` is 0 when no delimiter was found. Therefore the check against the length limit is never honored, and we continue to read data indefinitely, starting from the beginning of the stream each time.

## Customer Impact

Potential Denial-Of-Service attack on services using FormPipeReader

## Regression?

- [ ] Yes
- [ x ] No

[If yes, specify the version the behavior has regressed from]

## Risk

- [ ] High
- [ x ] Medium
- [ ] Low

The fix is a one-liner, and tests confirm a significant positive improvement on perf. There could be orthogonal issues that we've missed

## Verification

- [ x ] Manual (required)
- [ x ] Automated

## Packaging changes reviewed?

- [ x ] Yes
- [ ] No
- [ ] N/A

----

## When servicing release/2.1

- [ ] Make necessary changes in eng/PatchConfig.props

FormPipeReader

Author: wtgodbe

src/Http/WebUtilities/src/FormPipeReader.cs

@@ -5,6 +5,7 @@
 using System.Buffers;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Globalization;
 using System.IO;
 using System.IO.Pipelines;
 using System.Runtime.CompilerServices;
@@ -255,7 +256,8 @@ private void ParseValuesSlow(
                     if (!isFinalBlock)
                     {
                         // Don't buffer indefinitely
-                        if ((uint)(sequenceReader.Consumed - consumedBytes) > (uint)KeyLengthLimit + (uint)ValueLengthLimit)
+                        // +2 to account for '&' and '='
+                        if ((sequenceReader.Length - consumedBytes) > (long)KeyLengthLimit + (long)ValueLengthLimit + 2) 
                         {
                             ThrowKeyOrValueTooLargeException();
                         }
@@ -319,17 +321,30 @@ private void ParseValuesSlow(
 
         private void ThrowKeyOrValueTooLargeException()
         {
-            throw new InvalidDataException($"Form key length limit {KeyLengthLimit} or value length limit {ValueLengthLimit} exceeded.");
+            throw new InvalidDataException(
+                string.Format(
+                    CultureInfo.CurrentCulture,
+                    Resources.FormPipeReader_KeyOrValueTooLarge,
+                    KeyLengthLimit,
+                    ValueLengthLimit));
         }
 
         private void ThrowKeyTooLargeException()
         {
-            throw new InvalidDataException($"Form key length limit {KeyLengthLimit} exceeded.");
+            throw new InvalidDataException(
+                string.Format(
+                    CultureInfo.CurrentCulture,
+                    Resources.FormPipeReader_KeyTooLarge,
+                    KeyLengthLimit));
         }
 
         private void ThrowValueTooLargeException()
         {
-            throw new InvalidDataException($"Form value length limit {ValueLengthLimit} exceeded.");
+            throw new InvalidDataException(
+                string.Format(
+                    CultureInfo.CurrentCulture,
+                    Resources.FormPipeReader_ValueTooLarge,
+                    ValueLengthLimit));
         }
 
         [SkipLocalsInit]

src/Http/WebUtilities/src/Resources.resx

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!--
-    Microsoft ResX Schema
-
+  <!-- 
+    Microsoft ResX Schema 
+    
     Version 2.0
-
-    The primary goals of this format is to allow a simple XML format
-    that is mostly human readable. The generation and parsing of the
-    various data types are done through the TypeConverter classes
+    
+    The primary goals of this format is to allow a simple XML format 
+    that is mostly human readable. The generation and parsing of the 
+    various data types are done through the TypeConverter classes 
     associated with the data types.
-
+    
     Example:
-
+    
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-
-    There are any number of "resheader" rows that contain simple
+                
+    There are any number of "resheader" rows that contain simple 
     name/value pairs.
-
-    Each data row contains a name, and value. The row also contains a
-    type or mimetype. Type corresponds to a .NET class that support
-    text/value conversion through the TypeConverter architecture.
-    Classes that don't support this are serialized and stored with the
+    
+    Each data row contains a name, and value. The row also contains a 
+    type or mimetype. Type corresponds to a .NET class that support 
+    text/value conversion through the TypeConverter architecture. 
+    Classes that don't support this are serialized and stored with the 
     mimetype set.
-
-    The mimetype is used for serialized objects, and tells the
-    ResXResourceReader how to depersist the object. This is currently not
+    
+    The mimetype is used for serialized objects, and tells the 
+    ResXResourceReader how to depersist the object. This is currently not 
     extensible. For a given mimetype the value must be set accordingly:
-
-    Note - application/x-microsoft.net.object.binary.base64 is the format
-    that the ResXResourceWriter will generate, however the reader can
+    
+    Note - application/x-microsoft.net.object.binary.base64 is the format 
+    that the ResXResourceWriter will generate, however the reader can 
     read any of the formats listed below.
-
+    
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-
+    
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array
+    value   : The object must be serialized into a byte array 
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -117,6 +117,15 @@
   <resheader name="writer">
     <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
   </resheader>
+  <data name="FormPipeReader_KeyOrValueTooLarge" xml:space="preserve">
+    <value>Form key length limit {0} or value length limit {1} exceeded.</value>
+  </data>
+  <data name="FormPipeReader_KeyTooLarge" xml:space="preserve">
+    <value>Form key length limit {0} exceeded.</value>
+  </data>
+  <data name="FormPipeReader_ValueTooLarge" xml:space="preserve">
+    <value>Form value length limit {0} exceeded.</value>
+  </data>
   <data name="HttpRequestStreamReader_StreamNotReadable" xml:space="preserve">
     <value>The stream must support reading.</value>
   </data>

src/Http/WebUtilities/test/FormPipeReaderTests.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Buffers;
 using System.Collections.Generic;
+using System.Globalization;
 using System.IO;
 using System.IO.Pipelines;
 using System.Text;
@@ -215,6 +216,34 @@ public async Task ReadFormAsync_ValueLengthLimitExceededAcrossBufferBoundary_Thr
             Assert.Equal(Encoding.UTF8.GetBytes("baz=12345678901"), readResult.Buffer.ToArray());
         }
 
+        [Fact]
+        public void ReadFormAsync_ChunkedDataNoDelimiter_ThrowsEarly()
+        {
+            byte[] bytes = CreateBytes_NoDelimiter((10 * 1024) + 2);
+            var readOnlySequence = ReadOnlySequenceFactory.SegmentPerByteFactory.CreateWithContent(bytes);
+
+            KeyValueAccumulator accumulator = default;
+
+            var valueLengthLimit = 1024;
+            var keyLengthLimit = 10;
+
+            var formReader = new FormPipeReader(null!)
+            {
+                ValueLengthLimit = valueLengthLimit,
+                KeyLengthLimit = keyLengthLimit
+            };
+
+            var exception = Assert.Throws<InvalidDataException>(
+                () => formReader.ParseFormValues(ref readOnlySequence, ref accumulator, isFinalBlock: false));
+            // Make sure that FormPipeReader throws an exception after hitting KeyLengthLimit + ValueLengthLimit,
+            // Rather than after reading the entire request.
+            Assert.Equal(string.Format(
+                CultureInfo.CurrentCulture,
+                Resources.FormPipeReader_KeyOrValueTooLarge,
+                keyLengthLimit,
+                valueLengthLimit), exception.Message);
+        }
+
         // https://en.wikipedia.org/wiki/Percent-encoding
         [Theory]
         [InlineData("++=hello", "  ", "hello")]
@@ -606,5 +635,17 @@ private static async Task<PipeReader> MakePipeReader(string text)
             bodyPipe.Writer.Complete();
             return bodyPipe.Reader;
         }
+
+        private static byte[] CreateBytes_NoDelimiter(int n)
+        {
+            //Create the bytes of "key=vvvvvvvv....", of length n
+            var keyValue = new char[n];
+            Array.Fill(keyValue, 'v');
+            keyValue[0] = 'k';
+            keyValue[1] = 'e';
+            keyValue[2] = 'y';
+            keyValue[3] = '=';
+            return Encoding.UTF8.GetBytes(keyValue);
+        }
     }
 }