Rails - Dec 2004 - Bind Variables in Active Record

OK,  I have some basic functionality to support bind variables,  it
appears to work with the ''old'' %s style too.

I''ve altered sanitize_conditions in
activerecord/lib/active_record/base.rb to check whether bind variables
are in the statement (/\?/).  If they are, replace all the ?s to
escaped values from the arguments array.  else santize and expand.

There are a few gotchas with this simple approach:

1) ? in statements not meant for binding

None of the SQL dialects I''ve worked with use ?,  is this a fair
assumption?  Is there a better test for
''bind_variables_present''?

2) Mixing ? and %s is not supported i.e  find_all(["id=? and
first_name like ''%s''", 3
"''''''''bob''''''\''?"])

Is there a legitimate case for this kind of query?  

The next question is,  how to allow each database adapter to override
the quoting rules for each data type.   The current implementation has
a series of methods like this

def quote_string(value)
  value = value.gsub(/''/, "''''")
  "''#{value}''"
end

def quote_date(value)
  "''#{value.to_s}''"
end

etc.

What''s the most logical place for this code to live?  Mysql will have
to override quoting rules for '', and most databases will have to
provide customer formatting for Dates and Times.

The current santize method lives in ActiveRecord::Base right?  Is that
the right place for these ''quoters''?  All the DB specific
stuff
appears to live in ''connection''.  Should it go in there?

My lack of  exposure to the code, and lack of ruby exposure generally,
makes me think I''m missing some really easy option....

I''ll create a ticket & send a patch when I get my app working with
edge rails, hopefully tonight, but I''d appreciate any feedback ahead
of time.

-- 
Cheers

Koz

* Michael Koziarski (koziarski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org) [041206
19:45]:
> 2) Mixing ? and %s is not supported i.e  find_all(["id=? and
> first_name like ''%s''", 3
"''''''''bob''''''\''?"])
> 
> Is there a legitimate case for this kind of query?  
If we''re voting I''m skeptical that there would be a legitimate
case for
this, and it may get confusing later to try to support simultaneous use
of both different escapings (especially from underneath where the
individual database adapters are concerned).  If it has to be done it''s
not overly difficult, the only trick being to make sure the expansions
are done independently.  In a pinch one possible mechanism would be to
convert %-expansions into ''?'' bindings:

  find_all(["id=? and foo=%s and bar=%d", id, foo, bar]

  =>

  find_all(["id=? and foo=? and bar=?", id, foo, bar]

and then perform only one real type of expansion in the underlying
method.

Rick
-- 
 http://www.rickbradley.com    MUPRN: 453
                       |  etc for both the booth
   random email haiku  |  and theatre. At this point, I
                       |  would say, go for it.

> 2) Mixing ? and %s is not supported i.e  find_all(["id=? and
> first_name like ''%s''", 3
"''''''''bob''''''\''?"])
>
> Is there a legitimate case for this kind of query?
I''d agree with the other comment, I don''t think this sort of
style should
be encouraged.  I''d vote for converting it into "?"s as the
other posted
suggested at least as a transitionary measure, and deprecating it if
possible.

While we''re on the topic, I''d like to sneak in a feature
request and get
some general feedback.  Could something like this be easily added:

find_all("name = :name AND job = :job", :name => "Bob
Dole", :job =>
"Comedian")

I got to liking this style at my day job, so I thought I''d throw it out
there to see if anyone else is interested.  Not a big deal obviously, but
I find it more readable and it prevents me from messing up the order of
the bind variables.

Regards,
-- 
Dave Steinberg
http://www.geekisp.com/

I also agree that the old style can just be transformed into ? by way
of a simple regexp or similar. Once the prepared statement is in
activerecord i''m sure you will see it everywhere as recommendet style
and the printf style will disappear quickly. Its one of those rare
cases where the new style has only advantages over the old style
unless i''m missing something.

On Mon, 6 Dec 2004 23:08:39 -0500 (EST), Dave Steinberg
<dave-xviZR3pNp+Yxsqv6Oivclw@public.gmane.org>
wrote:
> > 2) Mixing ? and %s is not supported i.e  find_all(["id=? and
> > first_name like ''%s''", 3
"''''''''bob''''''\''?"])
> >
> > Is there a legitimate case for this kind of query?
> 
> I''d agree with the other comment, I don''t think this sort
of style should
> be encouraged.  I''d vote for converting it into "?"s as
the other posted
> suggested at least as a transitionary measure, and deprecating it if
> possible.

