Rails - Dec 2004 - How to implement more database adapters for Active Record

Hi guys,

There is now no less than four people working on new database adapters 
for Active Record. Allow me to introduce them:

* Joey Gibson: Has an almost complete MS SQL Server adapter using DBI 
and ADO ready to be included with Active Record. Just a few tidbits 
remaining.

* Maik Schmidt: Have a lot of stuff up and running for the DB2 adapter.

* Jim Weirich: Has plans to or has just started developing an Oracle 
adapter.

* Eric Ocean: Working on a FrontBase adapter.

One of the biggest problems with bringing these databases to Active 
Record is that they interpret quoting and types much more stringent 
than MySQL, SQLite, and  PostgreSQL. What do do about that?

Eric Ocean wrote me with this suggestion:
> FrontBase is SQL92-conformant, almost to a fault, and simply won''t
> accept SQL that''s not to the standard. Right now, ActiveRecord
isn''t
> maintaining enough information about types to output them correctly. 
> The clean solution, IMHO, is to add a SQL type member to each 
> ActiveRecord object, and set it to the exact SQL type (e.g. :tinyint) 
> coming from the database schema. This can, of course, be done lazily, 
> and the information should be cached in the class. AR should support 
> the union of sql types from all database supported by AR, and new 
> adaptors should be able to add their own. It would be the
adaptor''s
> responsibility to map from their database-native SQL type to the type 
> used internally by AR.
>
> The other change is to give more control over SQL generation for each 
> kind of SQL statement (INSERT, UPDATE, SELECT, etc.) so that quoting 
> can be controlled on a per statement basis. Existing adaptors would 
> just alias the various methods to the current implementations, so no 
> extra work for them. More sophisticated databases like FrontBase and 
> Oracle can use the SQL type along with more info about the context of 
> the quote generation to produce valid SQL.
>
> I suspect that SQL types would also ease the implementation of CLOBs 
> and BLOBs which require special handling for each kind of database.
Maik Schmidt wrote about his problems with DB2:
> A bigger problems is DB2 itself (at least Version 7.1, which is the 
> version I have to deal with). ActiveRecord assumes that it''s
possible
> to quote every value and you can find a lot of '' characters in the
> code. For example, in class HasAndBelongsToManyAssociation there''s
a
> method that looks like this:
>
> def insert_record(record)
>   if @options[:insert_sql]
>     @owner.connection.execute(
>       interpolate_sql(@options[:insert_sql], record)
>     )
>   else
>     sql = "INSERT INTO #{@join_table}
>         (#{@association_class_primary_key_name},
>          #{@association_foreign_key}) VALUES
>        
(''#{@owner.id}'',''#{record.id}'')"
>           @owner.connection.execute(sql)
>   end
> end
>
> Obviously, that''s fine SQL for a lot of databases, but ugly old
DB2
> complains like this:
>
> ERROR From DB2: INSERT INTO projects (name, id) VALUES(''Active 
> Record'', ''1'')
> DB2 return code is SQL_ERROR
> [IBM][CLI Driver][DB2/LINUX] SQL0408N  A value is not compatible with 
> the data type of its assignment target.  Target name is "ID". 
> SQLSTATE=42821E
>
> The column ''id'' has been declared as integer and
''1'' is no integer
> literal, so DB2 refuses to do its dirty work. What should we do now? 
> As far as I can see, there are some places, where the quotes have to 
> be replaced, but we should do something better to prevent surprises 
> while integrating the next database.
So it seems like something must indeed be done. One of the easy ways 
would of course be to make sure that every time a value is needed any 
where in an SQL statement generated by Active Record, we use a quote 
method that can be overwritten by each adapter.

There is already some underpinnings for this, but it may not be enough 
to satisfy all the databases involved.

Anyway, I just wanted to open up the floor for discussions so that we 
can all work in the same direction. It would be exceptionally cool to 
have all these adapters done and help Active Record and Rails 
tremendously.

Let''s get a solution and move forward ASAP.
--
David Heinemeier Hansson,
http://www.basecamphq.com/   -- Web-based Project Management
http://www.rubyonrails.org/  -- Web-application framework for Ruby
http://macromates.com/       -- TextMate: Code and markup editor (OS X)
http://www.loudthinking.com/ -- Broadcasting Brain

David Heinemeier Hansson wrote:
> 
> So it seems like something must indeed be done. One of the easy ways 
> would of course be to make sure that every time a value is needed any 
> where in an SQL statement generated by Active Record, we use a quote 
> method that can be overwritten by each adapter.
Makes sense - what would be the downside?
> 
> There is already some underpinnings for this, but it may not be enough 
> to satisfy all the databases involved.
> 
> Anyway, I just wanted to open up the floor for discussions so that we 
> can all work in the same direction. It would be exceptionally cool to 
> have all these adapters done and help Active Record and Rails 
> tremendously.
As for the problem with int''s being quoted etc., the framework should
not quote any field that doesn''t need to be quoted based on
it''s
datatype (int etc.).  I have seen wierdness with PostgreSQL and others
where quoting an int value messed up the query planner as it didn''t
treat the data type properly (I know, lame)

So it should be

SELECT * FROM t WHERE id=1, not WHERE id=''1'' and the same for
updates
and inserts.


> 
> Let''s get a solution and move forward ASAP.
> --
> David Heinemeier Hansson,
> http://www.basecamphq.com/   -- Web-based Project Management
> http://www.rubyonrails.org/  -- Web-application framework for Ruby
> http://macromates.com/       -- TextMate: Code and markup editor (OS X)
> http://www.loudthinking.com/ -- Broadcasting Brain
> 
> _______________________________________________
> Rails mailing list
> Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org
> http://lists.rubyonrails.org/mailman/listinfo/rails

David Heinemeier Hansson wrote:
> So it seems like something must indeed be done. One of the easy ways 
> would of course be to make sure that every time a value is needed any 
> where in an SQL statement generated by Active Record, we use a quote 
> method that can be overwritten by each adapter.
This seems as though it might create more work than necessary, but I 
don''t know enough about Rails'' internals to say for sure. If
the quoting
were made dependent on data type, would that fix the issue for all 
databases, without having to resort to adapter-specific coding? My 
experience is mostly with MySQL and a bit with Oracle 8i, so I don''t
know.

''Standardized'' though SQL is, implementations of it seem to
vary more
widely than nearly any other language I''ve seen. Perhaps this has 
improved in the past couple years, and if so it would make it easier to 
refactor how ActiveRecord writes its SQL. If not, one option might be to 
move the actual writing of SQL into the adapters. A ''default''
(i.e.,
SQL92-compliant) way of doing it could be implemented in AbstractAdapter.

-- 
Kevin R. Bullock
Internet Systems Designer and Administrator
Minnesota Center Against Violence and Abuse
University of Minnesota
612.624.8796

Kevin Bullock wrote:
> David Heinemeier Hansson wrote:
> 
>> So it seems like something must indeed be done. One of the easy ways 
>> would of course be to make sure that every time a value is needed any 
>> where in an SQL statement generated by Active Record, we use a quote 
>> method that can be overwritten by each adapter.
> 
> This seems as though it might create more work than necessary, but I 
> don''t know enough about Rails'' internals to say for sure.
If the quoting
> were made dependent on data type, would that fix the issue for all 
> databases, without having to resort to adapter-specific coding? My 
> experience is mostly with MySQL and a bit with Oracle 8i, so I
don''t know.
An escape method per adaptor would be the best route as MySQL escapes
with ''\'', and Oracle escapes with
'''''' etc, or at least have a default
escape with ''\'', and have an adaptor override it if needed
(like
Oracle).  The SQL doesn''t need to be built in the adaptors, that can
stay the way it is, just don''t quote values that don''t need to
be quoted.

Hi,

Why instead of generating inline SQL statements like AR is doing and passing
them throught the respective adapter to be executed, AR could generete SQL
statements with parameters and send these statements along with an array of
parameters to be binded by the adapters. That way the quotings could be
eliminated i guess.

So in summary instead of :

- sql = "INSERT INTO projects (name, id) VALUES(''Active 
Record'', ''1'')"
and calling <adapter>.[execute | insert | select_one](sql)

one would use
sql = "INSERT INTO projects (name, id) VALUES(?, ?)" 
params = [''Active Record'', 1]

and calling <adapter>.[execute | insert | select_one](sql, params) which
would bind the parameters to the statement.


I''ve been playing around with a native ruby FirebirdSQL driver build
upon IBPP lib (pre-alpha stuff)  and for working with BLOB field the only way to
be able to use them is by using parameters.

Another issue with AR is it doesn''t like quoted field names, (eg:
"First Name"), i know i can use (first_name) but some databases
convert unquoted field names to uppercase. But i can live with that...

Jorge

----- Original Message ----- 
From: "David Heinemeier Hansson"
<david-OiTZALl8rpK0mm7Ywyx6yg@public.gmane.org>
To: <rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org>; "Joey
Gibson" <joey-JZ7uTQG3sB7SUeElwK9/Pw@public.gmane.org>; "Jim
Weirich" <jim-Fxty1mrVU9GlFc2d6oM/ew@public.gmane.org>; "Maik
Schmidt" <contact-CKba8aKGf+ewktNWMKzPnQ@public.gmane.org>;
"Eric Ocean"
<eric.ocean-npdh1YFn4ZDQT0dZR+AlfA@public.gmane.org>
Sent: Wednesday, December 01, 2004 2:54 PM
Subject: [Rails] How to implement more database adapters for Active Record


Hi guys,

There is now no less than four people working on new database adapters 
for Active Record. Allow me to introduce them:

* Joey Gibson: Has an almost complete MS SQL Server adapter using DBI 
and ADO ready to be included with Active Record. Just a few tidbits 
remaining.

* Maik Schmidt: Have a lot of stuff up and running for the DB2 adapter.

* Jim Weirich: Has plans to or has just started developing an Oracle 
adapter.

* Eric Ocean: Working on a FrontBase adapter.

One of the biggest problems with bringing these databases to Active 
Record is that they interpret quoting and types much more stringent 
than MySQL, SQLite, and  PostgreSQL. What do do about that?

Eric Ocean wrote me with this suggestion:
> FrontBase is SQL92-conformant, almost to a fault, and simply won''t
> accept SQL that''s not to the standard. Right now, ActiveRecord
isn''t
> maintaining enough information about types to output them correctly. 
> The clean solution, IMHO, is to add a SQL type member to each 
> ActiveRecord object, and set it to the exact SQL type (e.g. :tinyint) 
> coming from the database schema. This can, of course, be done lazily, 
> and the information should be cached in the class. AR should support 
> the union of sql types from all database supported by AR, and new 
> adaptors should be able to add their own. It would be the
adaptor''s
> responsibility to map from their database-native SQL type to the type 
> used internally by AR.
>
> The other change is to give more control over SQL generation for each 
> kind of SQL statement (INSERT, UPDATE, SELECT, etc.) so that quoting 
> can be controlled on a per statement basis. Existing adaptors would 
> just alias the various methods to the current implementations, so no 
> extra work for them. More sophisticated databases like FrontBase and 
> Oracle can use the SQL type along with more info about the context of 
> the quote generation to produce valid SQL.
>
> I suspect that SQL types would also ease the implementation of CLOBs 
> and BLOBs which require special handling for each kind of database.
Maik Schmidt wrote about his problems with DB2:
> A bigger problems is DB2 itself (at least Version 7.1, which is the 
> version I have to deal with). ActiveRecord assumes that it''s
possible
> to quote every value and you can find a lot of '' characters in the
> code. For example, in class HasAndBelongsToManyAssociation there''s
a
> method that looks like this:
>
> def insert_record(record)
>   if @options[:insert_sql]
>     @owner.connection.execute(
>       interpolate_sql(@options[:insert_sql], record)
>     )
>   else
>     sql = "INSERT INTO #{@join_table}
>         (#{@association_class_primary_key_name},
>          #{@association_foreign_key}) VALUES
>        
(''#{@owner.id}'',''#{record.id}'')"
>           @owner.connection.execute(sql)
>   end
> end
>
> Obviously, that''s fine SQL for a lot of databases, but ugly old
DB2
> complains like this:
>
> ERROR From DB2: INSERT INTO projects (name, id) VALUES(''Active 
> Record'', ''1'')
> DB2 return code is SQL_ERROR
> [IBM][CLI Driver][DB2/LINUX] SQL0408N  A value is not compatible with 
> the data type of its assignment target.  Target name is "ID". 
> SQLSTATE=42821E
>
> The column ''id'' has been declared as integer and
''1'' is no integer
> literal, so DB2 refuses to do its dirty work. What should we do now? 
> As far as I can see, there are some places, where the quotes have to 
> be replaced, but we should do something better to prevent surprises 
> while integrating the next database.
So it seems like something must indeed be done. One of the easy ways 
would of course be to make sure that every time a value is needed any 
where in an SQL statement generated by Active Record, we use a quote 
method that can be overwritten by each adapter.

There is already some underpinnings for this, but it may not be enough 
to satisfy all the databases involved.

Anyway, I just wanted to open up the floor for discussions so that we 
can all work in the same direction. It would be exceptionally cool to 
have all these adapters done and help Active Record and Rails 
tremendously.

Let''s get a solution and move forward ASAP.
--
David Heinemeier Hansson,
http://www.basecamphq.com/   -- Web-based Project Management
http://www.rubyonrails.org/  -- Web-application framework for Ruby
http://macromates.com/       -- TextMate: Code and markup editor (OS X)
http://www.loudthinking.com/ -- Broadcasting Brain

_______________________________________________
Rails mailing list
Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org
http://lists.rubyonrails.org/mailman/listinfo/rails

Jorge Sousa <jhsousa@...> writes:
> Why instead of generating inline SQL statements like AR is doing and
passing
them throught the respective
> adapter to be executed, AR could generete SQL statements with parameters
and
send these statements along
> with an array of parameters to be binded by the adapters. That way the
quotings could be eliminated i guess.

This would allow using prepared statements and/or bind variables for those dbs
which support them, which is a good thing

On Wed, 01 Dec 2004 09:54:14 -0600, Kevin Bullock
<kbullock-Kwy/eXMQnt6WVfeAwA7xHQ@public.gmane.org>
wrote:
> > One of the easy ways
> > would of course be to make sure that every time a value is needed any
> > where in an SQL statement generated by Active Record, we use a quote
> > method that can be overwritten by each adapter.
> 
> This seems as though it might create more work than necessary, but I
> don''t know enough about Rails'' internals to say for sure.
If the quoting
> were made dependent on data type, would that fix the issue for all
> databases, without having to resort to adapter-specific coding? My
> experience is mostly with MySQL and a bit with Oracle 8i, so I
don''t know.
> 
> ''Standardized'' though SQL is, implementations of it seem
to vary more
> widely than nearly any other language I''ve seen. Perhaps this has
> improved in the past couple years, and if so it would make it easier to
> refactor how ActiveRecord writes its SQL. If not, one option might be to
> move the actual writing of SQL into the adapters. A
''default'' (i.e.,
> SQL92-compliant) way of doing it could be implemented in AbstractAdapter.
It looks like it''s time to address the lack of encapsulation of SQL. I
agree, I think there should be a Query object that AbstractAdapter (or
its subclass SQLAdapter in the future) can use to write SQL. This
would also make implementing
http://rubyonrails.org/show/MadeleineRecordWish much more possible, as
there wouldn''t be strings to parse...

-- 
Ryan Platte

On Dec 1, 2004, at 9:07 AM, Steve Woodcock wrote:
> Jorge Sousa <jhsousa@...> writes:
>> Why instead of generating inline SQL statements like AR is doing and 
>> passing
> them throught the respective
>> adapter to be executed, AR could generete SQL statements with 
>> parameters and
> send these statements along
>> with an array of parameters to be binded by the adapters. That way the
> quotings could be eliminated i guess.
>
> This would allow using prepared statements and/or bind variables for 
> those dbs
> which support them, which is a good thing
Agreed.

In FrontBase at least, the query planner only uses multi-column indexes 
if they
were originally defined in the same order as they are presented in a 
SELECT.
Getting rid of inline SQL would allow the FrontBase adapter to 
construct queries
that could be executed optimally by the database. In addition, a 
default order
could be defined in AR, such as defining all indexes in alphabetical 
order by
column. Then the optimizer would work in the default case, and other 
database
adapters and database admins could rely on that convention as well.

To further enhance things, we could define an :index_on [ id, column_1, 
columnt_4 ]
configuration that would provide non-alphabetic-order index information 
to the
adapter. (It''s also possible, in FrontBase at least, to pull this 
directly out of
the database. That might be better all the way around. At any rate, AR 
can support
both.)

Although I suspect there is already broad agreement on this, I''ll point
out that
in ActiveRecord, we can''t ignore database optimization. Every database 
is
_designed_ to be optimized to get acceptable performance. Unlike program
optimization, it''s generally not optional. It would be great if Rails 
could define
conventions that facilitated useful database optimization with minimal 
code changes
and certainly without falling back to rolling your own SQL.

Best, Eric

> Although I suspect there is already broad agreement on this, I''ll 
> point out that
> in ActiveRecord, we can''t ignore database optimization. Every
database
> is
> _designed_ to be optimized to get acceptable performance. Unlike 
> program
> optimization, it''s generally not optional. It would be great if
Rails
> could define conventions that facilitated useful database optimization 
> with minimal code changes and certainly without falling back to 
> rolling your own SQL.
I definitely agree. Put all the database specific knowledge about how 
things should work into the adapters. Much everything but making the AR 
user API more complicated is fair game.
--
David Heinemeier Hansson,
http://www.basecamphq.com/   -- Web-based Project Management
http://www.rubyonrails.org/  -- Web-application framework for Ruby
http://macromates.com/       -- TextMate: Code and markup editor (OS X)
http://www.loudthinking.com/ -- Broadcasting Brain

On Wed, 1 Dec 2004 19:29:57 +0100, David Heinemeier Hansson
<david-OiTZALl8rpK0mm7Ywyx6yg@public.gmane.org>
wrote:
> Much everything but making the AR
> user API more complicated is fair game.
To implement a Query object, I imagine adding new methods that would
be an alternative to the raw-SQL methods (which could remain). They
would have parameters and/or a block for query building. Would you
consider that making the API more complicated?

-- 
Ryan Platte

> To implement a Query object, I imagine adding new methods that would
> be an alternative to the raw-SQL methods (which could remain). They
> would have parameters and/or a block for query building. Would you
> consider that making the API more complicated?
Yeah, I''m not a fan of abstracting away SQL. Lots of other ORMs go that
direction (and mostly fail in my opinion). Active Record is for 
relational databases and SQL is the best query language for that.

Whatever tighter integration Rails will get with the likes of Madeleine 
and ActiveLDAP will be through duck typing. Not by trying to abstract 
our way out of it.

So the level of abstraction for SQL should only occur to ensure that 
Active Record will work with more databases. And even better, if we can 
NOT do that by implementing something more clever on types and quoting.
--
David Heinemeier Hansson,
http://www.basecamphq.com/   -- Web-based Project Management
http://www.rubyonrails.org/  -- Web-application framework for Ruby
http://macromates.com/       -- TextMate: Code and markup editor (OS X)
http://www.loudthinking.com/ -- Broadcasting Brain

On Wed, 1 Dec 2004, Jorge Sousa wrote:
> Why instead of generating inline SQL statements like AR is doing and
> passing them throught the respective adapter to be executed, AR could
> generete SQL statements with parameters....
I don''t understand why AR is not doing this now. Let''s look at
what
happens when you don''t do this:

    1. You now have code duplication: both the database driver and AR
    have quoting code for a particular database.

    2. Therefore, you don''t get quoting code changes and bug fixes from
    the driver.

    3. This leads to the potential that the quoting code is different
    between the two.

    4. This can actually lead to security holes, since the AR quoting
    code may improperly miss quoting something and allow SQL injection
    attacks.

SQL injection attacks are actually a big fear of mine with rails; I''ve
not seen any convincing evidence that care has been taken on this
issue, and some evidence (such as this) that it hasn''t be sufficiently
examined. I''d hate to see rails get a reputation akin to that of
PHPNuke, which seems to have a weekly SQL injection attack posted to
bugtraq.

Beyond that, there''s the issue of ruby''s security level,
tainting input
variables, and so on, which I''ve not looked at at all in relation to
rails.

cjs
-- 
Curt Sampson  <cjs-gHs2Wiolu3leoWH0uzbU5w@public.gmane.org>   +81 90 7737
2974   http://www.NetBSD.org
     Make up enjoying your city life...produced by BIC CAMERA

> SQL injection attacks are actually a big fear of mine with rails;
I''ve
> not seen any convincing evidence that care has been taken on this
> issue, and some evidence (such as this) that it hasn''t be
sufficiently
> examined. I''d hate to see rails get a reputation akin to that of
> PHPNuke, which seems to have a weekly SQL injection attack posted to
> bugtraq.
There''s no need to fear. Security is not likely to ever be a bullet 
point on the feature list of a framework. All Rails does is provide you 
with a number of features to _help_ deal with security, like SQL 
injection (see Conditions under 
http://ar.rubyonrails.org/classes/ActiveRecord/Base.html) and guarding 
access to attributes with 
(http://ar.rubyonrails.org/classes/ActiveRecord/Base.html#M000093).

Giving any language or framework the reputation of not dealing with 
SQL-injection is plain silly. As a developer, you''re always the one 
where the buck stops. If some feature of a framework isn''t available or
sufficient, you make sure that your application takes the proper 
measures to deal with it.

Additionally, it''s double silly to compare an application like PHPNuke 
with a framework like Rails. But I trust that this was not the intend 
of your post.

Bottom line: Please be specific about security concerns. If Rails has a 
security bug, I''m the first that wants to hear about it. If you have a 
specific feature that you think would make it easier on you as a 
developer to deal with security, please write up a proposal (your 
points for the quoting is almost that) or better yet supply a patch for 
consideration.
> Beyond that, there''s the issue of ruby''s security level,
tainting input
> variables, and so on, which I''ve not looked at at all in relation
to
> rails.
Just like Rails offer some features for security you can _choose_ to 
use, so does Ruby. For Rails, I''ve currently chosen that the tainting 
facilities haven''t been necessary or helpful in combatting security 
concerns. If someone wants to reevaluate that choice by showing a few 
cases where the tainting could help, I''d be more than happy to listen.
--
David Heinemeier Hansson,
http://www.basecamphq.com/   -- Web-based Project Management
http://www.rubyonrails.org/  -- Web-application framework for Ruby
http://macromates.com/       -- TextMate: Code and markup editor (OS X)
http://www.loudthinking.com/ -- Broadcasting Brain

On Dec 2, 2004, at 5:19, David Heinemeier Hansson wrote:
> Bottom line: Please be specific about security concerns. If Rails has 
> a security bug, I''m the first that wants to hear about it. If you
have
> a specific feature that you think would make it easier on you as a 
> developer to deal with security, please write up a proposal (your 
> points for the quoting is almost that) or better yet supply a patch 
> for consideration.
Interesting, I actually posted a bug report on this at the start of the 
week, then withdrew it a couple of hours later. I''m not thinking that I
should have let it stand.

I see the issue as this. In SQL dialects that allow substitution into 
commands, you are protected against sql injection. If you say

     stmt = db.prepare("select * from people where id=?")
     stmt.execute(@params[p_id"])

it doesn''t matter what''s in the parameter. You expect an
integer, but
if it contains the string "1; select * from passwords" it
won''t make a
difference.

In Rails, the documentation for criteria makes it look as if the same 
is true. It isn''t. If I say

    table.find_all([ "id=%d", @params{"p_id"]))

then a malicious incoming string will indeed inject SQL into my system. 
Rails _looks_ as if it gives protection, but in reality as a developer 
you have to take responsibility for your own. You have to know the 
quoting rules for your database, and make sure to take account of them 
in each criteria you use.

I''d very much like to see Rails move towards

   table.find_all([ "id = ?", id] )

and have it use the underlying database to do the substitution.


Cheers

Dave

On Dec 3, 2004, at 10:17, Dave Thomas wrote:
> In Rails, the documentation for criteria makes it look as if the same 
> is true. It isn''t. If I say
>
>    table.find_all([ "id=%d", @params{"p_id"]))
> then a malicious incoming string will indeed inject SQL into my system.

Typo - I should have said "id=%s"

Dave

Dave Thomas said:
> I''d very much like to see Rails move towards
>
>    table.find_all([ "id = ?", id] )
>
> and have it use the underlying database to do the substitution.
Ruby DBI handles this by providing tools for the driver writers to use to
handle quoting and parameter binding, even if the underlying database does
not.

The code of interest is in the sql.rb file, modules BasicBind (for
binding) and BasicQuote (for quoting).  Driver writer can override or
replace these services if they are not appropriate for their particular
database.

You might not be worried about SQL injection problems, but I attended a
security forum yesterday and saw a demo of SQL injection.  By typing
"'' OR
1=1" into the name field of a login screen, the user was able to log in as
a random user without using a password.  Granted, the demo was designed
with this in mind, but it does point out the potential problems if we are
not careful.

-- 
-- Jim Weirich     jim-Fxty1mrVU9GlFc2d6oM/ew@public.gmane.org   
http://onestepback.org
-----------------------------------------------------------------
"Beware of bugs in the above code; I have only proved it correct,
not tried it." -- Donald Knuth (in a memo to Peter van Emde Boas)

I am not really well versed in AR or Rails (nor the database adapters 
in Ruby) but using "bind variables" is usually the way to avoid SQL 
injection. Using bind variables also has a nice side effect that it 
increases performance (generally).

I am not sure if the database adapters for Ruby have support for that 
(I am using PostgreSQL) but I will have a look. Also, not sure how 
complex it would be to change AR if the Ruby-PGSQL adapter has support 
for that ... or if it''s at all possible with support for multiple 
RDBMS.

Just my 2 cents,

/B

On 04/12/2004, at 2:17 AM, Dave Thomas wrote:
>
> On Dec 2, 2004, at 5:19, David Heinemeier Hansson wrote:
>
>> Bottom line: Please be specific about security concerns. If Rails has 
>> a security bug, I''m the first that wants to hear about it. If
you
>> have a specific feature that you think would make it easier on you as 
>> a developer to deal with security, please write up a proposal (your 
>> points for the quoting is almost that) or better yet supply a patch 
>> for consideration.
>
> Interesting, I actually posted a bug report on this at the start of 
> the week, then withdrew it a couple of hours later. I''m not
thinking
> that I should have let it stand.
>
> I see the issue as this. In SQL dialects that allow substitution into 
> commands, you are protected against sql injection. If you say
>
>     stmt = db.prepare("select * from people where id=?")
>     stmt.execute(@params[p_id"])
>
> it doesn''t matter what''s in the parameter. You expect an
integer, but
> if it contains the string "1; select * from passwords" it
won''t make a
> difference.
>
> In Rails, the documentation for criteria makes it look as if the same 
> is true. It isn''t. If I say
>
>    table.find_all([ "id=%d", @params{"p_id"]))
>
> then a malicious incoming string will indeed inject SQL into my 
> system. Rails _looks_ as if it gives protection, but in reality as a 
> developer you have to take responsibility for your own. You have to 
> know the quoting rules for your database, and make sure to take 
> account of them in each criteria you use.
>
> I''d very much like to see Rails move towards
>
>   table.find_all([ "id = ?", id] )
>
> and have it use the underlying database to do the substitution.
>
>
> Cheers
>
> Dave
>
> _______________________________________________
> Rails mailing list
> Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org
> http://lists.rubyonrails.org/mailman/listinfo/rails
>
-- 
Bruno Mattarollo <bmatt-ee4meeAH724@public.gmane.org>
Currently in: Sydney, Australia
[ http://pokies.typepad.com/virtual_possum/ ]


_______________________________________________
Rails mailing list
Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org
http://lists.rubyonrails.org/mailman/listinfo/rails

On Dec 3, 2004, at 16:21, Bruno Mattarollo wrote:
> I am not really well versed in AR or Rails (nor the database adapters 
> in Ruby) but using "bind variables" is usually the way to avoid
SQL
> injection.
Indeed - that''s exactly what I''m recommending.



Cheers

Dave

Another benefit of bind variables is that we can easily use dates in
our finders.  There are a couple of factors to correctly quoting a
bound variable:

1) The type of the column
2) The database adapter.

Now 2 is easy,  rails can just provide a series of default
''escape_x''
functions which the different adapters can override.

Correct me if I''m wrong but 1 is a little harder here.  As
ActiveRecord doesn''t appear to parse the SQL provided to it, AR
can''t
figure out the correct type of  the column,  it can only work on the
type of the variable provided to it.

Is this a potential security problem?   If it''s not a safe approach,
we could have JDBC style:

s.setDate(1, theDate);
s.setInteger(2, theInt);
s.setString(3, theString);

but I''d much prefer something simple like:

find(["event_date BETWEEN ? and ?", range.from, range.to]);

I could have a try at implementing this tonight (NZDT) if it seems safe.

On Fri, 3 Dec 2004 18:12:19 -0600, Dave Thomas
<dave-kbbdpT5sCmpWk0Htik3J/w@public.gmane.org>
wrote:
> 
> On Dec 3, 2004, at 16:21, Bruno Mattarollo wrote:
> 
> > I am not really well versed in AR or Rails (nor the database adapters
> > in Ruby) but using "bind variables" is usually the way to
avoid SQL
> > injection.
> 
> Indeed - that''s exactly what I''m recommending.
> 
> 
> 
> 
> Cheers
> 
> Dave
> 
> _______________________________________________
> Rails mailing list
> Rails-1W37MKcQCpIf0INCOvqR/iCwEArCW2h5@public.gmane.org
> http://lists.rubyonrails.org/mailman/listinfo/rails
> 

-- 
Cheers

Koz

On Friday 03 December 2004 09:00 pm, Michael Koziarski
wrote:
> Another benefit of bind variables is that we can easily use dates in
> our finders.  There are a couple of factors to correctly quoting a
> bound variable:
>
> 1) The type of the column
> 2) The database adapter.
>
> Now 2 is easy,  rails can just provide a series of default
''escape_x''
> functions which the different adapters can override.
>
> Correct me if I''m wrong but 1 is a little harder here.  As
> ActiveRecord doesn''t appear to parse the SQL provided to it, AR
can''t
> figure out the correct type of  the column,  it can only work on the
> type of the variable provided to it.
>
> Is this a potential security problem?   If it''s not a safe
approach,
> we could have JDBC style:
>
> s.setDate(1, theDate);
> s.setInteger(2, theInt);
> s.setString(3, theString);
>
> but I''d much prefer something simple like:
>
> find(["event_date BETWEEN ? and ?", range.from, range.to]);
The Ruby DBI library provides standard binding and quoting functions for the 
use of its drivers (which can override for specific behavior).  You can find 
the code in the sql.rb file in the DBI distribution.  Look for BasicQuote and 
BasicBind.

(I tried to respond earlier today, but it looks like my outgoing mail is 
having problems)

-- 
-- Jim Weirich    jim-Fxty1mrVU9GlFc2d6oM/ew@public.gmane.org    
http://onestepback.org
-----------------------------------------------------------------
"Beware of bugs in the above code; I have only proved it correct, 
not tried it." -- Donald Knuth (in a memo to Peter van Emde Boas)

> The code of interest is in the sql.rb file, modules BasicBind (for
> binding) and BasicQuote (for quoting).  Driver writer can override or
> replace these services if they are not appropriate for their particular
> database.
Sounds like a good idea. I''d certainly be willing to adopt a patch that
provided a method for using real bind variables. Preferably, it would 
coexist with the current sprintf formatting for backwards compatibility 
while taking over any ? for the binds.

Who''s up for this?
> You might not be worried about SQL injection problems, but I attended a
> security forum yesterday and saw a demo of SQL injection.  By typing 
> "'' OR
> 1=1" into the name field of a login screen, the user was able to log 
> in as
> a random user without using a password.  Granted, the demo was designed
> with this in mind, but it does point out the potential problems if we 
> are
> not careful.
This is indeed a serious risk. Currently, Active Record mitigates it 
through sprintf. But as Dave Thomas have just so passionately argued 
for, this can open a hole if you don''t realize that the
sprintf''ed
string is inserted directly into the SQL. Meaning that this is 
dangerous:

   Person.find_first(["firm_id = %s", firm_id ])

Note the missing quotes around the %s, which makes it possible to give 
"1 OR 1=1" as the firm_id. This will return the first user in the 
system, which is often the admin account. That wouldn''t be good. So you
need to always be sure to quote %s parts or use %d. So here are two 
safe alternatives:

   Person.find_first(["firm_id = ''%s''", firm_id ])
   Person.find_first(["firm_id = %d", firm_id ])

The last one will raise an ArgumentError if firm_id can''t be converted 
to an integer ("23" can, but "xcv" can''t), which is
reasonable enough,
but something that you need to keep in mind.
--
David Heinemeier Hansson,
http://www.basecamphq.com/   -- Web-based Project Management
http://www.rubyonrails.org/  -- Web-application framework for Ruby
http://macromates.com/       -- TextMate: Code and markup editor (OS X)
http://www.loudthinking.com/ -- Broadcasting Brain

On Monday 06 December 2004 16:44, David Heinemeier Hansson
wrote:
> So here are two safe alternatives:
>
>    Person.find_first(["firm_id = ''%s''", firm_id
])
>    Person.find_first(["firm_id = %d", firm_id ])
The first alternative still appears unsafe to my eyes. Consider, for 
example, the case where

   firm_id == "whatever'' OR ''1'' =
''1"

The %d alternative provides some safety, but a truly safe binding mechanism 
needs awareness of escaping semantics for the target flavour of SQL.

Best wishes to all,

-Steve

> The first alternative still appears unsafe to my eyes. Consider, for
> example, the case where
>
>    firm_id == "whatever'' OR ''1'' =
''1"
>
> The %d alternative provides some safety, but a truly safe binding 
> mechanism
> needs awareness of escaping semantics for the target flavour of SQL.
The sanitation that Active Record does on variables when using the [] 
form prevents this. ;, :, and '' are simple stripped so that it is not 
possible to break out of the original quotation. This makes it 
impossible to get your own SQL executed.
--
David Heinemeier Hansson,
http://www.basecamphq.com/   -- Web-based Project Management
http://www.rubyonrails.org/  -- Web-application framework for Ruby
http://macromates.com/       -- TextMate: Code and markup editor (OS X)
http://www.loudthinking.com/ -- Broadcasting Brain

On Monday 06 December 2004 19:49, David Heinemeier Hansson
wrote:
> >    firm_id == "whatever'' OR ''1'' =
''1"
>
> The sanitation that Active Record does on variables when using the []
> form prevents this. ;, :, and '' are simple stripped so that it is
not
> possible to break out of the original quotation. This makes it
> impossible to get your own SQL executed.
Aha; I wasn''t aware of the munging step.

-Steve

> Sounds like a good idea. I''d certainly be willing to adopt a patch
that
> provided a method for using real bind variables. Preferably, it would
> coexist with the current sprintf formatting for backwards compatibility
> while taking over any ? for the binds.
> 
> Who''s up for this?
I''m willing to have a go at this, though my code will probably be
filled with java-isms.

Just so I''m not wasting anyone''s time, is this the process I
should follow:

1) Update my application to run with Edge Rails
2) Edit the code in vendor
3) svn diff -u 
4) Attach to a trac ticket-thingy?


-- 
Cheers

Koz

On Tue, Dec 07, 2004 at 09:26:59AM +1300, Michael Koziarski
wrote:
> > Sounds like a good idea. I''d certainly be willing to adopt a
patch that
> > provided a method for using real bind variables. Preferably, it would
> > coexist with the current sprintf formatting for backwards
compatibility
> > while taking over any ? for the binds.
> > 
> > Who''s up for this?
> 
> I''m willing to have a go at this, though my code will probably be
> filled with java-isms.
> 
> Just so I''m not wasting anyone''s time, is this the
process I should follow:
> 
> 1) Update my application to run with Edge Rails
> 2) Edit the code in vendor
> 3) svn diff -u 
> 4) Attach to a trac ticket-thingy?
That sounds fine to me. Though I''d add:

