Rent correct buffer size for SslStream.Encrypt #51060

benaadams · 2021-04-11T02:15:44Z

WriteSingleChunk rents a buffer the size of buffer.Length + FrameOverhead; however after the handshake the overhead is _headerSize + _trailerSize which can be different and causes Encrypt to ignore the buffer and allocate its own of the correct size.

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs

Lines 253 to 257 in 125253b

    
           int bufferSizeNeeded = checked(input.Length + headerSize + trailerSize); 
        
           if (output == null || output.Length < bufferSizeNeeded) 
        
           { 
        
               output = new byte[bufferSizeNeeded]; 
        
           }

Change to rent buffer.Length + _headerSize + _trailerSize when these values are available.

Also drop some unnecessary casting in the Pal layers by receiving the correct type rather than a less derived one.

Before

After

/cc @karelz @dotnet/ncl

Resolves #51076

ghost · 2021-04-11T02:15:50Z

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Seen in #50921 (comment)

WriteSingleChunk rents a buffer the size of buffer.Length + FrameOverhead; however after the handshake the overhead is _headerSize + _trailerSize which causes Encrypt to ignore the buffer and allocate its own of the correct size.

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Windows.cs

Lines 253 to 257 in 125253b

    
           int bufferSizeNeeded = checked(input.Length + headerSize + trailerSize); 
        
           if (output == null || output.Length < bufferSizeNeeded) 
        
           { 
        
               output = new byte[bufferSizeNeeded]; 
        
           }

Change to rent buffer.Length + _headerSize + _trailerSize when these values are available.

Before

After

/cc @davidfowl @stephentoub

Author:	benaadams
Assignees:	-
Labels:	`area-System.Net.Security`
Milestone:	-

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

geoffkizer · 2021-04-11T04:38:58Z

We really shouldn't ever allocate in SslStreamPal.EncryptMessage; this is a legacy of previous times where we weren't very smart about buffer management. Now it's just hiding issues like this.

Could we change that check to an assert? Is there any code depending on the current behavior? Could it be fixed?

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

geoffkizer · 2021-04-11T04:56:00Z

Out of curiosity, what values for header size and trailer size were you seeing?

Presumably FrameOverhead was intended to be large enough to handle any cipher mode; but apparently it's not...

benaadams · 2021-04-11T11:54:31Z

Out of curiosity, what values for header size and trailer size were you seeing?

Outputting when header + trailer > FrameOverhead

headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (348), output.Length (512)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (86), output.Length (256)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (83), output.Length (256)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (348), output.Length (512)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (348), output.Length (512)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (86), output.Length (256)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (348), output.Length (512)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (348), output.Length (512)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (84), output.Length (256)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (86), output.Length (256)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (86), output.Length (256)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (348), output.Length (512)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (348), output.Length (512)
headerSize (5) + trailerSize (44) > FrameOverhead (32) : input.Length (30), output.Length (128)

I see the reallocation about 4% of the calls; but I assume a lot get hidden from ArrayPool<byte>.Shared.Rent's round up to a power of 2?

benaadams · 2021-04-11T12:06:54Z

Looks like the FrameOverhead of 32 is too small from TLS 1.2 onwards with 32 byte HMAC? (5 + 32 is 37, and may be padding)

5 byte header
N byte payload
HMAC (32 bytes for the SHA-256-based HMAC, 20 bytes for the SHA-1-based HMAC, 16 bytes for the MD5-based HMAC.)
N byte Padding if a block cipher is used

benaadams · 2021-04-11T13:01:40Z

Could we change that check to an assert? Is there any code depending on the current behavior? Could it be fixed?

Not sure; it can wait for renegotiation to finish and then call it with the original rented buffer; so the sizes could have changed at that point. However it should only be that rare corner case now?

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs

Lines 690 to 693 in 3cbbade

    
           // Wait for renegotiation to finish. 
        
           await waitTask.ConfigureAwait(false); 
        
           SecurityStatusPal status = EncryptData(buffer, ref outBuffer, out int encryptedBytes);

geoffkizer · 2021-04-15T15:40:56Z

it can wait for renegotiation to finish and then call it with the original rented buffer;

Is it possible to either (a) not rent the buffer until we know that we are not renegotiating; or (b) release the old rented buffer and rent a new one when renegotiation happens?

However it should only be that rare corner case now?

Yeah, renegotiation is not common and I don't really worry about the perf cost in that case. I'm more worried that if we continue to have the automatic fallback in the low-level Encrypt code, then it will hide perf problems in cases we care about, like the original one here.

geoffkizer · 2021-04-15T15:41:49Z

Looks like the FrameOverhead of 32 is too small from TLS 1.2 onwards

We should audit if there are other places we use FrameOverhead.

benaadams · 2021-04-15T16:11:59Z