-- 
Tobi
http://blog.leetsoft.com

On Dec 6, 2004, at 18:34, Michael Koziarski wrote:
> OK,  I have some basic functionality to support bind variables,  it
> appears to work with the ''old'' %s style too.
>
> I''ve altered sanitize_conditions in
> activerecord/lib/active_record/base.rb to check whether bind variables
> are in the statement (/\?/).  If they are, replace all the ?s to
> escaped values from the arguments array.  else santize and expand.
Maybe I''m misunderstanding--in what way is this different to using %s 
everywhere?



Cheers

Dave

?  + a date yields ''2004-12-07''
%s + a date yields 2004-12-07

i.e. the adapters know whether the variable requires quotes,  what
kind of quotes,  and how to format the resulting string.

rather than  just blindly expanding %s


On Tue, 7 Dec 2004 00:47:24 -0600, Dave Thomas
<pragdave-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
> 
> On Dec 6, 2004, at 18:34, Michael Koziarski wrote:
> 
> > OK,  I have some basic functionality to support bind variables,  it
> > appears to work with the ''old'' %s style too.
> >
> > I''ve altered sanitize_conditions in
> > activerecord/lib/active_record/base.rb to check whether bind variables
> > are in the statement (/\?/).  If they are, replace all the ?s to
> > escaped values from the arguments array.  else santize and expand.
> 
> Maybe I''m misunderstanding--in what way is this different to using
%s
> everywhere?
> 
> Cheers
> 
> Dave
> 
> 

-- 
Cheers

Koz

Here''s the simplistic implementation to bind parameters to ? in SQL
statements.  Here are my thoughts:

Letting the adapter take care of quoting increases the security of the
SQL statements,  bad values get escaped correctly, irrespective of
what the user does.

We can''t stick with using %s and just call AbstractAdapter::quote on
the supplied values as it will break existing code.  "first_name
''%s''" will get double quoted.

Implementing the requested :variable_name functionality should be a
relatively simple matter if the quoting & pattern matching approach is
sound.

See http://dev.rubyonrails.org/trac.cgi/ticket/278



-- 
Cheers

Koz


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

> Here''s the simplistic implementation to bind parameters to ? in
SQL
> statements.
I''m digging it. Very simple, very nice. Please do go right ahead with 
the alternative expansion 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

The bind-style interpolation patch has been applied and the 
documentation has been updated to use that in the example. The 
CHANGELOG reads:

* Added bind-style variable interpolation for the condition arrays that 
uses the adapter''s quote method [Michael Koziarski]

   Before:
     find_first([ "user_name = ''%s'' AND password =
''%s''", user_name,
password ])]
     find_first([ "firm_id = %s", firm_id ])] # unsafe!

   After:
     find_first([ "user_name = ? AND password = ?", user_name,
password
])]
     find_first([ "firm_id = ?", firm_id ])]

Thanks for all who raised concerns on the old interpolation style. And 
many thanks to Michael for being so quick to do an implementation of 
this. What a great turn-around on this issue.

The beta gems already include this feature.
--
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 7, 2004, at 4:41, David Heinemeier Hansson wrote:
> Thanks for all who raised concerns on the old interpolation style. And 
> many thanks to Michael for being so quick to do an implementation of 
> this. What a great turn-around on this issue.
As a concern raiser, let me echo that--this is a great community.

