From 880fb2b14acdab002a00afb8615c61d5fee53b17 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 5 Oct 2022 11:59:40 +0100 Subject: [PATCH] Ruby: split out rb/sensitive-get-query using query/customizations pattern --- .../SensitiveGetQueryCustomizations.qll | 53 +++++++++++++++++++ .../ruby/security/SensitiveGetQueryQuery.qll | 32 +++++++++++ .../security/cwe-598/SensitiveGetQuery.ql | 38 +++---------- .../cwe-598/SensitiveGetQuery.expected | 11 ++++ 4 files changed, 103 insertions(+), 31 deletions(-) create mode 100644 ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryCustomizations.qll create mode 100644 ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryQuery.qll diff --git a/ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryCustomizations.qll new file mode 100644 index 00000000000..7da99aa3713 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryCustomizations.qll @@ -0,0 +1,53 @@ +/** + * Provides default sources and sinks for reasoning about sensitive data sourced + * from the query string of a GET request, as well as extension points for + * adding your own. + */ + +private import codeql.ruby.security.SensitiveActions +private import codeql.ruby.Concepts +private import codeql.ruby.DataFlow + +/** + * Provides default sources and sinks for reasoning about sensitive data sourced + * from the query string of a GET request, as well as extension points for + * adding your own. + */ +module SensitiveGetQuery { + /** + * A data flow source representing data sourced from the query string in a + * GET request handler. + */ + abstract class Source extends DataFlow::Node { + /** Gets the request handler corresponding to this data source. */ + abstract Http::Server::RequestHandler getHandler(); + } + + /** + * An access to data from the query string of a GET request as a data flow + * source. + */ + private class RequestInputAccessSource extends Source instanceof Http::Server::RequestInputAccess { + private Http::Server::RequestHandler handler; + + RequestInputAccessSource() { + handler = this.asExpr().getExpr().getEnclosingMethod() and + handler.getAnHttpMethod() = "get" + } + + override Http::Server::RequestHandler getHandler() { result = handler } + } + + /** + * A data flow sink suggesting a use of sensitive data. + */ + abstract class Sink extends DataFlow::Node { } + + /** A sensitive data node as a data flow sink. */ + private class SensitiveNodeSink extends Sink instanceof SensitiveNode { + SensitiveNodeSink() { + // User names and other similar information is not sensitive in this context. + not this.getClassification() = SensitiveDataClassification::id() + } + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryQuery.qll b/ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryQuery.qll new file mode 100644 index 00000000000..67d110445ae --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryQuery.qll @@ -0,0 +1,32 @@ +/** + * Provides a taint-tracking configuration for detecting flow of query string + * data to sensitive actions in GET query request handlers. + * + * Note, for performance reasons: only import this file if `Configuration` is + * needed, otherwise `SensitiveGetQueryCustomizations` should be imported + * instead. + */ + +private import ruby +private import codeql.ruby.DataFlow +private import codeql.ruby.TaintTracking + +/** + * Provides a taint-tracking configuration for detecting flow of query string + * data to sensitive actions in GET query request handlers. + */ +module SensitiveGetQuery { + import SensitiveGetQueryCustomizations::SensitiveGetQuery + + /** + * A taint-tracking configuration for reasoning about use of sensitive data + * from a GET request query string. + */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "SensitiveGetQuery" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + } +} diff --git a/ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql b/ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql index 8a9d1b810bc..6a7cb5a248f 100644 --- a/ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql +++ b/ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql @@ -12,37 +12,13 @@ */ import ruby -private import codeql.ruby.DataFlow -private import codeql.ruby.TaintTracking -private import codeql.ruby.security.SensitiveActions -private import codeql.ruby.Concepts -private import codeql.ruby.frameworks.ActionDispatch -private import codeql.ruby.frameworks.ActionController -private import codeql.ruby.frameworks.core.Array +import DataFlow::PathGraph +import codeql.ruby.DataFlow +import codeql.ruby.security.SensitiveGetQueryQuery +import codeql.ruby.security.SensitiveActions -class Source extends Http::Server::RequestInputAccess { - private Http::Server::RequestHandler handler; - - Source() { - handler = this.asExpr().getExpr().getEnclosingMethod() and - handler.getAnHttpMethod() = "get" - } - - Http::Server::RequestHandler getHandler() { result = handler } -} - -class Configuration extends TaintTracking::Configuration { - Configuration() { this = "SensitiveGetQuery" } - - override predicate isSource(DataFlow::Node source) { source instanceof Source } - - override predicate isSink(DataFlow::Node sink) { sink instanceof SensitiveNode } -} - -from DataFlow::PathNode source, DataFlow::PathNode sink, Configuration config -where - config.hasFlowPath(source, sink) and - not sink.getNode().(SensitiveNode).getClassification() = SensitiveDataClassification::id() +from DataFlow::PathNode source, DataFlow::PathNode sink, SensitiveGetQuery::Configuration config +where config.hasFlowPath(source, sink) select source.getNode(), source, sink, "$@ for GET requests uses query parameter as sensitive data.", - source.getNode().(Source).getHandler(), "Route handler" + source.getNode().(SensitiveGetQuery::Source).getHandler(), "Route handler" diff --git a/ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.expected b/ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.expected index 2d8077f0cf5..a32e70e832b 100644 --- a/ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.expected +++ b/ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.expected @@ -1,2 +1,13 @@ +edges +| app/controllers/users_controller.rb:4:16:4:21 | call to params : | app/controllers/users_controller.rb:4:16:4:32 | ...[...] | +| app/controllers/users_controller.rb:4:16:4:21 | call to params : | app/controllers/users_controller.rb:4:16:4:32 | ...[...] : | +| app/controllers/users_controller.rb:4:16:4:32 | ...[...] : | app/controllers/users_controller.rb:5:42:5:49 | password | +nodes +| app/controllers/users_controller.rb:4:16:4:21 | call to params : | semmle.label | call to params : | +| app/controllers/users_controller.rb:4:16:4:32 | ...[...] | semmle.label | ...[...] | +| app/controllers/users_controller.rb:4:16:4:32 | ...[...] : | semmle.label | ...[...] : | +| app/controllers/users_controller.rb:5:42:5:49 | password | semmle.label | password | +subpaths +#select | app/controllers/users_controller.rb:4:16:4:21 | call to params | app/controllers/users_controller.rb:4:16:4:21 | call to params : | app/controllers/users_controller.rb:4:16:4:32 | ...[...] | $@ for GET requests uses query parameter as sensitive data. | app/controllers/users_controller.rb:3:3:6:5 | login_get | Route handler | | app/controllers/users_controller.rb:4:16:4:21 | call to params | app/controllers/users_controller.rb:4:16:4:21 | call to params : | app/controllers/users_controller.rb:5:42:5:49 | password | $@ for GET requests uses query parameter as sensitive data. | app/controllers/users_controller.rb:3:3:6:5 | login_get | Route handler |