From 9b2cd768e13da49d3dbd3726b02d06451ea09f94 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 21 Jun 2023 16:32:44 +0100 Subject: [PATCH 1/4] Ruby: rack - add env['QUERY_STRING'] as an http request input --- .../ruby/frameworks/rack/internal/App.qll | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll b/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll index 748291f55bd..166f656e136 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll @@ -4,6 +4,7 @@ private import codeql.ruby.AST private import codeql.ruby.ApiGraphs +private import codeql.ruby.Concepts private import codeql.ruby.DataFlow private import codeql.ruby.typetracking.TypeTracker private import Response::Private as RP @@ -86,4 +87,22 @@ module App { /** Gets a response returned from this request handler. */ RP::PotentialResponseNode getAResponse() { result = resp } } + + /** A read of the query string via `env['QUERY_STRING']`. */ + private class EnvQueryStringRead extends Http::Server::RequestInputAccess::Range { + EnvQueryStringRead() { + exists(RequestHandler handler, DataFlow::ParameterNode env, ConstantValue key | + handler.getEnv() = env + | + this = env.getAnElementRead(key) and + key.isStringlikeValue("QUERY_STRING") + ) + } + + override string getSourceType() { result = "Rack env" } + + override Http::Server::RequestInputKind getKind() { + result = Http::Server::parameterInputKind() + } + } } From ec2c9f20f6694c55cd43491f13f9ecc4a081fb6b Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 5 Jul 2023 15:21:04 +0100 Subject: [PATCH 2/4] Ruby: rack - env['QUERY_STRING'] changenote --- ruby/ql/lib/change-notes/2023-07-05-rack-env-query-params.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ruby/ql/lib/change-notes/2023-07-05-rack-env-query-params.md diff --git a/ruby/ql/lib/change-notes/2023-07-05-rack-env-query-params.md b/ruby/ql/lib/change-notes/2023-07-05-rack-env-query-params.md new file mode 100644 index 00000000000..807db340146 --- /dev/null +++ b/ruby/ql/lib/change-notes/2023-07-05-rack-env-query-params.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `'QUERY_STRING'` field of a Rack `env` parameter is now recognized as a source of remote user input. From 08784d24b4c110ff557a3b5c282a5b00315aa75c Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 5 Jul 2023 15:46:31 +0100 Subject: [PATCH 3/4] Ruby: rack - add tests for env['QUERY_STRING'] --- .../test/library-tests/frameworks/rack/Rack.expected | 7 +++++-- ruby/ql/test/library-tests/frameworks/rack/Rack.ql | 3 +++ ruby/ql/test/library-tests/frameworks/rack/rack.rb | 12 ++++++++++-- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/ruby/ql/test/library-tests/frameworks/rack/Rack.expected b/ruby/ql/test/library-tests/frameworks/rack/Rack.expected index 01892a08dec..e99b12aa9b4 100644 --- a/ruby/ql/test/library-tests/frameworks/rack/Rack.expected +++ b/ruby/ql/test/library-tests/frameworks/rack/Rack.expected @@ -3,9 +3,10 @@ rackRequestHandlers | rack.rb:17:3:21:5 | call | rack.rb:17:12:17:18 | the_env | rack.rb:20:5:20:27 | call to [] | | rack.rb:30:3:36:5 | call | rack.rb:30:12:30:14 | env | rack.rb:35:5:35:26 | call to [] | | rack.rb:40:3:44:5 | call | rack.rb:40:12:40:14 | env | rack.rb:43:5:43:45 | call to [] | -| rack.rb:60:3:62:5 | call | rack.rb:60:12:60:14 | env | rack.rb:66:7:66:22 | call to [] | -| rack.rb:60:3:62:5 | call | rack.rb:60:12:60:14 | env | rack.rb:73:5:73:21 | call to [] | +| rack.rb:60:3:62:5 | call | rack.rb:60:12:60:14 | env | rack.rb:66:7:66:24 | call to [] | +| rack.rb:60:3:62:5 | call | rack.rb:60:12:60:14 | env | rack.rb:73:5:73:23 | call to [] | | rack.rb:79:3:81:5 | call | rack.rb:79:17:79:19 | env | rack.rb:93:5:93:78 | call to finish | +| rack.rb:98:3:102:5 | call | rack.rb:98:12:98:14 | env | rack.rb:101:5:101:42 | call to [] | | rack_apps.rb:6:3:12:5 | call | rack_apps.rb:6:12:6:14 | env | rack_apps.rb:10:12:10:34 | call to [] | | rack_apps.rb:16:3:18:5 | call | rack_apps.rb:16:17:16:19 | env | rack_apps.rb:17:5:17:28 | call to [] | | rack_apps.rb:21:14:21:50 | -> { ... } | rack_apps.rb:21:17:21:19 | env | rack_apps.rb:21:24:21:48 | call to [] | @@ -16,3 +17,5 @@ rackResponseContentTypes redirectResponses | rack.rb:43:5:43:45 | call to [] | rack.rb:42:30:42:40 | "/foo.html" | | rack.rb:93:5:93:78 | call to finish | rack.rb:93:60:93:70 | redirect_to | +requestInputAccesses +| rack.rb:99:14:99:32 | ...[...] | diff --git a/ruby/ql/test/library-tests/frameworks/rack/Rack.ql b/ruby/ql/test/library-tests/frameworks/rack/Rack.ql index 29b73b327fb..6e2efb590e8 100644 --- a/ruby/ql/test/library-tests/frameworks/rack/Rack.ql +++ b/ruby/ql/test/library-tests/frameworks/rack/Rack.ql @@ -1,4 +1,5 @@ private import codeql.ruby.AST +private import codeql.ruby.Concepts private import codeql.ruby.frameworks.Rack private import codeql.ruby.DataFlow @@ -17,3 +18,5 @@ query predicate rackResponseContentTypes( query predicate redirectResponses(Rack::Response::RedirectResponse resp, DataFlow::Node location) { location = resp.getRedirectLocation() } + +query predicate requestInputAccesses(Http::Server::RequestInputAccess ria) { any() } diff --git a/ruby/ql/test/library-tests/frameworks/rack/rack.rb b/ruby/ql/test/library-tests/frameworks/rack/rack.rb index 109016f018d..1eaa0decc48 100644 --- a/ruby/ql/test/library-tests/frameworks/rack/rack.rb +++ b/ruby/ql/test/library-tests/frameworks/rack/rack.rb @@ -63,14 +63,14 @@ class Baz def run(env) if env[:foo] == "foo" - [200, {}, "foo"] + [200, {}, ["foo"]] else error end end def error - [400, {}, "nope"] + [400, {}, ["nope"]] end end @@ -93,3 +93,11 @@ class Qux Rack::Response.new(['redirecting'], 302, 'Location' => redirect_to).finish end end + +class UsesEnvQueryParams + def call(env) + params = env['QUERY_STRING'] + user = Rack::Utils.parse_query(params)["user"] + [200, {}, [lookup_user_profile(user)]] + end +end From 06aefe01b8f9b0a3b7b3ffbfdb05660d732c3370 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Mon, 17 Jul 2023 14:08:44 +0100 Subject: [PATCH 4/4] Update ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll Co-authored-by: Asger F --- .../lib/codeql/ruby/frameworks/rack/internal/App.qll | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll b/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll index 166f656e136..58f7792ffee 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll @@ -91,12 +91,10 @@ module App { /** A read of the query string via `env['QUERY_STRING']`. */ private class EnvQueryStringRead extends Http::Server::RequestInputAccess::Range { EnvQueryStringRead() { - exists(RequestHandler handler, DataFlow::ParameterNode env, ConstantValue key | - handler.getEnv() = env - | - this = env.getAnElementRead(key) and - key.isStringlikeValue("QUERY_STRING") - ) + this = + any(RequestHandler h) + .getEnv() + .getAnElementRead(ConstantValue::fromStringlikeValue("QUERY_STRING")) } override string getSourceType() { result = "Rack env" }