Of course, now I have to go back and redo all my examples... :(

So, for our next trick, what do people think about changing the Ruby 
type corresponding to decimal(m,n) columns from Float to BigDecimal?


Cheers

Dave

On Dec 7, 2004, at 14:01, Dave Thomas wrote:
> Of course, now I have to go back and redo all my examples... :(
But examples of what, dear Dave? (the plot thickens! :))
--
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 7, 2004, at 5:01 AM, Dave Thomas wrote:
>
> On Dec 7, 2004, at 4:41, David Heinemeier Hansson wrote:
>
>> Thanks for all who raised concerns on the old interpolation style. 
>> And many thanks to Michael for being so quick to do an implementation 
>> of this. What a great turn-around on this issue.
>
> As a concern raiser, let me echo that--this is a great community.
>
> Of course, now I have to go back and redo all my examples... :(
>
> So, for our next trick, what do people think about changing the Ruby 
> type corresponding to decimal(m,n) columns from Float to BigDecimal?
I agree. I remember being surprised it wasn''t that way already.

Best, Eric

>> So, for our next trick, what do people think about changing the Ruby 
>> type corresponding to decimal(m,n) columns from Float to BigDecimal?
>
> I agree. I remember being surprised it wasn''t that way already.
Sounds like you''ve signed up for that patch, Eric :)? I''ll
certainly
accept it.
--
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 Tue, 7 Dec 2004, Michael Koziarski wrote:
> Letting the adapter take care of quoting increases the security of the
> SQL statements...
I may be missing something in the terminology here, but isn''t the
adapter a rails thing that comes between rails and the database driver?

If so, you''re again duplicating the quoting stuff, since the database
driver already does correct quoting, or some sort of magic (depending on
the wire protocol, it could be that quoting is not necessary) to make
sure that nothing in a value filling in a ''?'' is ever
interpreted as
something other than part of that parameter''s value.

It''s my opinion that, for best security, not to mention avoiding
writing
unnecessary code, nothing in rails should deal with any sort of quoting
whatsoever; it should all be left to the database driver by handing it
statements with ''?'' and values to be filled in there.

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

> It''s my opinion that, for best security, not to mention avoiding 
> writing
> unnecessary code, nothing in rails should deal with any sort of quoting
> whatsoever; it should all be left to the database driver by handing it
> statements with ''?'' and values to be filled in there.
The adapter is merely delegating the call, like:

       def quote_string(s)
         Mysql::quote(s)
       end

BTW, I''m sensing an antagonistic tone in your emails that I feel is 
creating unnecessary tension in our debates. Am I misinterpreting your 
writing? Email, as other writing mediums, are terribly bad at conveying 
proper emotions, so I thought I''d better validate my perception against
your intentions.
--
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, 2004-12-09 at 16:14 +0100, David Heinemeier Hansson
wrote:
> > It''s my opinion that, for best security, not to mention
avoiding
> > writing
> > unnecessary code, nothing in rails should deal with any sort of
quoting
> > whatsoever; it should all be left to the database driver by handing it
> > statements with ''?'' and values to be filled in
there.
> 
> The adapter is merely delegating the call, like:
> 
>        def quote_string(s)
>          Mysql::quote(s)
>        end
> 
> BTW, I''m sensing an antagonistic tone in your emails that I feel
is
> creating unnecessary tension in our debates. Am I misinterpreting your 
> writing? Email, as other writing mediums, are terribly bad at conveying 
> proper emotions, so I thought I''d better validate my perception
against
> your intentions.
While I haven''t dug into any of the code, when I see people using
''?'' in
a query, I expect the low level(probably C) driver to do any work
necessary. 

The idea being that you send the query to the backend to be
compiled/prepared on those backends that support it. Then you come back
around with a list of variables to fill the spots in the SQL query. The
benefit for security is that you don''t really need any quoting as there
shouldn''t be any string parsing going on anymore at the point of
attaching the variables to the query. 

-- 
Steven Critchfield <critch-wQLwMjUOumVBDgjK7y7TUQ@public.gmane.org>

> The idea being that you send the query to the backend to be
> compiled/prepared on those backends that support it. Then you come back
> around with a list of variables to fill the spots in the SQL query. The
> benefit for security is that you don''t really need any quoting as
there
> shouldn''t be any string parsing going on anymore at the point of
> attaching the variables to the query.
That would be a good next step. We are already using the database''s 
quoting mechanism, but prepared statements would be a nice addition for 
speed as well. As its done now, it should also be possible on a 
per-adapter basis to add this behavior. So we can move towards prepared 
statements as the implementations trickle in.
--
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, 2004-12-09 at 16:14 +0100, David Heinemeier Hansson wrote:
> > > It''s my opinion that, for best security, not to mention
avoiding
> > > writing
> > > unnecessary code, nothing in rails should deal with any sort of
quoting
> > > whatsoever; it should all be left to the database driver by
handing it
> > > statements with ''?'' and values to be filled in
there.
> >
> > The adapter is merely delegating the call, like:
> >
> >        def quote_string(s)
> >          Mysql::quote(s)
> >        end
Yes, but my complaint is about to whom it''s being delegated. Is that
MySQL delegation actually going to the code from the MySQL folks? How
about the PostgreSQL one? The vendors know much better than us how to
quote, or even if quoting is necessary. As I said before, it may be
the case that there is no quoting done when you use a ''?''
becuase it''s
translated into something different in the wire protocol.

That''s just the security issue, of course. There are other benefits
as well. For example, even if you''re not using prepared statements,
Microsoft SQL server will cache query plans, so if you do a lot of the
same query with just different values passed along for the ''?''
bits, the
query need be compiled only the first time you use it.

On Thu, 9 Dec 2004, Steven Critchfield wrote:
> The idea being that you send the query to the backend to be
> compiled/prepared on those backends that support it. Then you come back
> around with a list of variables to fill the spots in the SQL query. The
> benefit for security is that you don''t really need any quoting as
there
> shouldn''t be any string parsing going on anymore at the point of
> attaching the variables to the query.
>
> While I haven''t dug into any of the code, when I see people using
''?'' in
> a query, I expect the low level(probably C) driver to do any work
> necessary.
This is exactly what I''m hoping for: to use this mechanism always in
place of doing any textual substitution on the string.
> On Thu, 2004-12-09 at 16:14 +0100, David Heinemeier Hansson wrote:
>
> > BTW, I''m sensing an antagonistic tone in your emails that I
feel is
> > creating unnecessary tension in our debates.
That tone appears to me to be there, too, and I apologise for that. I
was a bit tired and frustrated yesterday, due to other things. I don''t
want to appear antagonistic (well, at least not too antagonistic :-)).
I''ll try to keep it under better control from now on.

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

