mirror of
https://github.com/github/codeql.git
synced 2026-04-25 00:35:20 +02:00
Implement rb/sql-injection
This commit is contained in:
Alex Ford
64
ql/src/queries/security/cwe-089/SqlInjection.qhelp
Normal file
64
ql/src/queries/security/cwe-089/SqlInjection.qhelp
Normal file
@@ -0,0 +1,64 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
<overview>
|
||||
<p>
|
||||
If a database query (such as a SQL or NoSQL query) is built from
|
||||
user-provided data without sufficient sanitization, a malicious user
|
||||
may be able to run malicious database queries.
|
||||
</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>
|
||||
Most database connector libraries offer a way of safely embedding
|
||||
untrusted data into a query by means of query parameters or
|
||||
prepared statements.
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>
|
||||
In the following Rails example, an <code>ActionController</code> class
|
||||
has a <code>text_bio</code> method to handle requests to fetch a biography
|
||||
for a specified user.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
The user is specified using a parameter, <code>user_name</code> provided by
|
||||
the client. This value is accessible using the <code>params</code> method.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
The method illustrates three different ways to construct and execute an SQL
|
||||
query to find the user by name.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
In the first case, the parameter <code>user_name</code> is inserted into an
|
||||
SQL fragment using string interpolation. The parameter is user-supplied and
|
||||
is not sanitized. An attacker could use this to construct SQL queries that
|
||||
were not intended to be executed here.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
The second case uses string concatenation and is vulnerable in the same way
|
||||
that the first case is.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
In the third case, the name is passed in a hash instead.
|
||||
<code>ActiveRecord</code> will construct a parameterized SQL query that is not
|
||||
vulnerable to SQL injection attacks.
|
||||
</p>
|
||||
|
||||
<sample src="examples/SqlInjection.rb" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/SQL_injection">SQL injection</a>.</li>
|
||||
<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html">SQL Injection Prevention Cheat Sheet</a>.</li>
|
||||
</references>
|
||||
</qhelp>
|
||||
38
ql/src/queries/security/cwe-089/SqlInjection.ql
Normal file
38
ql/src/queries/security/cwe-089/SqlInjection.ql
Normal file
@@ -0,0 +1,38 @@
|
||||
/**
|
||||
* @name SQL query built from user-controlled sources
|
||||
* @description Building a SQL query from user-controlled sources is vulnerable to insertion of
|
||||
* malicious SQL code by the user.
|
||||
* @kind path-problem
|
||||
* @problem.severity error
|
||||
* @precision high
|
||||
* @id rb/sql-injection
|
||||
* @tags security
|
||||
* external/cwe/cwe-089
|
||||
* external/owasp/owasp-a1
|
||||
*/
|
||||
|
||||
import ruby
|
||||
import codeql_ruby.Concepts
|
||||
import codeql_ruby.DataFlow
|
||||
import codeql_ruby.dataflow.BarrierGuards
|
||||
import codeql_ruby.dataflow.RemoteFlowSources
|
||||
import codeql_ruby.TaintTracking
|
||||
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 = any(SqlExecution e) }
|
||||
|
||||
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
|
||||
guard instanceof StringConstCompare or
|
||||
guard instanceof StringConstArrayInclusionCall
|
||||
}
|
||||
}
|
||||
|
||||
from SQLInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
|
||||
where config.hasFlowPath(source, sink)
|
||||
select sink.getNode(), source, sink, "This SQL query depends on $@.", source.getNode(),
|
||||
"a user-provided value"
|
||||
15
ql/src/queries/security/cwe-089/examples/SqlInjection.rb
Normal file
15
ql/src/queries/security/cwe-089/examples/SqlInjection.rb
Normal file
@@ -0,0 +1,15 @@
|
||||
class UserController < ActionController::Base
|
||||
def text_bio
|
||||
# BAD -- Using string interpolation
|
||||
user = User.find_by "name = '#{params[:user_name]}'"
|
||||
|
||||
# BAD -- Using string concatenation
|
||||
find_str = "name = '" + params[:user_name] + "'"
|
||||
user = User.find_by(find_str)
|
||||
|
||||
# GOOD -- Using a hash to parameterize arguments
|
||||
user = User.find_by name: params[:user_name]
|
||||
|
||||
render plain: user&.text_bio
|
||||
end
|
||||
end
|
||||
@@ -0,0 +1,64 @@
|
||||
class UserGroup < ActiveRecord::Base
|
||||
has_many :users
|
||||
end
|
||||
|
||||
class User < ApplicationRecord
|
||||
belongs_to :user_group
|
||||
end
|
||||
|
||||
class Admin < User
|
||||
end
|
||||
|
||||
class FooController < ActionController::Base
|
||||
|
||||
MAX_USER_ID = 100_000
|
||||
|
||||
# A string tainted by user input is inserted into an SQL query
|
||||
def some_request_handler
|
||||
# SELECT AVG(#{params[:column]}) FROM "users"
|
||||
User.calculate(:average, params[:column])
|
||||
|
||||
# DELETE FROM "users" WHERE (id = #{params[:id]})
|
||||
User.delete_all("id = #{params[:id]}")
|
||||
|
||||
# SELECT "users".* FROM "users" WHERE (id = #{params[:id]})
|
||||
User.destroy_all(["id = #{params[:id]}"])
|
||||
|
||||
# SELECT "users".* FROM "users" WHERE id BETWEEN #{params[:min_id]} AND 100000
|
||||
User.where(<<-SQL, MAX_USER_ID)
|
||||
id BETWEEN #{params[:min_id]} AND ?
|
||||
SQL
|
||||
|
||||
UserGroup.joins(:users).where("user.id = #{params[:id]}")
|
||||
end
|
||||
end
|
||||
|
||||
|
||||
class BarController < ApplicationController
|
||||
|
||||
def some_other_request_handler
|
||||
ps = params
|
||||
|
||||
uid = ps[:id]
|
||||
|
||||
# DELETE FROM "users" WHERE (id = #{uid})
|
||||
User.delete_all("id = " + uid)
|
||||
end
|
||||
|
||||
def sanitized_paths
|
||||
|
||||
dir = params[:order]
|
||||
# barrier guard prevents taint flow
|
||||
dir = "DESC" unless dir == "ASC"
|
||||
User.order("name #{dir}")
|
||||
|
||||
name = params[:user_name]
|
||||
# barrier guard prevents taint flow
|
||||
if %w(alice bob charlie).include? name
|
||||
User.find_by("username = #{name}")
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
class BazController < BarController
|
||||
end
|
||||
23
ql/test/query-tests/security/cwe-089/SqlInjection.expected
Normal file
23
ql/test/query-tests/security/cwe-089/SqlInjection.expected
Normal file
@@ -0,0 +1,23 @@
|
||||
edges
|
||||
| ActiveRecordInjection.rb:19:30:19:35 | call to params : | ActiveRecordInjection.rb:19:30:19:44 | ...[...] |
|
||||
| ActiveRecordInjection.rb:22:29:22:34 | call to params : | ActiveRecordInjection.rb:22:21:22:41 | "id = #{...}" |
|
||||
| ActiveRecordInjection.rb:29:20:29:25 | call to params : | ActiveRecordInjection.rb:28:16:28:21 | <<-SQL |
|
||||
| ActiveRecordInjection.rb:32:48:32:53 | call to params : | ActiveRecordInjection.rb:32:35:32:60 | "user.id = #{...}" |
|
||||
| ActiveRecordInjection.rb:40:10:40:15 | call to params : | ActiveRecordInjection.rb:45:21:45:33 | ... + ... |
|
||||
nodes
|
||||
| ActiveRecordInjection.rb:19:30:19:35 | call to params : | semmle.label | call to params : |
|
||||
| ActiveRecordInjection.rb:19:30:19:44 | ...[...] | semmle.label | ...[...] |
|
||||
| ActiveRecordInjection.rb:22:21:22:41 | "id = #{...}" | semmle.label | "id = #{...}" |
|
||||
| ActiveRecordInjection.rb:22:29:22:34 | call to params : | semmle.label | call to params : |
|
||||
| ActiveRecordInjection.rb:28:16:28:21 | <<-SQL | semmle.label | <<-SQL |
|
||||
| ActiveRecordInjection.rb:29:20:29:25 | call to params : | semmle.label | call to params : |
|
||||
| ActiveRecordInjection.rb:32:35:32:60 | "user.id = #{...}" | semmle.label | "user.id = #{...}" |
|
||||
| ActiveRecordInjection.rb:32:48:32:53 | call to params : | semmle.label | call to params : |
|
||||
| ActiveRecordInjection.rb:40:10:40:15 | call to params : | semmle.label | call to params : |
|
||||
| ActiveRecordInjection.rb:45:21:45:33 | ... + ... | semmle.label | ... + ... |
|
||||
#select
|
||||
| ActiveRecordInjection.rb:19:30:19:44 | ...[...] | ActiveRecordInjection.rb:19:30:19:35 | call to params : | ActiveRecordInjection.rb:19:30:19:44 | ...[...] | This SQL query depends on $@. | ActiveRecordInjection.rb:19:30:19:35 | call to params | a user-provided value |
|
||||
| ActiveRecordInjection.rb:22:21:22:41 | "id = #{...}" | ActiveRecordInjection.rb:22:29:22:34 | call to params : | ActiveRecordInjection.rb:22:21:22:41 | "id = #{...}" | This SQL query depends on $@. | ActiveRecordInjection.rb:22:29:22:34 | call to params | a user-provided value |
|
||||
| ActiveRecordInjection.rb:28:16:28:21 | <<-SQL | ActiveRecordInjection.rb:29:20:29:25 | call to params : | ActiveRecordInjection.rb:28:16:28:21 | <<-SQL | This SQL query depends on $@. | ActiveRecordInjection.rb:29:20:29:25 | call to params | a user-provided value |
|
||||
| ActiveRecordInjection.rb:32:35:32:60 | "user.id = #{...}" | ActiveRecordInjection.rb:32:48:32:53 | call to params : | ActiveRecordInjection.rb:32:35:32:60 | "user.id = #{...}" | This SQL query depends on $@. | ActiveRecordInjection.rb:32:48:32:53 | call to params | a user-provided value |
|
||||
| ActiveRecordInjection.rb:45:21:45:33 | ... + ... | ActiveRecordInjection.rb:40:10:40:15 | call to params : | ActiveRecordInjection.rb:45:21:45:33 | ... + ... | This SQL query depends on $@. | ActiveRecordInjection.rb:40:10:40:15 | call to params | a user-provided value |
|
||||
1
ql/test/query-tests/security/cwe-089/SqlInjection.qlref
Normal file
1
ql/test/query-tests/security/cwe-089/SqlInjection.qlref
Normal file
@@ -0,0 +1 @@
|
||||
queries/security/cwe-089/SqlInjection.ql
|
||||
Reference in New Issue
Block a user