it can wait for renegotiation to finish and then call it with the original rented buffer;

Is it possible to either (a) not rent the buffer until we know that we are not renegotiating; or (b) release the old rented buffer and rent a new one when renegotiation happens?

Already does (b); however there is a race condition with renegotiation which happens in EncryptData; and there are 3 different methods of working out the buffer size due to 3 different platform implementations.

I'm more worried that if we continue to have the automatic fallback in the low-level Encrypt code, then it will hide perf problems in cases we care about, like the original one here.

Separate issue to resolve across SChannel, OpenSsl and AppleCrypto as they all use different apis to determine size?

i.e. its currently always requesting a buffer that's too small; which is hidden 96% of the time due to ArrayPool's power of 2 round up, this resolves that and increases FrameOverhead to a larger value for newer TLS

I agree it would be better not to have the fallback; but changing it to throw an exception instead is a bit more responsibility that I want to take in this PR 😉

geoffkizer · 2021-04-15T16:25:45Z

I agree it would be better not to have the fallback; but changing it to throw an exception instead is a bit more responsibility that I want to take in this PR

I understand :)

That said, if we are not going to do a comprehensive fix right now, then we should probably do something with minimal risk and file a separate issue to address this in a comprehensive way. To that end, maybe we should just bump FrameOverhead up to 64.

Also adding @wfurt for his opinion.

benaadams · 2021-04-15T18:14:56Z

That said, if we are not going to do a comprehensive fix right now, then we should probably do something with minimal risk and file a separate issue to address this in a comprehensive way. To that end, maybe we should just bump FrameOverhead up to 64.

Something more like? #51320

benaadams · 2021-04-15T20:05:50Z

Opened PR for the casting #51324

benaadams · 2021-04-15T23:34:17Z

Will close this in favour of the other two

Rent correct buffer size in SslStream.Encrypt

d9a6588

dotnet-issue-labeler bot added the area-System.Net.Security label Apr 11, 2021

benaadams mentioned this pull request Apr 11, 2021

Use the new async state machine pooling feature on websockets #50921

Closed

davidfowl reviewed Apr 11, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

Feedback

fa6fded

geoffkizer reviewed Apr 11, 2021

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs Outdated Show resolved Hide resolved

benaadams added 2 commits April 11, 2021 13:51

Feedback

346b55c

Undo assert

7c3e827

benaadams requested a review from geoffkizer April 11, 2021 13:34

benaadams mentioned this pull request Apr 15, 2021

Increase SslStream FrameOverhead estimation for newer TLS protocols #51320

Merged

benaadams closed this Apr 15, 2021

benaadams deleted the Rent-correct-buffer-size-in-SslStream.Encrypt branch April 16, 2021 00:56

geoffkizer mentioned this pull request Apr 19, 2021

SslStream should do a better job of allocating buffer for encryption #51478

Closed

ghost locked as resolved and limited conversation to collaborators May 16, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rent correct buffer size for SslStream.Encrypt #51060

Rent correct buffer size for SslStream.Encrypt #51060

benaadams commented Apr 11, 2021 •

edited

Loading

ghost commented Apr 11, 2021

geoffkizer commented Apr 11, 2021

geoffkizer commented Apr 11, 2021

benaadams commented Apr 11, 2021

benaadams commented Apr 11, 2021 •

edited

Loading

benaadams commented Apr 11, 2021

geoffkizer commented Apr 15, 2021

geoffkizer commented Apr 15, 2021

benaadams commented Apr 15, 2021

geoffkizer commented Apr 15, 2021

benaadams commented Apr 15, 2021

benaadams commented Apr 15, 2021

benaadams commented Apr 15, 2021

	int bufferSizeNeeded = checked(input.Length + headerSize + trailerSize);
	if (output == null \|\| output.Length < bufferSizeNeeded)
	{
	output = new byte[bufferSizeNeeded];
	}

Rent correct buffer size for SslStream.Encrypt #51060

Rent correct buffer size for SslStream.Encrypt #51060

Conversation

benaadams commented Apr 11, 2021 • edited Loading

ghost commented Apr 11, 2021

geoffkizer commented Apr 11, 2021

geoffkizer commented Apr 11, 2021

benaadams commented Apr 11, 2021

benaadams commented Apr 11, 2021 • edited Loading

benaadams commented Apr 11, 2021

geoffkizer commented Apr 15, 2021

geoffkizer commented Apr 15, 2021

benaadams commented Apr 15, 2021

geoffkizer commented Apr 15, 2021

benaadams commented Apr 15, 2021

benaadams commented Apr 15, 2021

benaadams commented Apr 15, 2021

benaadams commented Apr 11, 2021 •

edited

Loading

benaadams commented Apr 11, 2021 •

edited

Loading