On Tue, 7 Dec 2004 07:01:35 -0600, Dave Thomas
<pragdave-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Of course, now I have to go back and redo all my examples... :(
Ahh, glad to see you''re writing a rails book, or at least a something
with a big chapter on Rails... :-). (You are, right?).
> 
> So, for our next trick, what do people think about changing the Ruby
> type corresponding to decimal(m,n) columns from Float to BigDecimal?
I made a comment about this recently. I really, really, really want
this type to be "Decimal(m,n)". If I have a type representing
currency, for example, then I''d like to be able to do, e.g. averages,
and have them render themselves as reasonable, rounded numbers, with
two digits to the right of the decimal! And I want to be able to
associate validation methods, input methods, and rendering methods
with the type.

That is, I want an extensible, open type system for ActiveRecord basic types.

I suppose I should put my money where my mouth is, and propose
something concrete...

Bob

> This is exactly what I''m hoping for: to use this mechanism always
in
> place of doing any textual substitution on the string.
For databases which support it,  I definitely agree that delegating to
some kind of prepared statement is the best bet.  However the current
implementation is exactly what several popular JDBC and Perl DBI
drivers use.  While it''s true that using the database is a better
guarantee of security,  in the meantime (and for drivers without this
functionality) the best idea is to provide lots of pathological unit
test cases.

The more tests. the merrier the database code will be.