2.5) Write tests
4.5) Attach tests to said trac ticket-thingy as well

Thanks!
marcel
-- 
Marcel Molina Jr. <marcel-WRrfy3IlpWYdnm+yROfE0A@public.gmane.org>

On Tue, Dec 07, 2004 at 05:46:10PM -0500, Marcel Molina Jr.
wrote:
> On Tue, Dec 07, 2004 at 09:26:59AM +1300, Michael Koziarski wrote:
> > > Sounds like a good idea. I''d certainly be willing to
adopt a patch that
> > > provided a method for using real bind variables. Preferably, it
would
> > > coexist with the current sprintf formatting for backwards
compatibility
> > > while taking over any ? for the binds.
> > > 
> > > Who''s up for this?
> > 
> > I''m willing to have a go at this, though my code will
probably be
> > filled with java-isms.
> > 
> > Just so I''m not wasting anyone''s time, is this the
process I should follow:
> > 
> > 1) Update my application to run with Edge Rails
> > 2) Edit the code in vendor
> > 3) svn diff -u 
> > 4) Attach to a trac ticket-thingy?
> 
> That sounds fine to me. Though I''d add:
> 
> 2.5) Write tests
> 4.5) Attach tests to said trac ticket-thingy as well
Oooops. I see you''ve already done both...sorry for the noise.

