1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
|
From 048e9fc05e18c91838a44e60175e475de8b2aad5 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@hawthorn.email>
Date: Tue, 6 Sep 2022 15:49:26 -0700
Subject: [PATCH] Make sanitize_as_sql_comment more strict
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]
---
.../connection_adapters/abstract/quoting.rb | 11 ++++++++++-
.../lib/active_record/relation/query_methods.rb | 2 ++
activerecord/test/cases/annotate_test.rb | 11 ++++++++---
activerecord/test/cases/relation_test.rb | 10 +++-------
4 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
index 143cd32606..aac5bfe0a0 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
@@ -138,7 +138,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:
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 5451ebef46..8be5d5b0a1 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1035,6 +1035,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!(:annotate, args)
spawn.annotate!(*args)
diff --git a/activerecord/test/cases/annotate_test.rb b/activerecord/test/cases/annotate_test.rb
index 4d71d28f83..21b95a97a1 100644
--- a/activerecord/test/cases/annotate_test.rb
+++ b/activerecord/test/cases/annotate_test.rb
@@ -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{\ASELECT #{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{\ASELECT #{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{\ASELECT #{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
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index ba0d96b68c..48b752a127 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -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
@@ -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
--
2.30.2
|
