diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index fe091f84e9c..861e4e7ca81 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -311,7 +311,7 @@ module CodeExecution { * Often, it is worthy of an alert if an SQL statement is constructed such that * executing it would be a security risk. * - * If it is important that the SQL statement is indeed executed, then use `SQLExecution`. + * If it is important that the SQL statement is indeed executed, then use `SqlExecution`. * * Extend this class to refine existing API models. If you want to model new APIs, * extend `SqlConstruction::Range` instead. @@ -329,7 +329,7 @@ module SqlConstruction { * Often, it is worthy of an alert if an SQL statement is constructed such that * executing it would be a security risk. * - * If it is important that the SQL statement is indeed executed, then use `SQLExecution`. + * If it is important that the SQL statement is indeed executed, then use `SqlExecution`. * * Extend this class to model new APIs. If you want to refine existing API models, * extend `SqlConstruction` instead. diff --git a/ruby/ql/lib/change-notes/2022-11-10-arel-sql.md b/ruby/ql/lib/change-notes/2022-11-10-arel-sql.md new file mode 100644 index 00000000000..e803d0e0895 --- /dev/null +++ b/ruby/ql/lib/change-notes/2022-11-10-arel-sql.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- +* The `codeql.ruby.Concepts` library now has a `SqlConstruction` class, in addition to the existing `SqlExecution` class. +* Calls to `Arel.sql` are now modeled as instances of the new `SqlConstruction` concept. diff --git a/ruby/ql/lib/codeql/ruby/Concepts.qll b/ruby/ql/lib/codeql/ruby/Concepts.qll index 2d70cc31796..54eee4e3e0a 100644 --- a/ruby/ql/lib/codeql/ruby/Concepts.qll +++ b/ruby/ql/lib/codeql/ruby/Concepts.qll @@ -11,9 +11,47 @@ private import codeql.ruby.Frameworks private import codeql.ruby.dataflow.RemoteFlowSources private import codeql.ruby.ApiGraphs +/** + * A data-flow node that constructs a SQL statement. + * + * Often, it is worthy of an alert if a SQL statement is constructed such that + * executing it would be a security risk. + * + * If it is important that the SQL statement is executed, use `SqlExecution`. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `SqlConstruction::Range` instead. + */ +class SqlConstruction extends DataFlow::Node instanceof SqlConstruction::Range { + /** Gets the argument that specifies the SQL statements to be constructed. */ + DataFlow::Node getSql() { result = super.getSql() } +} + +/** Provides a class for modeling new SQL execution APIs. */ +module SqlConstruction { + /** + * A data-flow node that constructs a SQL statement. + * + * Often, it is worthy of an alert if a SQL statement is constructed such that + * executing it would be a security risk. + * + * If it is important that the SQL statement is executed, use `SqlExecution`. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `SqlConstruction` instead. + */ + abstract class Range extends DataFlow::Node { + /** Gets the argument that specifies the SQL statements to be constructed. */ + abstract DataFlow::Node getSql(); + } +} + /** * A data-flow node that executes SQL statements. * + * If the context of interest is such that merely constructing a SQL statement + * would be valuable to report, consider using `SqlConstruction`. + * * Extend this class to refine existing API models. If you want to model new APIs, * extend `SqlExecution::Range` instead. */ @@ -27,6 +65,9 @@ module SqlExecution { /** * A data-flow node that executes SQL statements. * + * If the context of interest is such that merely constructing a SQL + * statement would be valuable to report, consider using `SqlConstruction`. + * * Extend this class to model new APIs. If you want to refine existing API models, * extend `SqlExecution` instead. */ diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Arel.qll b/ruby/ql/lib/codeql/ruby/frameworks/Arel.qll index 9fa17f4e5a5..f57fa41c740 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/Arel.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/Arel.qll @@ -6,6 +6,7 @@ private import codeql.ruby.ApiGraphs private import codeql.ruby.dataflow.FlowSummary +private import codeql.ruby.Concepts /** * Provides modeling for Arel, a low level SQL library that powers ActiveRecord. @@ -28,4 +29,14 @@ module Arel { input = "Argument[0]" and output = "ReturnValue" and preservesValue = false } } + + /** A call to `Arel.sql`, considered as a SQL construction. */ + private class ArelSqlConstruction extends SqlConstruction::Range, DataFlow::CallNode { + ArelSqlConstruction() { + this = DataFlow::getConstant("Arel").getAMethodCall() and + this.getMethodName() = "sql" + } + + override DataFlow::Node getSql() { result = this.getArgument(0) } + } } diff --git a/ruby/ql/lib/codeql/ruby/security/SqlInjectionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/SqlInjectionCustomizations.qll new file mode 100644 index 00000000000..66d3b0d4afd --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/SqlInjectionCustomizations.qll @@ -0,0 +1,55 @@ +/** + * Provides default sources, sinks and sanitizers for detecting SQL injection + * vulnerabilities, as well as extension points for adding your own. + */ + +private import codeql.ruby.Concepts +private import codeql.ruby.DataFlow +private import codeql.ruby.dataflow.BarrierGuards +private import codeql.ruby.dataflow.RemoteFlowSources + +/** + * Provides default sources, sinks and sanitizers for detecting SQL injection + * vulnerabilities, as well as extension points for adding your own. + */ +module SqlInjection { + /** A data flow source for SQL injection vulnerabilities. */ + abstract class Source extends DataFlow::Node { } + + /** A data flow sink for SQL injection vulnerabilities. */ + abstract class Sink extends DataFlow::Node { } + + /** A sanitizer for SQL injection vulnerabilities. */ + abstract class Sanitizer extends DataFlow::Node { } + + /** + * A source of remote user input, considered as a flow source. + */ + private class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { } + + /** + * An SQL statement of a SQL execution, considered as a flow sink. + */ + private class SqlExecutionAsSink extends Sink { + SqlExecutionAsSink() { this = any(SqlExecution e).getSql() } + } + + /** + * An SQL statement of a SQL construction, considered as a flow sink. + */ + private class SqlConstructionAsSink extends Sink { + SqlConstructionAsSink() { this = any(SqlConstruction e).getSql() } + } + + /** + * A comparison with a constant string, considered as a sanitizer-guard. + */ + private class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { } + + /** + * An inclusion check against an array of constant strings, considered as a + * sanitizer-guard. + */ + class StringConstArrayInclusionCallAsSanitizer extends Sanitizer, + StringConstArrayInclusionCallBarrier { } +} diff --git a/ruby/ql/lib/codeql/ruby/security/SqlInjectionQuery.qll b/ruby/ql/lib/codeql/ruby/security/SqlInjectionQuery.qll new file mode 100644 index 00000000000..f74e919ffe5 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/SqlInjectionQuery.qll @@ -0,0 +1,21 @@ +/** + * Provides default sources, sinks and sanitizers for detecting SQL injection + * vulnerabilities, as well as extension points for adding your own. + */ + +private import codeql.ruby.DataFlow +private import codeql.ruby.TaintTracking +import SqlInjectionCustomizations::SqlInjection + +/** + * A taint-tracking configuration for detecting SQL injection vulnerabilities. + */ +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "SqlInjectionConfiguration" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node source) { source instanceof Sink } + + override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer } +} diff --git a/ruby/ql/src/change-notes/2022-11-10-arel-sql.md b/ruby/ql/src/change-notes/2022-11-10-arel-sql.md new file mode 100644 index 00000000000..918e46a9d9b --- /dev/null +++ b/ruby/ql/src/change-notes/2022-11-10-arel-sql.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `rb/sql-injection` query now considers consider SQL constructions, such as calls to `Arel.sql`, as sinks. diff --git a/ruby/ql/src/queries/security/cwe-089/SqlInjection.ql b/ruby/ql/src/queries/security/cwe-089/SqlInjection.ql index fa7bc13402c..28be96deb7d 100644 --- a/ruby/ql/src/queries/security/cwe-089/SqlInjection.ql +++ b/ruby/ql/src/queries/security/cwe-089/SqlInjection.ql @@ -11,28 +11,11 @@ * external/cwe/cwe-089 */ -import codeql.ruby.AST -import codeql.ruby.Concepts import codeql.ruby.DataFlow -import codeql.ruby.dataflow.BarrierGuards -import codeql.ruby.dataflow.RemoteFlowSources -import codeql.ruby.TaintTracking +import codeql.ruby.security.SqlInjectionQuery import DataFlow::PathGraph -class SqlInjectionConfiguration extends TaintTracking::Configuration { - SqlInjectionConfiguration() { this = "SQLInjectionConfiguration" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof SqlExecution } - - override predicate isSanitizer(DataFlow::Node node) { - node instanceof StringConstCompareBarrier or - node instanceof StringConstArrayInclusionCallBarrier - } -} - -from SqlInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink +from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink where config.hasFlowPath(source, sink) select sink.getNode(), source, sink, "This SQL query depends on a $@.", source.getNode(), "user-provided value" diff --git a/ruby/ql/test/library-tests/frameworks/arel/SqlConstruction.expected b/ruby/ql/test/library-tests/frameworks/arel/SqlConstruction.expected new file mode 100644 index 00000000000..b25b2b387ca --- /dev/null +++ b/ruby/ql/test/library-tests/frameworks/arel/SqlConstruction.expected @@ -0,0 +1,2 @@ +| arel.rb:3:8:3:18 | call to sql | arel.rb:3:17:3:17 | x | +| arel.rb:8:8:8:18 | call to sql | arel.rb:8:17:8:17 | x | diff --git a/ruby/ql/test/library-tests/frameworks/arel/SqlConstruction.ql b/ruby/ql/test/library-tests/frameworks/arel/SqlConstruction.ql new file mode 100644 index 00000000000..2b2920918e5 --- /dev/null +++ b/ruby/ql/test/library-tests/frameworks/arel/SqlConstruction.ql @@ -0,0 +1,4 @@ +import codeql.ruby.Concepts +import codeql.ruby.DataFlow + +query predicate sqlConstructions(SqlConstruction c, DataFlow::Node sql) { sql = c.getSql() } diff --git a/ruby/ql/test/query-tests/security/cwe-089/ArelInjection.rb b/ruby/ql/test/query-tests/security/cwe-089/ArelInjection.rb new file mode 100644 index 00000000000..a1efb3adabb --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-089/ArelInjection.rb @@ -0,0 +1,8 @@ + +class PotatoController < ActionController::Base + def unsafe_action + name = params[:user_name] + # BAD: SQL statement constructed from user input + sql = Arel.sql("SELECT * FROM users WHERE name = #{name}") + end +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected b/ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected index d78b2675f25..d664bad4293 100644 --- a/ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected @@ -33,6 +33,8 @@ edges | ActiveRecordInjection.rb:137:21:137:44 | ...[...] : | ActiveRecordInjection.rb:20:22:20:30 | condition : | | ActiveRecordInjection.rb:151:59:151:64 | call to params : | ActiveRecordInjection.rb:151:59:151:74 | ...[...] : | | ActiveRecordInjection.rb:151:59:151:74 | ...[...] : | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | +| ArelInjection.rb:4:12:4:17 | call to params : | ArelInjection.rb:4:12:4:29 | ...[...] : | +| ArelInjection.rb:4:12:4:29 | ...[...] : | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | nodes | ActiveRecordInjection.rb:8:25:8:28 | name : | semmle.label | name : | | ActiveRecordInjection.rb:8:31:8:34 | pass : | semmle.label | pass : | @@ -85,6 +87,9 @@ nodes | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | semmle.label | "this is an unsafe annotation:..." | | ActiveRecordInjection.rb:151:59:151:64 | call to params : | semmle.label | call to params : | | ActiveRecordInjection.rb:151:59:151:74 | ...[...] : | semmle.label | ...[...] : | +| ArelInjection.rb:4:12:4:17 | call to params : | semmle.label | call to params : | +| ArelInjection.rb:4:12:4:29 | ...[...] : | semmle.label | ...[...] : | +| ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | semmle.label | "SELECT * FROM users WHERE nam..." | subpaths #select | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:70:23:70:28 | call to params : | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on a $@. | ActiveRecordInjection.rb:70:23:70:28 | call to params | user-provided value | @@ -105,3 +110,4 @@ subpaths | ActiveRecordInjection.rb:92:21:92:35 | ...[...] | ActiveRecordInjection.rb:92:21:92:26 | call to params : | ActiveRecordInjection.rb:92:21:92:35 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:92:21:92:26 | call to params | user-provided value | | ActiveRecordInjection.rb:104:20:104:32 | ... + ... | ActiveRecordInjection.rb:98:10:98:15 | call to params : | ActiveRecordInjection.rb:104:20:104:32 | ... + ... | This SQL query depends on a $@. | ActiveRecordInjection.rb:98:10:98:15 | call to params | user-provided value | | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | ActiveRecordInjection.rb:151:59:151:64 | call to params : | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | This SQL query depends on a $@. | ActiveRecordInjection.rb:151:59:151:64 | call to params | user-provided value | +| ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | ArelInjection.rb:4:12:4:17 | call to params : | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | This SQL query depends on a $@. | ArelInjection.rb:4:12:4:17 | call to params | user-provided value |