marcel
-- 
Marcel Molina Jr. <marcel-WRrfy3IlpWYdnm+yROfE0A@public.gmane.org>

On Thu, 2 Dec 2004, David Heinemeier Hansson wrote:
> Giving any language or framework the reputation of not dealing with
> SQL-injection is plain silly. As a developer, you''re always the
one
> where the buck stops.
Developers *will* make mistakes if given any opportunity to. That''s
human nature. A framework with a better approach to security will seek
to minimize the ways that developers can make security-related errors,
and minimize the effect of those errors when they are made.

So looking at the example you came up with:
> Meaning that this is dangerous:
>
>    Person.find_first(["firm_id = %s", firm_id ])
>
> Note the missing quotes around the %s, which makes it possible to give
> "1 OR 1=1" as the firm_id. This will return the first user in the
> system, which is often the admin account. That wouldn''t be good.
So you
> need to always be sure to quote %s parts or use %d.
I consider this an opportunity for programmers to make errors (leaving
out the quotes). If you replace the use of sprintf with the use of bind
variables, the programmer instead uses

    Person.find_first(["firm_id = ?", firm_id ])

and now it doesn''t matter what firm_id is; this will either work
securely or fail completely (assuming that the driver doesn''t have a
bug
in the binding code--and the driver author likely knows more than you
about the quoting conventions for his particular database).

