When you use SQL bind params where you are binding a string with a question mark, the adapter creates an invalid SQL statement #322

afiedler opened this Issue Feb 5, 2013 · 13 comments

5 participants


Version: 1.2.6

Suppose you try to bind a SQL statement like this

INSERT INTO questions ("question", "answer") VALUES (?,?)

with the values ["How much wood could a woodchuck chuck?", "Who cares"].

The gem runs this code in lib/arjdbc/jdbc/adapter.rb:

      # Substitutes SQL bind (?) parameters
      def to_sql(sql, binds = [])
        sql = sql.send(:to_sql) if sql.respond_to?(:to_sql)
        return sql if binds.blank?
        copy = binds.dup
        sql.gsub('?') { quote(*copy.shift.reverse) }

Where sql.gsub is called with a block that returns a string with a "?", the sql string is tainted as per http://apidock.com/ruby/String/gsub. This means that in the above query, the first question mark will be gsubbed with 'How much wood could a woodchuck chuck?'. The second time through the block, sql.gsub will substitute the question mark after chuck with the second bind string. This results in an invalid SQL query (obviously) like this:

INSERT INTO questions ("question", "answer") VALUES ('How much wood could a woodchuck chuck'Who cares'',?)

Looking more into this.

Here's what I think is the issue. The gsub code above is OK unless you attempt to bind variables twice. Say you start out with

INSERT INTO questions ("question", "answer") VALUES (?,?)

Then you bind the some values to the ?'s. You end up with:

INSERT INTO questions ("question", "answer") VALUES ('How much wood?','Who cares')

If you were to attempt to bind this again using that gsub code. You'd have the above issue, ending up with SQL like this:

INSERT INTO questions ("question", "answer") VALUES ('How much wood'How much wood?'','Who cares')


Why the heck would you do that? Well, Rails does that in activerecord-3.2.11/lib/active_record/connection_adapters/abstract/database_statements.rb:

      def insert(arel, name = nil, pk = nil, id_value = nil, sequence_name = nil, binds = [])
        sql, binds = sql_for_insert(to_sql(arel, binds), pk, id_value, sequence_name, binds)
        value      = exec_insert(sql, name, binds)
        id_value || last_inserted_id(value)

The first line of this method binds the bind variables with the to_sql(arel,binds) call. However, then the library tries to rebind the same variables with exec_insert(sql,name,binds).

There's the problem (I think). I don't think its a good idea to use gsub to handle variable binding like this in SQL. You really need to parse the SQL and do the binding, AFAIK. Doesn't JDBC support this?

JRuby Team member

We are having this problem with very simple use:

Foo.create! :bar => 'bar2', :baz => 'baz2?'

This will generate the following SQL:

