Skip to content

Ruby: refine ActiveRecord update_all as an SQL sink #14627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hmac merged 2 commits into from
Dec 4, 2023
Merged

Ruby: refine ActiveRecord update_all as an SQL sink #14627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
013e7aa
Ruby: test whitespace changes
alexrford Oct 29, 2023
8db23dc
Ruby: refine ActiveRecord update_all as an SQL sink
alexrford Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ruby/ql/lib/change-notes/2023-10-30-update-all.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Improved modeling for `ActiveRecord`s `update_all` method
16 changes: 15 additions & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ private predicate sqlFragmentArgumentInner(DataFlow::CallNode call, DataFlow::No
"delete_all", "delete_by", "destroy_all", "destroy_by", "exists?", "find_by", "find_by!",
"find_or_create_by", "find_or_create_by!", "find_or_initialize_by", "find_by_sql", "from",
"group", "having", "joins", "lock", "not", "order", "reorder", "pluck", "where", "rewhere",
"select", "reselect", "update_all"
"select", "reselect"
]) and
sink = call.getArgument(0)
or
Expand All @@ -198,6 +198,20 @@ private predicate sqlFragmentArgumentInner(DataFlow::CallNode call, DataFlow::No
or
call = activeRecordConnectionInstance().getAMethodCall("execute") and
sink = call.getArgument(0)
or
call = activeRecordQueryBuilderCall("update_all") and
(
// `update_all([sink, var1, var2, var3])`
sink = call.getArgument(0).getALocalSource().(DataFlow::ArrayLiteralNode).getElement(0)
or
// or arg0 is not of a known "safe" type
sink = call.getArgument(0) and
not (
sink.getALocalSource() = any(DataFlow::ArrayLiteralNode arr) or
Copy link
Copy Markdown
Contributor

@aibaars aibaars Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to implement a syntactic check to determine whether the argument is an array or hash. While this improves the status quo, I think we could use a type tracker instead to improve accuracy even more.

sink.getALocalSource() = any(DataFlow::HashLiteralNode hash) or
sink.getALocalSource() = any(DataFlow::PairNode pair)
)
)
}

private predicate sqlFragmentArgument(DataFlow::CallNode call, DataFlow::Node sink) {
Expand Down
22 changes: 17 additions & 5 deletions ruby/ql/test/query-tests/security/cwe-089/ActiveRecordInjection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,21 @@ def some_request_handler
# BAD: executes `UPDATE "users" SET #{params[:fields]}`
# where `params[:fields]` is unsanitized
User.update_all(params[:fields])


# GOOD -- `update_all` sanitizes its bind variable arguments
User.find_by(name: params[:user_name])
.update_all(['name = ?', params[:new_user_name]])

# BAD -- `update_all` does not sanitize its query (array arg)
User.find_by(name: params[:user_name])
.update_all(["name = '#{params[:new_user_name]}'"])

# BAD -- `update_all` does not sanitize its query (string arg)
User.find_by(name: params[:user_name])
.update_all("name = '#{params[:new_user_name]}'")

User.reorder(params[:direction])

User.count_by_sql(params[:custom_sql_query])
end
end
Expand Down Expand Up @@ -168,13 +180,13 @@ def index
result = Regression.find_by_sql(query)
end


def permitted_params
params.require(:my_key).permit(:id, :user_id, :my_type)
end

def show
ActiveRecord::Base.connection.execute("SELECT * FROM users WHERE id = #{permitted_params[:user_id]}")
Regression.connection.execute("SELECT * FROM users WHERE id = #{permitted_params[:user_id]}")
end
end
end
Loading