Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.1] Remove cookie name decoding #24264

Merged
merged 1 commit into from
Aug 13, 2020
Merged

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Jul 23, 2020

Description

Decoding arbitrary input for cookie names allows spoofing of special inputs like __Host- with encoded values like __%48ost-.

Fix: Only compare to known values. e.g. the key the developer passes in with the encoding we would have used.

This fix works seamlessly for the indexer and TryGetValue, but leaves the encoded value in enumerator.

Customer Impact

Security

Regression?

No, reserved name prefixes are a newer browser cookie feature.

Risk (see taxonomy)

Low. It's not clear than many customers were dependent on the encoding/decoding behavior for cookie names. There is a mitigation in place that works for most scenarios, and a compat switch to opt out of the change.

Link the PR to the original issue and/or the PR to master.

#23578
#23579
#24389
aspnet/AspNetKatana#368.

Packaging impact? (if a libraries change)

  • Microsoft.AspNetCore.Http

@Tratcher Tratcher added this to the 2.1.x milestone Jul 23, 2020
@Tratcher Tratcher requested review from blowdart and HaoK July 23, 2020 23:32
@Tratcher Tratcher self-assigned this Jul 23, 2020
@ghost ghost added the area-servers label Jul 23, 2020
@ghost ghost added this to PR Open in Servicing Status Jul 23, 2020
@Tratcher Tratcher force-pushed the tratcher/release/2.1/cookiename branch 2 times, most recently from ba33657 to a37d632 Compare July 24, 2020 19:29
@Pilchie Pilchie added the Servicing-consider Shiproom approval is required for the issue label Aug 11, 2020
@ghost
Copy link

ghost commented Aug 11, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@ghost ghost moved this from PR Open to Submitted to Shiproom in Servicing Status Aug 11, 2020
@leecow leecow removed the Servicing-consider Shiproom approval is required for the issue label Aug 11, 2020
@leecow leecow modified the milestones: 2.1.x, 2.1.22 Aug 11, 2020
@Tratcher Tratcher force-pushed the tratcher/release/2.1/cookiename branch from 38e7995 to 1eaaf15 Compare August 12, 2020 23:56
@Tratcher
Copy link
Member Author

@dougbu can this be merged, or do you want to wait for #24375?

@dougbu
Copy link
Member

dougbu commented Aug 13, 2020

Only thing blocking a merge is shiproom (Tactics) approval.

@Tratcher
Copy link
Member Author

It was already approved at the same time as #24389. I think @leecow forgot the new label?

@dougbu dougbu merged commit 3e29d0b into release/2.1 Aug 13, 2020
@dougbu dougbu deleted the tratcher/release/2.1/cookiename branch August 13, 2020 17:12
dougbu added a commit that referenced this pull request Jan 5, 2021
- **Do Not Merge**
- undo an important part of #24264
- this is an attempt to demonstrate #29023
  - should show test indirect references let most tests keep working
dougbu added a commit that referenced this pull request Jan 19, 2021
- **Do Not Merge**
- always fail `RequestCookieCollection.ContainsKey(...)` and `RequestCookieCollection.TryGetValue(...)`
- undo an important part of #24264 and demonstrate #29023 problem
  - shows many tests working despite break of an indirect reference
dougbu added a commit that referenced this pull request Jan 26, 2021
- **Do Not Merge**
- always fail `RequestCookieCollection.ContainsKey(...)` and `RequestCookieCollection.TryGetValue(...)`
- undo an important part of #24264 and demonstrate #29023 problem
  - shows many tests working despite break of an indirect reference
dougbu added a commit that referenced this pull request Jan 27, 2021
- **Do Not Merge**
- always fail `RequestCookieCollection.ContainsKey(...)` and `RequestCookieCollection.TryGetValue(...)`
- undo an important part of #24264 and demonstrate #29023 problem
  - shows many tests working despite break of an indirect reference