-- 
Cheers

Koz

Curt Sampson wrote:
> That''s just the security issue, of course. There are other
benefits
> as well. For example, even if you''re not using prepared
statements,
> Microsoft SQL server will cache query plans, so if you do a lot of the
> same query with just different values passed along for the
''?'' bits, the
> query need be compiled only the first time you use it.
Agreed.  MySQL supports caching of the query parsing, and Oracle and
Postgre support caching of the query parsing and query execution plan,
so it makes sense from a performance perspective to use bind variables
wherever possible.  I don''t agree with a couple of the posts saying
Rails should not allow the sprintf version though, just make it more
prominent to use the bind variables format.

On Fri, 10 Dec 2004, Sean Leach wrote:
> I don''t agree with a couple of the posts saying
> Rails should not allow the sprintf version though, just make it more
> prominent to use the bind variables format.
So what is the benefit of leaving the sprintf stuff in place? The
benefits of taking it out (after making sure that ? format is used
everywhere internally, of course) are:

    1. It''s a relatively cheap way of getting users to go through and
    fix security-sensitive code that is easy to get wrong.

    2. It more strongly discourages the use of security-sensitive code.

    3. For those cases where the user really does want to use sprintf,
    it''s trivial to work around. (Just call sprintf directly!)

    4. Users unwittingly working from bad examples will find that they
    don''t work, and then learn the right way to do things.

Doing this sooner, rather than later, minimizes the pain of the
transition.

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

>> I don''t agree with a couple of the posts saying
>> Rails should not allow the sprintf version though, just make it more
>> prominent to use the bind variables format.
>
> So what is the benefit of leaving the sprintf stuff in place? The
> benefits of taking it out (after making sure that ? format is used
> everywhere internally, of course) are:
The benefit is minimizing the headache of breaking backwards 
compatibility on an issue that everyone I queried on #rubyonrails a few 
days ago got "right". So it''s a trade off. Have enough people
gotten
this wrong currently in their applications to make it reasonable to 
cause breakage for the people who got it right? I don''t think it is.

The sprintf method is now deprecated. The examples in the Active Record 
documentation showcases the new system. I appreciate your eager in 
pushing for this direction and I note your concern, but unless 
overwhelming evidence mounts that lots of bad examples or broken code 
exists out there, this discussion is now closed.

I will, however, happily print an article of guidance on this issue to 
go along with the rest of the porting information for Rails 0.9 from 
you. Would that be of interest?
--
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 Mon, 2004-12-13 at 16:33 +0100, David Heinemeier Hansson
wrote:
> >> I don''t agree with a couple of the posts saying
> >> Rails should not allow the sprintf version though, just make it
more
> >> prominent to use the bind variables format.
> >
> > So what is the benefit of leaving the sprintf stuff in place? The
> > benefits of taking it out (after making sure that ? format is used
> > everywhere internally, of course) are:
> 
> The benefit is minimizing the headache of breaking backwards 
> compatibility on an issue that everyone I queried on #rubyonrails a few 
> days ago got "right". So it''s a trade off. Have enough
people gotten
> this wrong currently in their applications to make it reasonable to 
> cause breakage for the people who got it right? I don''t think it
is.
> 
> The sprintf method is now deprecated. The examples in the Active Record 
> documentation showcases the new system. I appreciate your eager in 
> pushing for this direction and I note your concern, but unless 
> overwhelming evidence mounts that lots of bad examples or broken code 
> exists out there, this discussion is now closed.
Odd question here. Why do you think the old method has to be supported
in the current way? Wouldn''t it be just as convenient to detect the
percent signs and then invoke a regex to convert to the question marks
so we can move to bind variables in a backward compatible way? 

I appreciate the concern for backwards compatibility as I am trying to
use some software that I didn''t code and have enjoyed the upgrade
process so far(debian problems aside).

