#122 ✓resolved
Sam Smoot

Security: Escape sequences in query values

Reported by Sam Smoot | January 2nd, 2008 @ 08:52 PM

We're not handling escape sequences correctly in queries apparently.

This is a major security issue. We need to gsub \ with \\.

As a part of this, do we need to thing about other sequences though? Do we need to replace newlines, carriage returns or tabs? Does that make sense at all? I've never done that before, but then again, I've never had an issue with back-slashes in values either, so I'm on unfamiliar footing with MySQL/Postgres/Sqlite3 and how each relates to this issue.

As a side note, this is a DataObjects issue for now. Though I'll be happy to patch it myself once I know for sure the appropriate action to take...

Comments and changes to this ticket

  • Quinn Slack

    Quinn Slack January 2nd, 2008 @ 09:13 PM

    Specifically: when using bind variables, a bind value for a string column ending in a single backslash will result in the right single quote surrounding the value to be escaped. In most cases, this results in an SQL syntax error (since the string value is left unterminated). This could potentially allow for arbitrary SQL execution. (I tried for about 30 minutes to come up with code that'd execute arbitrary SQL queries but was unsuccessful; it could still be possible, though.)

    Here's some code that demonstrates the problem. You should be able to duplicate this problem on your existing models by running Model.create with arguments like those shown below.

    Model:

    class Company < DataMapper::Base
      property :name, :string
    end
    

    Code:

    Company.create(:name=>'Hello!\\')
    

    The error:

    DataObject::QueryError: Your query failed.
    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 ''Hello!\')' at line 1
    INSERT INTO `companies` (`name_official`) VALUES ?
    	from /opt/local/lib/ruby/gems/1.8/gems/do_mysql-0.2.2/lib/do_mysql.rb:193:in `execute_non_query'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/adapters/data_object_adapter.rb:286:in `create'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/adapters/data_object_adapter.rb:77:in `connection'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/support/connection_pool.rb:68:in `hold'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/adapters/data_object_adapter.rb:77:in `connection'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/adapters/data_object_adapter.rb:285:in `create'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/adapters/data_object_adapter.rb:209:in `send'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/adapters/data_object_adapter.rb:209:in `save'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/context.rb:60:in `save'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/support/active_record_impersonation.rb:11:in `save'
    	from /opt/local/lib/ruby/gems/1.8/gems/datamapper-0.2.5/lib/data_mapper/support/active_record_impersonation.rb:89:in `create'
    	from (irb):2
    
  • Tim Pope

    Tim Pope January 25th, 2008 @ 02:33 PM

    The attached patch adds specs to all three adapters for escaping and provides the changes necessary to make them pass. The spec files for do_mysql and do_postgres should be renamed to something for meaningful as well, but the patch does not do this.

    For SQLite3, single quote doubling is all that is necessary, and only the specs were updated.

    MySQL supports backslash escape sequences in strings. Doubling both single quotes and backslashes is a robust solution.

    PostgreSQL supports backslashes escape sequences in strings as well, though this is deprecated. A new syntax has been added, E'...', which will continue to support backslashes. The attached patch simply uses the same solution as for MySQL, which currently works as expected. A more ideal solution would be to check for the presence of the E'...' syntax and use that if it's available, but I've gone for the easy fix for now.

  • Scott Bauer

    Scott Bauer March 29th, 2008 @ 08:21 PM

    I've update do_mysql to use mysql.h's built-in "mysql_real_escape_string" method to properly quote strings.

    http://github.com/sam/do/commit/0727c202514557521cf23896b7aac5934a68ef0e

  • Dan Kubb (dkubb)
  • Sam Smoot

    Sam Smoot April 28th, 2008 @ 12:31 PM

    • Assigned user changed from “Yehuda Katz” to “Scott Bauer”

    Scott, is this still in place? Do you know the status of do_sqlite3 and do_postgres? I wanna close this ticket...

    Also, can you make sure that the default quoting.rb is specced to do this as well so that if a particular driver doesn't quote itself, it can fall-back to quoting.rb and still get handled?

  • Sam Smoot
  • Scott Bauer

    Scott Bauer April 28th, 2008 @ 05:55 PM

    Added proper string escaping to do_postgres and do_sqlite3 (do_mysql was already updated to support this).

    http://github.com/sam/do/commit/...

    http://github.com/sam/do/commit/...

  • Sam Smoot

    Sam Smoot April 28th, 2008 @ 06:15 PM

    • State changed from “open” to “resolved”

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

People watching this ticket

Attachments

Referenced by

Pages