Skip to content

Commit

Permalink
Make sanitize_as_sql_comment more strict
Browse files Browse the repository at this point in the history
Though this method was likely never meant to take user input, it was
attempting sanitization. That sanitization could be bypassed with
carefully crafted input.

This commit makes the sanitization more robust by replacing any
occurrances of "/*" or "*/" with "/ *" or "* /". It also performs a
first pass to remove one surrounding comment to avoid compatibility
issues for users relying on the existing removal.

This also clarifies in the documentation of annotate that it should not
be provided user input.

[CVE-2023-22794]
  • Loading branch information
jhawthorn committed Jan 17, 2023
1 parent 13016ce commit 732bd05
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,16 @@ def quoted_binary(value) # :nodoc:
end

def sanitize_as_sql_comment(value) # :nodoc:
value.to_s.gsub(%r{ (/ (?: | \g<1>) \*) \+? \s* | \s* (\* (?: | \g<2>) /) }x, "")
# Sanitize a string to appear within a SQL comment
# For compatibility, this also surrounding "/*+", "/*", and "*/"
# charcacters, possibly with single surrounding space.
# Then follows that by replacing any internal "*/" or "/ *" with
# "* /" or "/ *"
comment = value.to_s.dup
comment.gsub!(%r{\A\s*/\*\+?\s?|\s?\*/\s*\Z}, "")
comment.gsub!("*/", "* /")
comment.gsub!("/*", "/ *")
comment
end

def column_name_matcher # :nodoc:
Expand Down
13 changes: 12 additions & 1 deletion activerecord/lib/active_record/query_logs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ module ActiveRecord
# ActiveSupport::CurrentAttributes can be used to store application values. Tags with +nil+ values are
# omitted from the query comment.
#
# Escaping is performed on the string returned, however untrusted user input should not be used.
#
# Example:
#
# config.active_record.query_log_tags = [
Expand Down Expand Up @@ -127,7 +129,16 @@ def uncached_comment
end

def escape_sql_comment(content)
content.to_s.gsub(%r{ (/ (?: | \g<1>) \*) \+? \s* | \s* (\* (?: | \g<2>) /) }x, "")
# Sanitize a string to appear within a SQL comment
# For compatibility, this also surrounding "/*+", "/*", and "*/"
# charcacters, possibly with single surrounding space.
# Then follows that by replacing any internal "*/" or "/ *" with
# "* /" or "/ *"
comment = content.to_s.dup
comment.gsub!(%r{\A\s*/\*\+?\s?|\s?\*/\s*\Z}, "")
comment.gsub!("*/", "* /")
comment.gsub!("/*", "/ *")
comment
end

def tag_content
Expand Down
2 changes: 2 additions & 0 deletions activerecord/lib/active_record/relation/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1330,6 +1330,8 @@ def skip_preloading! # :nodoc:
# # SELECT "users"."name" FROM "users" /* selecting */ /* user */ /* names */
#
# The SQL block comment delimiters, "/*" and "*/", will be added automatically.
#
# Some escaping is performed, however untrusted user input should not be used.
def annotate(*args)
check_if_method_has_arguments!(__callee__, args)
spawn.annotate!(*args)
Expand Down
11 changes: 8 additions & 3 deletions activerecord/test/cases/annotate_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,22 @@ def test_annotate_wraps_content_in_an_inline_comment
def test_annotate_is_sanitized
quoted_posts_id, quoted_posts = regexp_escape_table_name("posts.id"), regexp_escape_table_name("posts")

assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/}i) do
assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \* /foo/ \* \*/}i) do
posts = Post.select(:id).annotate("*/foo/*")
assert posts.first
end

assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/}i) do
assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \*\* //foo// \*\* \*/}i) do
posts = Post.select(:id).annotate("**//foo//**")
assert posts.first
end

assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* foo \*/ /\* bar \*/}i) do
assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \* \* //foo// \* \* \*/}i) do
posts = Post.select(:id).annotate("* *//foo//* *")
assert posts.first
end

assert_sql(%r{SELECT #{quoted_posts_id} FROM #{quoted_posts} /\* \* /foo/ \* \*/ /\* \* /bar \*/}i) do
posts = Post.select(:id).annotate("*/foo/*").annotate("*/bar")
assert posts.first
end
Expand Down
5 changes: 3 additions & 2 deletions activerecord/test/cases/query_logs_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ def test_escaping_good_comment_with_custom_separator
end

def test_escaping_bad_comments
assert_equal "; DROP TABLE USERS;", ActiveRecord::QueryLogs.send(:escape_sql_comment, "*/; DROP TABLE USERS;/*")
assert_equal "; DROP TABLE USERS;", ActiveRecord::QueryLogs.send(:escape_sql_comment, "**//; DROP TABLE USERS;/*")
assert_equal "* /; DROP TABLE USERS;/ *", ActiveRecord::QueryLogs.send(:escape_sql_comment, "*/; DROP TABLE USERS;/*")
assert_equal "** //; DROP TABLE USERS;/ *", ActiveRecord::QueryLogs.send(:escape_sql_comment, "**//; DROP TABLE USERS;/*")
assert_equal "* * //; DROP TABLE USERS;// * *", ActiveRecord::QueryLogs.send(:escape_sql_comment, "* *//; DROP TABLE USERS;//* *")
end

def test_basic_commenting
Expand Down
10 changes: 3 additions & 7 deletions activerecord/test/cases/relation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ def test_relation_with_annotation_chains_sql_comments

def test_relation_with_annotation_filters_sql_comment_delimiters
post_with_annotation = Post.where(id: 1).annotate("**//foo//**")
assert_match %r{= 1 /\* foo \*/}, post_with_annotation.to_sql
assert_includes post_with_annotation.to_sql, "= 1 /* ** //foo// ** */"
end

def test_relation_with_annotation_includes_comment_in_count_query
Expand All @@ -367,13 +367,9 @@ def test_relation_without_annotation_does_not_include_an_empty_comment

def test_relation_with_optimizer_hints_filters_sql_comment_delimiters
post_with_hint = Post.where(id: 1).optimizer_hints("**//BADHINT//**")
assert_match %r{BADHINT}, post_with_hint.to_sql
assert_no_match %r{\*/BADHINT}, post_with_hint.to_sql
assert_no_match %r{\*//BADHINT}, post_with_hint.to_sql
assert_no_match %r{BADHINT/\*}, post_with_hint.to_sql
assert_no_match %r{BADHINT//\*}, post_with_hint.to_sql
assert_includes post_with_hint.to_sql, "/*+ ** //BADHINT// ** */"
post_with_hint = Post.where(id: 1).optimizer_hints("/*+ BADHINT */")
assert_match %r{/\*\+ BADHINT \*/}, post_with_hint.to_sql
assert_includes post_with_hint.to_sql, "/*+ BADHINT */"
end

def test_does_not_duplicate_optimizer_hints_on_merge
Expand Down

0 comments on commit 732bd05

Please sign in to comment.