The one thing I haven''t seen discussed yet and I think may be a bigger
hurdle is how many of the currently supported databases support bind
variables in a sane enough way to even bother for a forward push on
this. I haven''t really looked into it much before. I seem to remember
previous perl and postgres issues where you could use bind variables but
the perl driver having issues with prepared statements. Of course this
is from very hazy memory, but it should be a very big concern on whether
or not to bother making changes.
-- 
Steven Critchfield <critch-wQLwMjUOumVBDgjK7y7TUQ@public.gmane.org>

> Odd question here. Why do you think the old method has to be supported
> in the current way? Wouldn''t it be just as convenient to detect
the
> percent signs and then invoke a regex to convert to the question marks
> so we can move to bind variables in a backward compatible way?
Oh, this is a great idea! I''d certainly accept a patch that did this 
with a good number of unit tests backing up the claim. We could have 
our pie and eat it too ;)
--
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 Mon, 2004-12-13 at 20:18 +0100, David Heinemeier Hansson
wrote:
> > Odd question here. Why do you think the old method has to be supported
> > in the current way? Wouldn''t it be just as convenient to
detect the
> > percent signs and then invoke a regex to convert to the question marks
> > so we can move to bind variables in a backward compatible way?
> 
> Oh, this is a great idea! I''d certainly accept a patch that did
this
> with a good number of unit tests backing up the claim. We could have 
> our pie and eat it too ;)
On further thinking of this, I have one concern about the approach I
suggested. What would happen if someone was using special sprintf
formatting to deal with something like decimal places. 

The option that came to mind that might make this work would be to
detect the sprintf formats and replace with a question mark while in the
background actually applying the sprintf format to the variable being
used. 

-- 
Steven Critchfield <critch-wQLwMjUOumVBDgjK7y7TUQ@public.gmane.org>

> The option that came to mind that might make this work would be to
> detect the sprintf formats and replace with a question mark while in 
> the
> background actually applying the sprintf format to the variable being
> used.
Or just have the replacement work on %s in isolation. As soon as you''re
doing %d, or something else, you''re out of harms way, security wise. So
it''s only the unadorned %s that''s a problem.
--
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 further thinking of this, I have one concern about the approach I
> suggested. What would happen if someone was using special sprintf
> formatting to deal with something like decimal places.
>
> The option that came to mind that might make this work would be to
> detect the sprintf formats and replace with a question mark while in the
> background actually applying the sprintf format to the variable being
> used.
The other thing to consider when replacing,  is that you could
actually make the problem worse by adding quotes where they''re not
needed  What do these strings resolve to with "'' or 1=1 -- "
as the
value?

first_name = ''%s''  => first_name =
'''''' or 1 = 1 --''
first_name = %s => first_name = '''' or 1 = 1 --
first_name like ''%%s''  first_name like
''%''''or 1 = 1 --''

etc.

-- 
Cheers

Koz

On Dec 13, 2004, at 15:49, David Heinemeier Hansson wrote:
> Or just have the replacement work on %s in isolation. As soon as 
> you''re doing %d, or something else, you''re out of harms
way, security
> wise.
Unless the error message contains a semicolon... :)

Cheers

Dave

* David Heinemeier Hansson (david-OiTZALl8rpK0mm7Ywyx6yg@public.gmane.org)
[041213 16:51]:
> >The option that came to mind that might make this work would be to
> >detect the sprintf formats and replace with a question mark while in 
> >the
> >background actually applying the sprintf format to the variable being
> >used.
> 
> Or just have the replacement work on %s in isolation. As soon as
you''re
> doing %d, or something else, you''re out of harms way, security
wise. So
> it''s only the unadorned %s that''s a problem.
It might be worth noting somewhere, as an additional reason people might
want to move away from the deprecated %-notation, that if individual
database handlers start doing compilation of queries for efficiency,
that non-''%s'' %-specifiers will effectively be recompiled each
time
since the non-''%s'' specifiers don''t merge down to
bind markers (''?''),
and so the queries vary with the values supplied at execution time.

Of course, I think not converting ''%d'' and friends to
''?'' is a feature
since it multiplies the reasons to move to the new ''?'' binding
notation.