INSERT INTO "foos" ("bar", "baz") VALUES ('bar2', 'baz2'bar2') RETURNING "id"

This is with ARJDBC 1.2.6. Reverting to 1.2.5 avoids the problem.

JRuby Team member

OK, thanks a lot ... I did not expect such a regression in 1.2.6 thus I'll try to release 1.2.7 (this or next week).

@kares kares was assigned Feb 6, 2013
JRuby Team member

it seems there was only a logical typo in the code - and it (otherwise) works, work-around is to copy-paste-eval the AR version based branched code (e.g. in an initializer) for any AR (Rails) >= 3.0 :

require 'arjdbc'
require 'arjdbc/jdbc'
ActiveRecord::ConnectionAdapters::JdbcAdapter.class_eval do
  def to_sql(arel, binds = [])
    if arel.respond_to?(:ast)
      visitor.accept(arel.ast) do
    else # for backwards compatibility :
      sql = arel.respond_to?(:to_sql) ? arel.send(:to_sql) : arel
      return sql if binds.blank?
      copy = binds.dup # NOTE: this shall not happen, right ?!
      sql.gsub('?') { quote(*copy.shift.reverse) }

would be great if someone could confirm this resolves the issue on 1.2.6, release should come sometime next week.

@kares kares added a commit that referenced this issue Feb 8, 2013
@kares kares fix 1.2.6 regression - incorrectly setup to_sql method based on Rails…
… version

this caused double '?' bind substitution issues (reported as #322)
@kares kares added a commit that referenced this issue Feb 8, 2013
@kares kares fix 1.2.6 regression - incorrectly setup to_sql method based on Rails…
… version

this caused double '?' bind substitution issues (reported as #322)
JRuby Team member

it is also possible to test the fix out using master of the 1-2-stable branch :
gem 'activerecord-jdbc-adapter', :github => 'jruby/activerecord-jdbc-adapter', :branch => '1-2-stable'

@kares kares closed this Feb 12, 2013
JRuby Team member

for the record - this has been causing a SQL injection vulnerability in 1.2.6 (as @ebeigarts reminded us), sample test (setup by Edgars) at http://cl.ly/code/0n381E353414 1.2.6 is yanked please update ASAP if you happen to be using it !

@bjeanes bjeanes referenced this issue in SquareSquash/web Mar 9, 2013

Add to Travis-CI #59


This still seems to be a problem with rails - I am using 1.2.7 with active-record 3.2.12. My fix was to use exec_update directly rather than update - missing the double bind.

JRuby Team member

@kimptoc thanks, could you please provide us than with a failing piece of code than (against 1.2.7 or master) ?



Not sure if this is an ActiveRecord bug - should it call bind twice?

Also, I am not really using ActiveRecord models per se - just doing a raw DB update - could be I am doing it the wrong way...

Anyway, these steps reproduce the error:

[31] pry(main)> conn = ActiveRecord::Base.connection
[32] pry(main)> sql = "update cities set name = ? where id = 616"
=> "update cities set name = ? where id = 616"
[33] pry(main)> conn.update sql, "blah", [[nil, "city of chris"]]  # UPDATE WORKS
20130312 07:45:04.511:DBG:THD1>  blah (3.0ms)  update cities set name = 'city of chris' where id = 616
=> 1
[34] pry(main)> conn.update sql, "blah", [[nil, "city of chris?"]] # UPDATE FAILS DUE TO DOUBLE BIND
20130312 07:45:09.025:DBG:THD1>  blah (4.0ms)  update cities set name = 'city of chris'city of chris?'' where id = 616
ActiveRecord::StatementInvalid: ActiveRecord::JDBCError: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'city of chris?'' where id = 616' at line 1: update cities set name = 'city of chris'city of chris?'' where id = 616
from arjdbc/jdbc/RubyJdbcConnection.java:215:in `execute'
[35] pry(main)> conn.exec_update sql, "blah", [[nil, "city of chris?"]] # UPDATE WORKS AS EXEC_ DOES NOT DOUBLE BIND
20130312 07:45:12.644:DBG:THD1>  blah (1.0ms)  update cities set name = 'city of chris?' where id = 616
=> 1

Some versions (on OSX 10.8.2):

$ ruby -v                                                          [7:49:27]
jruby 1.7.3 (1.9.3p385) 2013-02-21 dac429b on Java HotSpot(TM) 64-Bit Server VM 1.7.0_15-b03 [darwin-x86_64]

$ rvm -v                                                           [7:50:11]

rvm 1.18.10 (stable) by Wayne E. Seguin <wayneeseguin@gmail.com>, Michal Papis <mpapis@gmail.com> [https://rvm.io/]

My gem list:

$ gem list                                                         [7:49:23]

*** LOCAL GEMS ***

actionmailer (3.2.12)
actionpack (3.2.12)
activemodel (3.2.12)
activerecord (3.2.12)
activerecord-jdbc-adapter (1.2.7)
activerecord-jdbcmysql-adapter (1.2.7)
activeresource (3.2.12)
activesupport (3.2.12)
acts_as_executor (1.0.0.rc5)
addressable (2.3.2)
akami (1.2.0)
arel (3.0.2)
awesome_print (1.1.0)
bcrypt-ruby (3.0.1 java)
bouncy-castle-java (1.5.0146.1)
brakeman (1.9.2)
builder (3.0.4)
bundler (1.2.4)
bundler-audit (0.1.2)
cache (0.4.0)
childprocess (0.3.8)
coderay (1.0.8)
coffee-rails (3.2.2)
coffee-script (2.2.0)
coffee-script-source (1.4.0)
composite_primary_keys (5.0.12)
domain_name (0.5.7)
ejs (1.1.1)
erubis (2.7.0)
execjs (1.4.0)
fastercsv (1.5.5)
ffi (1.4.0 java)
guard (1.6.2)
guard-spork (1.4.2)
guard-test (0.7.0)
gyoku (1.0.0)
haml (3.1.8)
highline (1.6.15)
hike (1.2.1)
httpi (2.0.2)
i18n (0.6.1)
jdbc-mysql (
journey (1.0.4)
jquery-rails (2.1.4)
jruby-lint (0.4.0)
jruby-openssl (0.8.2)
jruby-rack (
json (1.7.7 java)
launchy (2.2.0 java)
letter_opener (1.1.0)
listen (0.7.2)
lumberjack (1.0.2)
mail (2.4.4)
mechanize (2.5.1)
method_source (0.8.1)
mime-types (1.21)
multi_json (1.6.1)
net-http-digest_auth (1.2.1)
net-http-persistent (2.8)
nokogiri (1.5.6 java)
nori (2.0.3)
ntlm-http (0.1.1)
polyglot (0.3.3)
pry (0.9.12 java)
pry-rails (0.2.2)
rabl (0.8.0)
rack (1.4.5)
rack-cache (1.2)
rack-ssl (1.3.3)
rack-test (0.6.2)
rails (3.2.12)
rails-backbone (0.9.10)
railties (3.2.12)
rake (10.0.3)
rdoc (3.12.1)
ruby2ruby (2.0.3)
ruby_parser (3.1.1)
rufus-scheduler (2.0.17)
sass (3.2.5)
sass-rails (3.2.6)
savon (2.1.0)
sexp_processor (4.1.5)
slop (3.4.3)
spoon (0.0.1)
spork (1.0.0rc3)
spork-rails (3.2.1)
spork-testunit (0.0.8)
sprockets (2.2.2)
term-ansicolor (1.0.7)
terminal-table (1.4.5)
test-unit (2.5.4)
thor (0.17.0)
tilt (1.3.3)
treetop (1.4.12)
trinidad (1.4.4)
trinidad_jars (1.2.0)
tzinfo (0.3.35)
uglifier (1.3.0)
unf (0.0.5 jruby)
wasabi (3.0.0)
webrobots (0.1.0)
xml-simple (1.1.2)
@kares kares reopened this Mar 12, 2013
JRuby Team member

@kimptoc thanks again, should be fixed on master as well as 1-2-stable I shall release 1.2.8 just in case tomorrow

@kares kares closed this Mar 12, 2013
@kares kares added a commit that referenced this issue Mar 13, 2013
@kares kares prepare for 1.2.8 due regression (still present double bind suble issue

contains a few backports from master (which targets 1.3.0)

A bit late, but thank you.


Still occurs in master but works with tag v1.3.0.beta2. Is this intended?

JRuby Team member

@milgner for sure not - could we have code that reproduces this on master - this should work esp. if it does with beta2

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