There are other potential security risks, though. For example if you use an ORM mapper (like DBIx::Class or Rose::DB) and construct a complicated query, you have to know exactly which arguments are parsed as SQL and which aren't.

I am not sure I understand this comment. As far as I know DBIx::Class uses placeholders internally so you don't have to worry about things like this. As for the "you have to know exactly which arguments are parsed as SQL" part of the comment, it treats *all* of its arguments as perl values and not SQL values unless you use the options to pass in a raw SQL string, in which case its your responsibility to make sure it doesnt open you up to risks. I can't speak for Rose::DB because I don't use it, but I would be shocked if the author didn't take all this stuff into account too.

But if you really stick to plain DBI with placeholders you don't have to worry very much about SQL injection.

Yes, you also have to manage your own SQL -> Object conversion, and write a LOT more code, and more code == more bugs. Modules like DBIx::Class and Rose::DB have been pretty thoroughly tested by lots of users in serious production applications (I can count 4 we have here at $work), if they didn't handle simple security stuff like this correctly then they would have been dismissed as crap long ago.

As for the "you have to know exactly which arguments are parsed as SQL" part of the comment, it treats *all* of its arguments as perl values and not SQL values unless you use the options to pass in a raw SQL string, in which case its your responsibility to make sure it doesnt open you up to risks. I can't speak for Rose::DB because I don't use it, but I would be shocked if the author didn't take all this stuff into account too.

Your instincts were right :)

Rose::DB::Object (note: not Rose::DB, which is a db abstraction layer used by the ORM, Rose::DB::Object) uses bind values everywhere, except in cases where the DBD::* driver requires that values be "inlined." There is no ambiguity in these cases as the values that are allowed to be are specific for each column type/database combination.

For example, Informix supports a syntax like this:

SELECT * FROM t1 WHERE somedate > CURRENT;

where "somedate" is a DATE column type. Unfortunately, DBD::Informix chokes on this:

$sth = $dbh->prepare('SELECT * FROM t1 WHERE somedate = ?');
$sth->execute('CURRENT'); # Boom: error!
[download]

because it considers the above the equivalent of comparing somedate to the string "CURRENT", as if it were:

SELECT * FROM t1 WHERE somedate > 'CURRENT';

Then the database itself throws an error since it refuses to compare a DATE column to a string. (Other databases are more lenient about this kind of thing.)

Anyway, the upshot is that, if you want to use CURRENT (or any other "special" server-evaluated value), you must inline it directly into the query passed to DBI.

In Rose::DB::Object, such values are called "keywords" and are automatically inlined. So if you add this clause to a Rose::DB::Object::Manager query:

somedate => { gt => 'CURRENT' },

it'll be smart enough to realize that a) "somedate" is a DATE column and b) "CURRENT" is a valid keyword for DATE columns in Informix, so it'll inline the value instead of using a bind parameter.

Again, these lists of keywords are specific to each column-type/database combination, so the word "CURRENT" would not be inlined if you were connected to a MySQL database, for example.

Also note the lack of ambiguity: it's clear that there's only one reasonable meaning of CURRENT when used as a comparison value for a DATE column. IOW, there's never a reason for it not to be inlined in this case, so the auto-inlining is never a hindrance.

Finally, you can always explicitly force any value to be taken as a literal and inlined by passing it as a scalar reference:

somecol => { gt => \q(iknowwhatimdoing) },

That'll produce SQL like this, with no "?" placeholders, quoting, or bind values:

somecol > iknowwhatimdoing

Obviously, you'd only do this in very specific cases when you're sure what you're doing is safe :)

If you treat your database as a dumb object store I bet my supadances against your socks that you are using the database very very inefficiently, bogging it down with unoptimized (and unoptimizable) ad-hoc queries, fetching many times more data that you actually need etc.

All I want from my database access layer is to let me call the stored procedures (few of them one statement only) without much fuss, thank you very much.

Jenda


Support Denmark! Defend the free world!

If you treat your database as a dumb object store I bet my supadances against your socks that you are using the database very very inefficiently, bogging it down with unoptimized (and unoptimizable) ad-hoc queries, fetching many times more data that you actually need etc.

1. How I use my database is my business, and in some of my business use cases it makes more sense to treat it like a dumb object store. For that I use DBIx::Class.
2. Why the f*ck would I want your old dancing shoes anyway?

Now, before you make assumptions, I suggest you actually take a look at the DBIx::Class code and the SQL queries it does generate (you can use DBIx::Class::QueryLog for that). To start with DBIx::Class allows you an extremely high degree of control over when and how much of the database it queries. Second, the many contributors to the project have made sure that the SQL it does generate is both optimized and smart (after all these people are not idiots they know how databases work very well).

All I want from my database access layer is to let me call the stored procedures (few of them one statement only) without much fuss, thank you very much.

If that is all you want then why do you care what I do? I am not going to tell you that what you doing is inefficient and stupid, cause I assume that you are smart enough to do that is most appropriate for your business use case. Remember, this is Perl, the land of TIMTOWTDI, you do it your way and I will do it mine. But please, please, please don't talk shit about stuff you have no clue about (*cough* DBIx::Class *cough*).

-stvn

Nice work (although the outcome is not unexpected ;).

Thanks -- I was hoping to see the result that I did actually see, but I was very worried. I've always assumed that placeholders are safe. With the uncertainty, I did what any engineer or scientist would do -- I conducted an experiment to find out what was really happing.

The modules like DBIxC and Rose sound nice, and I will try DBIxC again -- ten years ago I was putting raw HTML into my Perl CGIs, and that's a no-no for me now. Perhaps in another ten years there won't be any SQL in my modules because I'm using DBIxC. Or maybe I'll reach that state of Nirvana sooner. I'll see.

But if you really stick to plain DBI with placeholders you don't have to worry very much about SQL injection.

Yeah. :)

but I was very worried

About a single quote character causing a problem? Wow, you must think that the DBD modules are almost completely untested?

Though I don't see how you jump from preventing one trivial SQL injection attack to the grand conclusion that all injection attacks will surely be prevented. But placeholders aren't exactly rocket surgery so I'd be rather surprised if somebody could find character strings that are not properly handled by a DBD (ignoring character encoding problems which are still quite a nuisance), much less allowing SQL to be injected.

But I've certainly found plenty of problems with DBD placeholders. None of them were the type that would allow for SQL injection. Most of them were in various versions of DBD::ODBC and include such things as not being able to handle dates without jumping through extra hoops of complexity (or just not being able to use dates via placeholders at all) and more vague problems of queries just not matching properly until I replaced the placeholders with simple string templates using DBD's ->quote(). There were also some problems with DBD::mysql and quotes around numerical values in some situations.

Update: Also consider that your audience's FUD may be related to PHP which, from what I've heard, does some rather unreliable "magic" trying to automatically deal with single quotes in data to/from databases without making a clear distinction between the quoted strings and the (unquoted) string values and that this "magic" doesn't work very well.

- tye        

About a single quote character causing a problem? Wow, you must think that the DBD modules are almost completely untested?

No -- I'm confident that the DBD modules are tested against this specific challenge.

Anyway, just to be sure, I expanded my script ..

Read more... (2 kB)

And I still got good results ..

Read more... (3 kB)

So it's not guaranteed that these three DBDs are 100% protected, but a few obvious tests show that I'm fairly confident that the three DBDs that I care about are OK.

Alex / talexb / Toronto

"Groklaw is the open-source mentality applied to legal research" ~ Linus Torvalds