On a slightly different note (having seen the issue of how to parse a
query with quoting in it raising its head already), before the
''%s''->''?''
quoting discrimination problem becomes too hairy (i.e., noone wants to
take the time to untangle the meanings of the various quoting and
escaping conventions) it''s probably worth noting that sprintf itself
includes the interpretation machinery sufficient to sort this out.  The
trick is that we can have ''%d'', ''%f'', etc.,
interspersed with the ''%s'',
among various bits of quotery.


Here is a hack which perhaps (hopefully) can be streamlined but which
solves the problem of converting %-specifiers to ''?'' specifier
without
having to reinvent the sprintf parser.  I had to resort to defining a
wrapper class because my ruby-fu is not strong enough to make this work
directly on the values themselves:

class QueryCleanser
  def initialize(value, capture)
    @value = value
        @capture = capture
  end
  def to_i; @value.to_i; end
  def to_f; @value.to_f; end
  def to_s
        @capture << @value
        ''?''
  end
end
 
def convert_query(query, *args)
  newargs = []
  wrapped = []
  args.each { |x| wrapped << QueryCleanser.new(x, newargs) }
  [ sprintf(query, *wrapped), newargs ]
end


----

irb> convert_query("foo ''%s'''', %d, %2.3f
%s", 1, 2, 3, 4)
=> ["foo ''?'''', 2, 3.000 ?", [1, 4]]
irb> convert_query("select * from table where a=''%s''
and b < %d and c > %d and abs(d-%f) < 0.01 and x like
''%%%s''", "bob", 3, 5, 24.24, "wins")
=> ["select * from table where a=''?'' and b < 3 and
c > 5 and abs(d-24.240000) < 0.01 and x like ''%?''",
["bob", "wins"]]

(please forgive any line-wrap)

Of course, this won''t work with combinations of %-specifiers and
''?''-specifiers for placeholders, but that shouldn''t
be a big issue.

Rick
-- 
 http://www.rickbradley.com    MUPRN: 703
                       |  obtain a PC
   random email haiku  |  and NICs, this sounds like pretty
                       |  much of a black box.

On Mon, 13 Dec 2004, Steven Critchfield wrote:
> Odd question here. Why do you think the old method has to be supported
> in the current way? Wouldn''t it be just as convenient to detect
the
> percent signs and then invoke a regex to convert to the question marks
> so we can move to bind variables in a backward compatible way?
I don''t actually see how we can do this without potentially breaking
stuff. Here are some examples:

    "SELECT * FROM member_%s WHERE email_address =
''%s''", "secondary", "a@b.c"

this turns into

    "SELECT * FROM member_secondary WHERE email_address = ?",
"a@b.c"

Note that we needed to do an actual subsitution of the (we hope
untainted) string for the first %s, but not for the second. Now here''s
a case which is correct within the current system, and one where a
mistake has been made:

    "SELECT * FROM member WHERE email_address =
''%s''", "a@b.c"
    "SELECT * FROM member WHERE email_address = %s", "a@b.c"
    "SELECT * FROM member WHERE member_id = %s", 17

Here are the results we want:

    "SELECT * FROM member WHERE email_address = ?", "a@b.c"
    "SELECT * FROM member WHERE email_address = ?", "a@b.c"

In the first, correct case, we have single quotes around the "%s" that
need to be removed. In the second case, the quotes have (incorrectly)
been forgotten, though this may work with some databases. But we again
want it without quotes. But now let''s look at this one:

    "SELECT * FROM member WHERE email_address LIKE
''%s@docomo.ne.jp''", "wxy"

this needs to be replaced with

    "SELECT * FROM member WHERE email_address LIKE ?", "wxy"
+ "@docomo.ne.jp"

Sprintf is an extremely general mechanism, which you can''t easily
emulate and almost certainly can''t safely emulate. If you''re
not going
to leave it just as it is, you''re going to run the risk of breaking
something. (Not that I object to this at all--at least there''d be some
chance of catching some security problems.)

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