In short, when I see error-prone styles of programming used consistently
throughout a system, it certainly makes me nervous about the security of it.

After that, here''s another exaple of something that really worries me:
> > The %d alternative provides some safety, but a truly safe binding
> > mechanism needs awareness of escaping semantics for the target
> > flavour of SQL.
>
> The sanitation that Active Record does on variables when using the
> [] form prevents this. ;, :, and '' are simple stripped so that it
is
> not possible to break out of the original quotation. This makes it
> impossible to get your own SQL executed.
You cannot make a claim that it is "not possible to break out of the
original quotation" except for databases that you know about. It could
well be that in some databases, perhaps ones for which there''s
currently
no rails support, that there are other escape sequences available that
would allow you break out of the quotation. This is why I believe it''s
extremely important to rely on the database-specific driver to deal
with quoting. Even if it''s impossible to break out of quoting for the
current databases (and are you really so sure that this is true for
PostgreSQL?), this is a time bomb waiting for someone to add support for
a database with syntax you''d not anticipated.

Anyway, I''m glad to see that you''re in favour of using bind
variables
instead of sprintf. However, what would really convince me that you''re
taking a serious approach to security would be to remove completely the
"auto-sprintf" feature in the code you presented above, causing this

    Person.find_first(["firm_id = %s", firm_id ])

