rubygem-activerecord/CVE-2023-22794.patch

From d7aba06953f9fa789c411676b941d20df8ef73de 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 ++++++++++-
 activerecord-7.0.4/lib/active_record/query_logs.rb        | 13 ++++++++++++-
 .../lib/active_record/relation/query_methods.rb     |  2 ++
 activerecord-7.0.4/test/cases/annotate_test.rb            | 11 ++++++++---
 activerecord-7.0.4/test/cases/query_logs_test.rb          |  5 +++--
 activerecord-7.0.4/test/cases/relation_test.rb            | 10 +++-------
 6 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/activerecord-7.0.4/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord-7.0.4/lib/active_record/connection_adapters/abstract/quoting.rb
index dda3145bdd..3b7819eb56 100644
--- a/activerecord-7.0.4/lib/active_record/connection_adapters/abstract/quoting.rb
+++ b/activerecord-7.0.4/lib/active_record/connection_adapters/abstract/quoting.rb
@@ -146,7 +146,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-7.0.4/lib/active_record/query_logs.rb b/activerecord-7.0.4/lib/active_record/query_logs.rb
index f116a154dd..2fd6ca3640 100644
--- a/activerecord-7.0.4/lib/active_record/query_logs.rb
+++ b/activerecord-7.0.4/lib/active_record/query_logs.rb
@@ -33,6 +33,8 @@
   # want to add to the comment. Dynamic content can be created by setting a proc or lambda value in a hash,
   # and can reference any value stored in the +context+ object.
   #
+  # Escaping is performed on the string returned, however untrusted user input should not be used.
+  #
   # Example:
   #
   #    tags = [
@@ -109,7 +111,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
diff --git a/activerecord-7.0.4/lib/active_record/relation/query_methods.rb b/activerecord-7.0.4/lib/active_record/relation/query_methods.rb
index 25136331f9..cf7c524291 100644
--- a/activerecord-7.0.4/lib/active_record/relation/query_methods.rb
+++ b/activerecord-7.0.4/lib/active_record/relation/query_methods.rb
@@ -1216,6 +1216,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)
diff --git a/test/cases/annotate_test.rb b/test/cases/annotate_test.rb
index b0802ca559..ed1d846178 100644
--- a/test/cases/annotate_test.rb
+++ b/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{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
diff --git a/test/cases/query_logs_test.rb b/test/cases/query_logs_test.rb
index 05207f17e3..09ca530417 100644
--- a/test/cases/query_logs_test.rb
+++ b/test/cases/query_logs_test.rb
@@ -42,8 +42,9 @@ def test_escaping_good_comment
   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
diff --git a/test/cases/relation_test.rb b/test/cases/relation_test.rb
index 1da95bd3ae..0aed326678 100644
--- a/test/cases/relation_test.rb
+++ b/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.35.1