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

Default to & as separator instead of either & or ; #1733

Merged

Conversation

jeremyevans
Copy link
Contributor

Allowing ; as separator by default can lead to web cache poisoning.

Fixes #1732

# parameter (which defaults to '&;').
# Parses a query string by breaking it up at the '&'. You can also use this
# to parse cookies by changing the characters used in the second parameter
# (which defaults to '&').
def parse_query(qs, d = nil, &unescaper)
Copy link
Collaborator

@rafaelfranca rafaelfranca Jan 20, 2021

Choose a reason for hiding this comment

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

Should we rename qs and d to something more descriptive? I would never think that d would mean separator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea, but as a separate commit.

@ioquatix
Copy link
Member

The next release should be a 3.0 release, so this is fine to merge.

Allowing ; as separator by default can lead to web cache poisoning.

Fixes rack#1732
@jeremyevans jeremyevans force-pushed the fix-web-cache-poisoning-semicolon-1732 branch from b5cbe43 to 24bf471 Compare January 21, 2021 16:16
@jeremyevans
Copy link
Contributor Author

Ubuntu CI is failing due to the inability to install packages, before it runs tests. All tests pass on Mac, and this change is probably platform independent, so I'll merge it.

@jeremyevans jeremyevans merged commit a05f8d5 into rack:master Jan 21, 2021
@hctan
Copy link

hctan commented Jun 11, 2021

Hi, any chances this fix can be rolled out for 2.x? I'm currently using 2.2.3. When is 3.0 expected to be released?

@ioquatix
Copy link
Member

Probably by the end of the year.

@blakegearin
Copy link

Any updates?

@jeremyevans
Copy link
Contributor Author

There is no estimated release date yet. We are nearing the end of making major feature changes, I think, but there are few more pull requests and issues we need to work through. Due to many changes in 3.0, we'll probably need to start with a prerelease version and allow for a substantial period for people to test before the final release. So it don't think we can provide a good estimate yet for the final release of 3.0.

@blakegearin
Copy link

It appears a05f8d5 was cherry-picked out of the release of 2.2.3.1 and 2.2.4? Please correct me if I'm wrong.

Was wondering why it would be excluded since this commit patches the CWE-444 vulnerability (Snyk report). So applications wanting to avoid the vulnerability will have to depend on the main branch instead of a version which isn't ideal.

@jeremyevans
Copy link
Contributor Author

It appears a05f8d5 was cherry-picked out of the release of 2.2.3.1 and 2.2.4? Please correct me if I'm wrong.

The commit was only made to the main branch, it was never applied to the 2-2-stable branch. So the phrase cherry-picked out certainly doesn't apply. The commit wasn't backported to 2.2. I think it is too backwards incompatible to be backported, and it looks like @ioquatix feels the same way. Maybe @tenderlove can give his opinion.

Was wondering why it would be excluded since this commit patches the CWE-444 vulnerability (Snyk report). So applications wanting to avoid the vulnerability will have to depend on the main branch instead of a version which isn't ideal.

Again, this wasn't excluded, it just wasn't backported due to backwards compatibility concerns.

@blakegearin
Copy link

blakegearin commented Jul 22, 2022

Understood. The reason I said "cherry-picked" was because I'm used to versions being different slices of main instead of the multiple stable branches you all have going. So since it was merged to main I expected any new release to include it. My bad!

Again, this wasn't excluded, it just wasn't backported due to backwards compatibility concerns.

I guess I consider any version with a known vulnerability to be functionally dead in the water and backwards compatibility not particularly relevant. But that's just my opinion and we can disagree. Backporting is easier said than done.

At the places I've worked when a vulnerability was found in a dependency a new version with a patch was typically released within 24-48 hours or at most a week. If not, the dependency was removed altogether because it's not an acceptable risk and pointing at main is a hard sell due to lack of confidence in future vulnerability fixes. If Rack doesn't aim for/cater to a lot of commercial usage though, I understand. Just wanted to share my experiences for context.

@robopolomsky
Copy link

robopolomsky commented Apr 25, 2023

Hi, could somebody please cherry-pick this issue into 2.2? It's a vulnerability issue known for 2 years https://security.snyk.io/vuln/SNYK-RUBY-RACK-1061917 and unfortunately ruby-rails still didn't include rack 3 and I don't know when it will happen.

(Update): I read #2043 and it seems that's not possible unfortunately :(.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web cache poisoning - semicolon as a separator
6 participants