From fa58c51810375b7d1d523f1deed2fa2cb357305f Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 5 Oct 2022 15:57:38 +0100 Subject: [PATCH] Ruby: switch rb/sensitive-get-query back to using local flow --- .../SensitiveGetQueryCustomizations.qll | 54 ------------------- .../ruby/security/SensitiveGetQueryQuery.qll | 31 ----------- .../security/cwe-598/SensitiveGetQuery.ql | 34 +++++++++--- .../cwe-598/SensitiveGetQuery.expected | 14 +---- 4 files changed, 27 insertions(+), 106 deletions(-) delete mode 100644 ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryCustomizations.qll delete 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 deleted file mode 100644 index a1a4277fe8f..00000000000 --- a/ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryCustomizations.qll +++ /dev/null @@ -1,54 +0,0 @@ -/** - * 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" and - this.getKind() = "parameter" - } - - 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 deleted file mode 100644 index 7c3531fe18c..00000000000 --- a/ruby/ql/lib/codeql/ruby/security/SensitiveGetQueryQuery.qll +++ /dev/null @@ -1,31 +0,0 @@ -/** - * 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.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 ba3faf3fcae..4fe1becaab3 100644 --- a/ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql +++ b/ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql @@ -2,7 +2,7 @@ * @name Sensitive data read from GET request * @description Placing sensitive data in a GET request increases the risk of * the data being exposed to an attacker. - * @kind path-problem + * @kind problem * @problem.severity warning * @security-severity 6.5 * @precision high @@ -12,12 +12,30 @@ */ import ruby -import DataFlow::PathGraph -import codeql.ruby.security.SensitiveGetQueryQuery +import codeql.ruby.Concepts import codeql.ruby.security.SensitiveActions -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().(SensitiveGetQuery::Source).getHandler(), "Route handler" +// Local flow augmented with flow through element references +private predicate localFlowWithElementReference(DataFlow::LocalSourceNode src, DataFlow::Node to) { + src.flowsTo(to) + or + exists(DataFlow::Node midRecv, DataFlow::LocalSourceNode mid, Ast::ElementReference ref | + src.flowsTo(midRecv) and + midRecv.asExpr().getExpr() = ref.getReceiver() and + mid.asExpr().getExpr() = ref + | + localFlowWithElementReference(mid, to) + ) +} + +from + Http::Server::RequestHandler handler, Http::Server::RequestInputAccess input, + SensitiveNode sensitive +where + handler.getAnHttpMethod() = "get" and + input.asExpr().getExpr().getEnclosingMethod() = handler and + input.getKind() = "parameter" and + localFlowWithElementReference(input, sensitive) and + not sensitive.getClassification() = SensitiveDataClassification::id() +select input, "$@ for GET requests uses query parameter as sensitive data.", handler, + "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 a32e70e832b..9563e1eefae 100644 --- a/ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.expected +++ b/ruby/ql/test/query-tests/security/cwe-598/SensitiveGetQuery.expected @@ -1,13 +1 @@ -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 | +| app/controllers/users_controller.rb:4:16:4:21 | call to params | $@ for GET requests uses query parameter as sensitive data. | app/controllers/users_controller.rb:3:3:6:5 | login_get | Route handler |