to break. That removes the ability to make a certain class of error, at
the cost of backward compatability. What do you think?

cjs
-- 
Curt Sampson  <cjs-gHs2Wiolu3leoWH0uzbU5w@public.gmane.org>   +81 90 7737
2974   http://www.NetBSD.org
     Make up enjoying your city life...produced by BIC CAMERA

> Anyway, I''m glad to see that you''re in favour of using
bind variables
> instead of sprintf. However, what would really convince me that
you''re
> taking a serious approach to security would be to remove completely the
> "auto-sprintf" feature in the code you presented above, causing
this
> 
>     Person.find_first(["firm_id = %s", firm_id ])
> 
> to break. That removes the ability to make a certain class of error, at
> the cost of backward compatability. What do you think?
I think that removing it completely is excessive.  Active Record now
supports the following models:

Person.find_first(["firm_id = %s", firm_id ])
Person.find_first(["firm_id = ?", firm_id ])
Person.find_first(["firm_id = :firm_id", {:firm_id => firm_id }])

I''m confident that the escaping I wrote will work completely,  because
it delegates to the database connection. :)

Turning off sprintf  is too much,  though perhaps adding a logger.warn
is worth while?  And deprecating the original option in the documents?

-- 
Cheers

Koz

> Turning off sprintf  is too much,  though perhaps adding a logger.warn
> is worth while?  And deprecating the original option in the documents?
I fully agree. Turning off sprintf is indeed excessive and would 
unnecessarily burden the many applications already built on top of 
Rails that has managed to use statement interpolation without doing 
unquoted %s.

