Skip to content
This repository

Quote integers when comparing a string column with integers. #162

Open
wants to merge 1 commit into from

4 participants

Dylan Thacker-Smith Ernie Miller Rafael Mendonça França Łukasz Strzałkowski
Dylan Thacker-Smith

Problem

An equality with a string column and integer like

SELECT * FROM `users` WHERE `login_token` = 0 LIMIT 1;

will match any string that doesn't start with a digit in certain databases (including MySQL). However, Arel will simply treat Fixnum and Bignum as literals, and not give the connection an opportunity to quote these values.

See Potential Query Manipulation with Common Rails Practises for details on the potential security vulnerability.

Solution

Arel should allow the connection to quote integers, but needs to provide the correct column type to avoid quoting integers like the argument to LIMIT.

Quoting the integer will avoid this potential security issue in a database independent way. So the above query can be quoted as follows and only match a login_token with the exact string '0'.

SELECT * FROM `users` WHERE `login_token` = '0' LIMIT 1;
Dylan Thacker-Smith dylanahsmith referenced this pull request from a commit February 13, 2013
Commit has since been removed from the repository and is no longer available.
Ernie Miller
Collaborator

Interesting. Definitely along the lines of the short-term solution we were discussing the other day, @tenderlove. Assuming the AR tests pass with this, it's not a bad workaround. Tentatively :+1: on this.

Rafael Mendonça França

I just tested this pull request reverting the changes on rails/rails#9207 and using this test case with mysql2 adapter I still get this SQL:

SELECT COUNT(*) FROM `posts` WHERE `posts`.`title` = 0
Dylan Thacker-Smith

@rafaelfranca I never recommended to completely revert that pull request, only the changes to the predicate builder. Arel delegates to the connection adapters quote method for quoting, which must still must quote integers as strings for string column types.

This pull request is necessary because quoting wasn't even being delegated to the quote method for Fixnum and Bignum, and the column passed in would be irrelevant because it was never cleared.

Rafael Mendonça França

Ok. I misunderstood, sorry. So we still need to fix the regressions on Rails either if this pull request right?

Dylan Thacker-Smith

The regressions in rails were from the changes to the predicate builder.

Dylan Thacker-Smith

Please don't underestimate the importance of this pull request. It addresses a bugs in Arel that is preventing a serious security issue from being fixed.

Attempts to work around this issue have led to regressions in rails. E.g. rails/rails#9207 has been reverted. This is the correct place to fix the issue, without any need for hacks.

Łukasz Strzałkowski

@rafaelfranca @tenderlove any progress on this?

Ernie Miller
Collaborator
ernie commented March 13, 2013

Happy to merge this myself but since I'd talked with @tenderlove about it previously just wanted his input before doing so. Seems like a really good option to me, even with the shuffling of last_column. Might be able to do away with last_column altogether by continuing down this path.

Ernie Miller
Collaborator
ernie commented March 15, 2013

So, I chatted with @tenderlove briefly and he raised a good point (one I missed, oops) which is that this exacerbates the problem we already have of mutating the visitor, which makes this code not thread-safe. It's a bigger problem to address than what we have here, but I have an idea about how we might attack it, based on some of the stuff I've done in Squeel which has worked OK. I'm going to at least try to play with it a bit this weekend and see if I get anywhere.

Dylan Thacker-Smith

@ernie awesome work with commit 68a9554. Now the correct column type is being provided for quoting, so all that was left was to actually quote integers rather than treat them as literals. This is a much simpler pull request after rebasing.

Ernie Miller
Collaborator

Thanks for the :heart:!

Dylan Thacker-Smith

@ernie ping for review

Ernie Miller
Collaborator

@dylanahsmith This seems OK to me. Have you run the full AR test suite with this version of Arel?

Dylan Thacker-Smith

I tested it against AR master, and it didn't introduce any new test failures, but there were existing failures.

Dylan Thacker-Smith Quote integers when comparing a string column with integers.
An equality with a string column and integer like

  SELECT * FROM `users` WHERE `login_token` = 0 LIMIT 1;

will match match any string that doesn't start with a digit in certain
databases, like mysql. Quoting the integer will avoid this problem in
a database independant way.
12fc47c
Dylan Thacker-Smith

