From d714f386957418f85fc25a19c53316d8005823c6 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 20 Jan 2021 08:26:13 -0800 Subject: [PATCH 1/2] Default to & as separator instead of either & or ; Allowing ; as separator by default can lead to web cache poisoning. Fixes #1732 --- CHANGELOG.md | 4 ++++ lib/rack/query_parser.rb | 9 ++++----- lib/rack/request.rb | 2 +- test/spec_request.rb | 6 +++--- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7ccdf964..df1b3ac05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. For info on ## [3.0.0] - Unreleased +### Security + +- Do not use semicolon as GET parameter separator. ([#1733](https://github.com/rack/rack/pull/1733), [@jeremyevans](https://github.com/jeremyevans)) + ### Added - `Rack::RewindableInput` supports size. ([@ahorek](https://github.com/ahorek)) diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb index dbbb18e5a..388e083c7 100644 --- a/lib/rack/query_parser.rb +++ b/lib/rack/query_parser.rb @@ -4,7 +4,7 @@ module Rack class QueryParser (require_relative 'core_ext/regexp'; using ::Rack::RegexpExtensions) if RUBY_VERSION < '2.4' - DEFAULT_SEP = /[&;] */n + DEFAULT_SEP = /[&] */n COMMON_SEP = { ";" => /[;] */n, ";," => /[;,] */n, "&" => /[&] */n } # ParameterTypeError is the error that is raised when incoming structural @@ -29,10 +29,9 @@ def initialize(params_class, key_space_limit, param_depth_limit) end # Stolen from Mongrel, with some small modifications: - # Parses a query string by breaking it up at the '&' - # and ';' characters. You can also use this to parse - # cookies by changing the characters used in the second - # 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) unescaper ||= method(:unescape) diff --git a/lib/rack/request.rb b/lib/rack/request.rb index 74377ede3..541c3801b 100644 --- a/lib/rack/request.rb +++ b/lib/rack/request.rb @@ -427,7 +427,7 @@ def GET if get_header(RACK_REQUEST_QUERY_STRING) == query_string get_header(RACK_REQUEST_QUERY_HASH) else - query_hash = parse_query(query_string, '&;') + query_hash = parse_query(query_string, '&') set_header(RACK_REQUEST_QUERY_STRING, query_string) set_header(RACK_REQUEST_QUERY_HASH, query_hash) end diff --git a/test/spec_request.rb b/test/spec_request.rb index 6b73cd5b5..6bc77b01a 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -308,12 +308,12 @@ def initialize(*) req.params[:quux].must_equal "bla" end - it "use semi-colons as separators for query strings in GET" do + it "does not use semi-colons as separators for query strings in GET" do req = make_request(Rack::MockRequest.env_for("/?foo=bar&quux=b;la;wun=duh")) req.query_string.must_equal "foo=bar&quux=b;la;wun=duh" - req.GET.must_equal "foo" => "bar", "quux" => "b", "la" => nil, "wun" => "duh" + req.GET.must_equal "foo" => "bar", "quux" => "b;la;wun=duh" req.POST.must_be :empty? - req.params.must_equal "foo" => "bar", "quux" => "b", "la" => nil, "wun" => "duh" + req.params.must_equal "foo" => "bar", "quux" => "b;la;wun=duh" end it "limit the keys from the GET query string" do From 24bf471950f249c058082f8ff951951fb332ef83 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Thu, 21 Jan 2021 08:15:31 -0800 Subject: [PATCH 2/2] Use more descriptive argument name --- lib/rack/query_parser.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb index 388e083c7..cea6b9b09 100644 --- a/lib/rack/query_parser.rb +++ b/lib/rack/query_parser.rb @@ -32,12 +32,12 @@ def initialize(params_class, key_space_limit, param_depth_limit) # 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) + def parse_query(qs, separator = nil, &unescaper) unescaper ||= method(:unescape) params = make_params - (qs || '').split(d ? (COMMON_SEP[d] || /[#{d}] */n) : DEFAULT_SEP).each do |p| + (qs || '').split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |p| next if p.empty? k, v = p.split('=', 2).map!(&unescaper) @@ -60,11 +60,11 @@ def parse_query(qs, d = nil, &unescaper) # query strings with parameters of conflicting types, in this case a # ParameterTypeError is raised. Users are encouraged to return a 400 in this # case. - def parse_nested_query(qs, d = nil) + def parse_nested_query(qs, separator = nil) params = make_params unless qs.nil? || qs.empty? - (qs || '').split(d ? (COMMON_SEP[d] || /[#{d}] */n) : DEFAULT_SEP).each do |p| + (qs || '').split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |p| k, v = p.split('=', 2).map! { |s| unescape(s) } normalize_params(params, k, v, param_depth_limit)