@Pilchie Pilchie added the Servicing-approved Shiproom has approved the issue label Aug 30, 2021
@ghost ghost moved this from Submitted to Shiproom to Approved by Shiproom in Servicing Status Aug 30, 2021
@Pilchie Pilchie moved this from Approved by Shiproom to Merged in Servicing Status Aug 30, 2021
jackbatzner added a commit to jackbatzner/azure-functions-openapi-extension that referenced this pull request Oct 10, 2022
…vulneratbility.

There is a vulnerability in Microsoft.AspNetcore.Http in 2.1.0 that we need to upgrade

References:
- GHSA-hxrm-9w7p-39cc
- dotnet/aspnetcore#24264
@jeran-urban
Copy link

jeran-urban commented Nov 22, 2022

Hello,

Can anyone verify that GHSA-hxrm-9w7p-39cc was permanently fixed for Mictosoft.AspNetCore.Http as of 2.1.22, and the higher versions, including versions 2.2.x no longer have this vulnerability? Sonatype is reporting this is still an active issue directly via support chat:

"""
For Microsoft.AspNetCore.Http:

So for the 2.1.x branch, we do have the vulnerable range closed off at 2.1.22 (not inclusive).

For the 2.2.x version, the advisory does not address this branch and we have found that it does have the vulnerable code in its versions. There are currently 5 2.2.x versions published to Nuget, the latest published on 2/12/2019, and all contain the vulnerable code. We are monitoring new releases of this component and will close off the vulnerable range for the 2.2.x branch should a fix ever be released for it.
"""

If you can verify, can you please provide documentation so I can try to get this updated? Thank you!

@ghost
Copy link

ghost commented Nov 22, 2022

Hi @jeran-urban. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented Nov 22, 2022

@jeran-urban,

For the 2.2.x version, …

2.2.x was last patched on November 19, 2019 and reached end of life on December 23, 2019. Both dates were long before this PR was merged. Even 2.1.x is no longer supported except if using a subset of the packages on .NET Framework.

Please see https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core#lifecycle for more information.

@jeran-urban
Copy link

@jeran-urban,

For the 2.2.x version, …

2.2.x was last patched on November 19, 2019 and reached end of life on December 23, 2019. Both dates were long before this PR was merged. Even 2.1.x is no longer supported except if using a subset of the packages on .NET Framework.

Please see https://dotnet.microsoft.com/en-us/platform/support/policy/dotnet-core#lifecycle for more information.

Sorry, to clarify, I am asking in regards to the Microsoft.AspNetCore.Http Nuget Package, version 2.2.x (in this specific case, 2.2.2), not the .net core parent framework. It is my understanding the Microsoft.AspNetCore.Http Nuget package is still in use and working against .net Core 5 and 6

@ghost
Copy link

ghost commented Nov 23, 2022

Hi @jeran-urban. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented Nov 23, 2022

All Microsoft.AspNetCore.* v2.2.x packages ended support at the same time as the larger framework. From 3.1.0 on, Microsoft.AspNetCore.Http.dll is in the shared framework; no separate package needed.

@jeran-urban
Copy link

jeran-urban commented Nov 23, 2022

All Microsoft.AspNetCore.* v2.2.x packages ended support at the same time as the larger framework. From 3.1.0 on, Microsoft.AspNetCore.Http.dll is in the shared framework; no separate package needed.

thank you so much for the explanation. And sorry to seek further clarification:
2. for .net core >= 3.1.0 there are some external nuget packages that still depend on older versions of those same nuget packages that have been included into the shared runtime framework. Is there a way to prevent VS from pulling in those nuget packages as external resources, and instead have it reference the ones built in to the framework? Or is that just dependent on the specified nuget package and its transitive dependencies?

Thank you again!

@ghost
Copy link

ghost commented Nov 23, 2022

Hi @jeran-urban. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Servicing-approved Shiproom has approved the issue
Projects
No open projects
Servicing Status
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants