Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

[Wip] Don't process infinite request lengths #313

Closed
wants to merge 2 commits into from
Closed

[Wip] Don't process infinite request lengths #313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8694a5b
Don't process infinite request lengths
benaadams Nov 1, 2015
9d55987
Read request max values from config
benaadams Nov 14, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/Microsoft.AspNet.Server.Kestrel/IKestrelServerInformation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ public interface IKestrelServerInformation

bool NoDelay { get; set; }

int MaxHeaderBytes { get; set; }

long MaxUploadBytes { get; set; }

IConnectionFilter ConnectionFilter { get; set; }
}
}
22 changes: 22 additions & 0 deletions src/Microsoft.AspNet.Server.Kestrel/Infrastructure/MemoryPoolIterator2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.IO;
using System.Linq;
using System.Numerics;
using System.Text;
Expand All @@ -26,16 +27,19 @@ public struct MemoryPoolIterator2

private MemoryPoolBlock2 _block;
private int _index;
private int _processed;

public MemoryPoolIterator2(MemoryPoolBlock2 block)
{
_block = block;
_index = _block?.Start ?? 0;
_processed = 0;
}
public MemoryPoolIterator2(MemoryPoolBlock2 block, int index)
{
_block = block;
_index = index;
_processed = 0;
}

public bool IsDefault => _block == null;
Expand Down Expand Up @@ -170,6 +174,11 @@ public int Seek(int char0)
}
while (block.End != index)
{
if (_processed > _maxHeaderLength)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The definition of this variable is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying to work out how to get it there :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions? Always pass it to constructor is what I'm looking at currently... Hoping there is a better way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halter73 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could put these limits directly on ServiceContext like we do for ConnectionFilter. It might be best to put the entire IKestrelServerInformation object in ServiceContext.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think that the limits need to be passed as arguments to functions like Seek. Just in case it is ever used for something other than headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return a ResultType { int count, bool limitReached } struct and add a limit into each function and the caller can decide what to do? (e.g. throw BadRequest)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Another option might be to throw directly and let the caller wrap the exception.

{
throw new InvalidDataException("Malformed request");
}

var following = block.End - index;
if (following >= vectorStride)
{
Expand All @@ -180,6 +189,7 @@ public int Seek(int char0)
if (ch0Count == 0)
{
index += vectorStride;
_processed += vectorStride;
continue;
}
else if (ch0Count == 1)
Expand Down Expand Up @@ -240,6 +250,11 @@ public int Seek(int char0, int char1)
}
while (block.End != index)
{
if (_processed > _maxHeaderLength)
{
throw new InvalidDataException("Malformed request");
}

var following = block.End - index;
if (following >= vectorStride)
{
Expand All @@ -252,6 +267,7 @@ public int Seek(int char0, int char1)
if (ch0Count == 0 && ch1Count == 0)
{
index += vectorStride;
_processed += vectorStride;
continue;
}
else if (ch0Count < 2 && ch1Count < 2)
Expand Down Expand Up @@ -332,6 +348,11 @@ public int Seek(int char0, int char1, int char2)
}
while (block.End != index)
{
if (_processed > _maxHeaderLength)
{
throw new InvalidDataException("Malformed request");
}

var following = block.End - index;
if (following >= vectorStride)
{
Expand All @@ -346,6 +367,7 @@ public int Seek(int char0, int char1, int char2)
if (ch0Count == 0 && ch1Count == 0 && ch2Count == 0)
{
index += vectorStride;
_processed += vectorStride;
continue;
}
else if (ch0Count < 2 && ch1Count < 2 && ch2Count < 2)
Expand Down
34 changes: 34 additions & 0 deletions src/Microsoft.AspNet.Server.Kestrel/KestrelServerInformation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ public class KestrelServerInformation : IKestrelServerInformation, IServerAddres

public bool NoDelay { get; set; }

public int MaxHeaderBytes { get; set; } = 16384; // 16kB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is out of sync with this PR. This property is read from config but the server never checks it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is incomplete; just making sure I'm reading correctly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your config approach looks reasonable.


public long MaxUploadBytes { get; set; } = 8388608; // 8MB

public IConnectionFilter ConnectionFilter { get; set; }

public void Initialize(IConfiguration configuration)
Expand All @@ -26,6 +30,36 @@ public void Initialize(IConfiguration configuration)
{
Addresses.Add(url);
}

var maxHeaderBytes = configuration["request.maxHeaderBytes"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request. => server.

if (!string.IsNullOrEmpty(maxHeaderBytes))
{
int value;
if (!int.TryParse(maxHeaderBytes, out value))
{
throw new ArgumentException("maxHeaderBytes must be an integer 1024 or greater", "request.maxHeaderBytes");
}
if (value < 1024)
{
throw new ArgumentOutOfRangeException("request.maxHeaderBytes", maxHeaderBytes, "maxHeaderBytes must be 1024 or greater");
}
MaxHeaderBytes = value;
}

var maxUploadBytes = configuration["request.maxUploadBytes"];
if (!string.IsNullOrEmpty(maxHeaderBytes))
{
long value;
if (!long.TryParse(maxHeaderBytes, out value))
{
throw new ArgumentException("maxUploadBytes must be an integer 0 or greater", "request.maxUploadBytes");
}
if (value < 0)
{
throw new ArgumentOutOfRangeException("request.maxUploadBytes", maxUploadBytes, "maxUploadBytes must be a positive integer");
}
MaxUploadBytes = value;
}
}
}
}