The new styles are already what''s featured in the documentation and 
people will surely upgrade to it over time. Deprecation is more than 
adequate.

Also, the sprintf method now delegates to the database quoting 
mechanism as well.
--
David Heinemeier Hansson,
http://www.basecamphq.com/   -- Web-based Project Management
http://www.rubyonrails.org/  -- Web-application framework for Ruby
http://macromates.com/       -- TextMate: Code and markup editor (OS X)
http://www.loudthinking.com/ -- Broadcasting Brain

On Thu, 9 Dec 2004, David Heinemeier Hansson wrote:
> I fully agree. Turning off sprintf is indeed excessive and would
> unnecessarily burden the many applications already built on top of
> Rails that has managed to use statement interpolation without doing
> unquoted %s.
It would also fix the ones that quoted it incorrectly. Once again,
we''re
at "leave in potential security holes because it would be inconvenient
to remove them."
> The new styles are already what''s featured in the documentation
and
> people will surely upgrade to it over time.
Why would they upgrade? Current users of that style may not even know
that there''s another option; they certainly have no reliable way of
finding all instances where it''s used.

cjs
-- 
Curt Sampson  <cjs-gHs2Wiolu3leoWH0uzbU5w@public.gmane.org>   +81 90 7737
2974   http://www.NetBSD.org
     Make up enjoying your city life...produced by BIC CAMERA