Skip to content

[release/7.0] Ensure free buffer space when reading TLS messages#83574

Merged
rzikm merged 4 commits into
release/7.0-stagingfrom
backport/pr-83480-to-release/7.0
Apr 3, 2023
Merged

[release/7.0] Ensure free buffer space when reading TLS messages#83574
rzikm merged 4 commits into
release/7.0-stagingfrom
backport/pr-83480-to-release/7.0

Conversation

@github-actions

@github-actions github-actions Bot commented Mar 17, 2023

Copy link
Copy Markdown
Contributor

Backport of #83480 to release/7.0

Fixes #83455.

/cc @rzikm

Customer Impact

7.0 Introduced regression where SslStream would fail to establish TLS connection (and, by extension, HTTPS) to some servers in very specific circumstances (depending on the sizing of TLS records exchanged during the handshake). One such server is lh3.googleusercontent.com.
For affected servers it happens all the time - i.e. .NET is unable to establish connection to them.

Testing

Manual test against the affected server lh3.googleusercontent.com.
CI test suite passes.

Risk

Low, we didn't change code path when the frame size is known. The code change affects only code path when the frame size is unknown, which happens only when the server sends TLS frames slightly shorter than 16K/8K/4K (see example of the google server above).

@ghost

ghost commented Mar 17, 2023

Copy link
Copy Markdown

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

Issue Details

Backport of #83480 to release/7.0

/cc @rzikm

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

@rzikm
rzikm requested a review from wfurt March 17, 2023 08:25
@rzikm rzikm added this to the 7.0.x milestone Mar 17, 2023
@rzikm
rzikm requested a review from karelz March 17, 2023 08:27

@wfurt wfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

}

if (frameSize < int.MaxValue)
if (frameSize != int.MaxValue)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is it needed? Looks like semantically it is the same. Only MaxValue can be "not <".

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not needed, it was part of stylistic change to make it evident that int.MaxValue is a special value ("unknown size"). I can remove it from the backport.

@karelz karelz added the Servicing-consider Issue for next servicing release review label Mar 28, 2023
@karelz

karelz commented Mar 28, 2023

Copy link
Copy Markdown
Member

Impact on customers hitting real servers (see top post), worth servicing.

@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.6 Mar 28, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 28, 2023
@carlossanlop

Copy link
Copy Markdown
Contributor

I'm retargeting this PR to the new release/7.0-staging branch, which is the one that we will use from now on for servicing fixes.

Repo maintainers will now be allowed to merge their own servicing PR as long as it meets the requirements:

  • It is approved by Tactics (signaled by adding the Servicing-approved label).
  • It's signed-off by an area owner.
  • The CI is green, or the failures are investigated as unrelated.
  • And if the PR touches an OOB package, the necessary OOB authoring changes are added.

The new process is described here: runtime/docs/project/library-servicing.md.

The infra team will be actively monitoring servicing PRs to ensure all requirements are met and to help with any issues.

Let me know if you have any questions.

@carlossanlop
carlossanlop changed the base branch from release/7.0 to release/7.0-staging March 28, 2023 21:00
@rzikm

rzikm commented Mar 29, 2023

Copy link
Copy Markdown
Member

Test failures are known problems - tracked in #83901

@rzikm

rzikm commented Mar 29, 2023

Copy link
Copy Markdown
Member

@carlossanlop The check-labels check seems to be stuck in the Expected -- Waiting for status to be reported state. Is there a way to restart it?

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-approved Approved for servicing release labels Mar 30, 2023
@rzikm

rzikm commented Apr 3, 2023

Copy link
Copy Markdown
Member

Test failures are #83901 (known and closed already)

@rzikm
rzikm merged commit 001172f into release/7.0-staging Apr 3, 2023
@jkotas
jkotas deleted the backport/pr-83480-to-release/7.0 branch April 18, 2023 22:46
@ghost ghost locked as resolved and limited conversation to collaborators May 19, 2023
@rbhanda rbhanda modified the milestones: 7.0.6, 7.0.7 Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants