hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-29 02:35:15 +02:00
Code Issues Packages Projects Releases Wiki Activity

Make ActiveRecord model flag more potentially dangerous SQL executions

Browse Source
This commit is contained in:
Alex Ford
2021-06-23 16:08:26 +01:00
parent 8761873cd1
commit 6e5665da8c
2 changed files with 34 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
ql/src/codeql_ruby/frameworks/ActiveRecord.qll
View File

@@ -66,6 +66,31 @@ private predicate methodWithSqlFragmentArg(string methodName, int argIndex) {
methodName = "calculate" and argIndex = 1
}
// An expression that, if tainted by unsanitized input, should not be used as
// part of an argument to an SQL executing method
private predicate basicUnsafeSqlArg(Expr sqlFragmentExpr) {
// Literals containing an interpolated value
exists(StringInterpolationComponent interpolated |
interpolated = sqlFragmentExpr.(StringlikeLiteral).getComponent(_)
)
or
// String concatenations
sqlFragmentExpr instanceof AddExpr
or
// Variable reads
sqlFragmentExpr instanceof VariableReadAccess
or
// Method call
sqlFragmentExpr instanceof MethodCall
}
// An expression that, if used as an argument to an SQL executing method,
// may be unsafe if tainted by unsanitized input
private predicate unsafeSqlArg(Expr sqlFragmentExpr) {
basicUnsafeSqlArg(sqlFragmentExpr) or
basicUnsafeSqlArg(sqlFragmentExpr.(ArrayLiteral).getElement(0))
}
/**
* A method call that may result in executing unintended user-controlled SQL
* queries if the `getSqlFragmentSinkArgument()` expression is tainted by
@@ -89,22 +114,13 @@ class PotentiallyUnsafeSqlExecutingMethodCall extends ActiveRecordModelClassMeth
private Expr sqlFragmentExpr;
// TODO: `find` with `lock:` option also takes an SQL fragment
// TODO: refine this further to account for cases where the method called has
// been overriden to perform validation on its arguments
PotentiallyUnsafeSqlExecutingMethodCall() {
methodName = this.getMethodName() and
sqlFragmentExpr = this.getArgument(sqlFragmentArgumentIndex) and
methodWithSqlFragmentArg(methodName, sqlFragmentArgumentIndex) and
(
// select only literals containing an interpolated value...
exists(StringInterpolationComponent interpolated |
interpolated = sqlFragmentExpr.(StringlikeLiteral).getComponent(_)
)
or
// ...or string concatenations...
sqlFragmentExpr instanceof AddExpr
or
// ...or variable reads
sqlFragmentExpr instanceof VariableReadAccess
)
unsafeSqlArg(sqlFragmentExpr)
}
Expr getSqlFragmentSinkArgument() { result = sqlFragmentExpr }
@@ -124,3 +140,5 @@ class ActiveRecordSqlExecutionRange extends SqlExecution::Range {
override DataFlow::Node getSql() { result = this }
}
// TODO: model `ActiveRecord` sanitizers
// https://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html

4
ql/test/library-tests/frameworks/ActiveRecord.expected
View File

@@ -3,7 +3,9 @@ activeRecordModelClasses
| ActiveRecordInjection.rb:5:1:7:3 | User |
| ActiveRecordInjection.rb:9:1:10:3 | Admin |
activeRecordSqlExecutionRanges
| ActiveRecordInjection.rb:19:30:19:44 | ...[...] |
| ActiveRecordInjection.rb:22:21:22:41 | "id = #{...}" |
| ActiveRecordInjection.rb:25:22:25:44 | [...] |
| ActiveRecordInjection.rb:28:16:28:21 | <<-SQL |
| ActiveRecordInjection.rb:32:35:32:60 | "user.id = #{...}" |
| ActiveRecordInjection.rb:45:21:45:33 | ... + ... |
@@ -16,7 +18,9 @@ activeRecordModelClassMethodCalls
| ActiveRecordInjection.rb:32:5:32:61 | call to where |
| ActiveRecordInjection.rb:45:5:45:34 | call to delete_all |
potentiallyUnsafeSqlExecutingMethodCall
| ActiveRecordInjection.rb:19:5:19:45 | call to calculate |
| ActiveRecordInjection.rb:22:5:22:42 | call to delete_all |
| ActiveRecordInjection.rb:25:5:25:45 | call to destroy_all |
| ActiveRecordInjection.rb:28:5:28:35 | call to where |
| ActiveRecordInjection.rb:32:5:32:61 | call to where |
| ActiveRecordInjection.rb:45:5:45:34 | call to delete_all |