Rebased and ran the full AR test suite again with this version of Arel, still doesn't break any tests.

@ernie can we get this merged now? Or is anything else needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Mar 21, 2014
Dylan Thacker-Smith Quote integers when comparing a string column with integers.
An equality with a string column and integer like

  SELECT * FROM `users` WHERE `login_token` = 0 LIMIT 1;

will match match any string that doesn't start with a digit in certain
databases, like mysql. Quoting the integer will avoid this problem in
a database independant way.
12fc47c
This page is out of date. Refresh to see the latest.
4  lib/arel/visitors/to_sql.rb
@@ -555,13 +555,13 @@ def literal o, a; o end
555 555
 
556 556
       alias :visit_Arel_Nodes_BindParam  :literal
557 557
       alias :visit_Arel_Nodes_SqlLiteral :literal
558  
-      alias :visit_Bignum                :literal
559  
-      alias :visit_Fixnum                :literal
560 558
 
561 559
       def quoted o, a
562 560
         quote(o, column_for(a))
563 561
       end
564 562
 
  563
+      alias :visit_Bignum                        :quoted
  564
+      alias :visit_Fixnum                        :quoted
565 565
       alias :visit_ActiveSupport_Multibyte_Chars :quoted
566 566
       alias :visit_ActiveSupport_StringInquirer  :quoted
567 567
       alias :visit_BigDecimal                    :quoted
2  test/nodes/test_equality.rb
@@ -43,7 +43,7 @@ def quote_table_name(*args) @quote_count += 1; super; end
43 43
             attr = Table.new(:users)[:id]
44 44
             test = attr.eq(10)
45 45
             test.to_sql engine
46  
-            engine.connection.quote_count.must_equal 2
  46
+            engine.connection.quote_count.must_equal 3
47 47
           end
48 48
         end
49 49
       end
14  test/support/fake_record.rb
@@ -60,9 +60,13 @@ def schema_cache
60 60
     end
61 61
 
62 62
     def quote thing, column = nil
63  
-      if column && column.type == :integer
64  
-        return 'NULL' if thing.nil?
65  
-        return thing.to_i
  63
+      if column && !thing.nil?
  64
+        case column.type
  65
+        when :integer
  66
+          thing = thing.to_i
  67
+        when :string
  68
+          thing = thing.to_s
  69
+        end
66 70
       end
67 71
 
68 72
       case thing
@@ -107,6 +111,10 @@ def columns_hash
107 111
     def schema_cache
108 112
       connection
109 113
     end
  114
+
  115
+    def quote thing, column = nil
  116
+      connection.quote thing, column
  117
+    end
110 118
   end
111 119
 
112 120
   class Base
16  test/visitors/test_to_sql.rb
@@ -111,6 +111,12 @@ def dispatch
111 111
           sql.must_be_like %{ "users"."id" = 1 }
112 112
         end
113 113
 
  114
+        it 'should use the column to quote integers' do
  115
+          table = Table.new(:users)
  116
+          sql = @visitor.accept Nodes::Equality.new(table[:name], 0)
  117
+          sql.must_be_like %{ "users"."name" = '0' }
  118
+        end
  119
+
114 120
         it 'should handle nil' do
115 121
           sql = @visitor.accept Nodes::Equality.new(@table[:name], nil)
116 122
           sql.must_be_like %{ "users"."name" IS NULL }
@@ -144,6 +150,16 @@ def dispatch
144 150
         assert_match(/LIMIT 'omg'/, @visitor.accept(sc))
145 151
       end
146 152
 
  153
+      it "should quote LIMIT without column" do
  154
+        sc = Arel::Nodes::SelectStatement.new
  155
+        core = sc.cores.first
  156
+        core.from = Table.new(:users)
  157
+        core.projections << Arel.star
  158
+        core.wheres << Nodes::Equality.new(core.from[:name], 0)
  159
+        sc.limit = Arel::Nodes::Limit.new(1)
  160
+        assert_match(/WHERE "users"."name" = '0' LIMIT 1/, @visitor.accept(sc))
  161
+      end
  162
+
147 163
       it "should visit_DateTime" do
148 164
         @visitor.accept DateTime.now
149 165
       end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.