Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Check against bit string values using multiline regexp 
Fix CVE-2014-3482.
  • Loading branch information
@rafaelfranca
rafaelfranca committed Jul 2, 2014
1 parent 297bff7 commit 1f2192e
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 3 deletions.
6 changes: 3 additions & 3 deletions activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
View file
Expand Up @@ -442,8 +442,8 @@ def quote(value, column = nil) #:nodoc:
when 'xml' then "xml '#{quote_string(value)}'"
when /^bit/
case value
when /^[01]*$/ then "B'#{value}'" # Bit-string notation
when /^[0-9A-F]*$/i then "X'#{value}'" # Hexadecimal notation
when /\A[01]*\Z/ then "B'#{value}'" # Bit-string notation
when /\A[0-9A-F]*\Z/i then "X'#{value}'" # Hexadecimal notation
end
else
super
Expand Down Expand Up @@ -1160,7 +1160,7 @@ def translate_exception(exception, message)
FEATURE_NOT_SUPPORTED = "0A000" # :nodoc:

def exec_no_cache(sql, binds)
@connection.async_exec(sql)
@connection.async_exec(sql, [])

This comment has been minimized.

Sign in to view

Copy link
@jmehnle

jmehnle Jul 10, 2014

What's the significance of this change in addressing the security vulnerability? My team noticed that this causes PostgreSQL to fail with a "cannot insert multiple commands into a prepared statement" error message if multiple SQL statements are passed in a single query. Is passing the explicit empty array even in the absence of any actual bind parameters intended to force the pg gem to call PQexecParams instead of PQexec? We're going to change our own app, but this breaks compatibility with any Rails app issuing multiple SQL statements in a single query, for no apparent benefit.

CC: @agaridata/engineering @vidurapparao @zmt

This comment has been minimized.

Sign in to view

Copy link
@rafaelfranca

rafaelfranca Jul 10, 2014

Author Member

Yes, it is to force the pg gem to use PQexecParams and avoid future security issues like this one.

This comment has been minimized.

Sign in to view

Copy link
@jmehnle

jmehnle Jul 10, 2014

I'm probably missing something — how does this make a difference if the array of params passed is empty?
In any case, should this incompatibility with multiple statements per query be documented somewhere?

This comment has been minimized.

Sign in to view

Copy link
@rafaelfranca

rafaelfranca Jul 10, 2014

Author Member

It make difference because it would not be possible to execute multiple statements in the same call to exec_no_cache. This way vulnerabilities like the fixed on this commit will not be so destructive. Attackers can not use a vulnerability on the quote code to execute a DESTROY query in a find call for example.

In any case, should this incompatibility with multiple statements per query be documented somewhere?

As far I know it was never documented you can execute multiple statements per query. But I'm 👍 to document it is not possible.
end

def exec_cache(sql, binds)
Expand Down
5 changes: 5 additions & 0 deletions activerecord/test/cases/adapters/postgresql/quoting_test.rb
View file
Expand Up @@ -19,6 +19,11 @@ def test_type_cast_false
assert_equal 'f', @conn.type_cast(false, nil)
assert_equal 'f', @conn.type_cast(false, c)
end

def test_quote_bit_string
c = PostgreSQLColumn.new(nil, 1, 'bit')
assert_equal nil, @conn.quote("'); SELECT * FORM users; /*\n01\n*/--", c)
end
end
end
end
Expand Down

0 comments on commit 1f2192e

Please sign in to comment.