[AOLSERVER] nssqlite3 troubles
Hi all, I'm using the nssqlite3 driver for my latest experiment and found a serious limitation in it: you cannot execute a select statement that returns no rows. (at least not with select or 0or1row; you could probably hack it with exec but that would be annoying and is supposed to be unnecessary) The problem is straightforward - the driver checks how many rows are returned after executing a statement; 0 returns NS_DML and 0 returns NS_ROWS. 'ns_db select' and '0or1row' expect NS_ROWS results, even if there are no rows to be read (which is a perfectly valid result). So how to fix it? There's no direct way to tell what kind of statement was parsed in the sqlite api (unlike say, OCI) and checking how many rows were changed will give bad results for an update affecting no rows. The dumb thing to do would be to compare the start of the passed sql with select to determine if it is a select or dml statement. But there may be a cleverer way! sqlite allows you to set up an authorization callback that is called when parsing the sql. Among other things this callback can allow or deny operations like select, insert, delete, drop, and so forth. That makes it easy, just create an authorizer to allow select (and read) and deny everything else, and use that for select operations only, and for dml or exec leave the authorizer null to allow everything. The nsdb driver allows for a different selectProc and execProc, so this should be straightforward. Fortunately I read the code before implementing this, for had I not I would surely have slammed my head into the wall several times trying to figure out what was going wrong. nsdb does allow you to register a different selectProc and execProc, and one would think that a select operation would use the more specific function if it was available, and fall back to the generic one if not. One would be mistaken; if an execProc is registered it will be used and the selectProc is only used when there is no execProc. Not only is this backward IMHO, it thwarts a potentially useful protection against sql-injection attacks - if the select method really only let the database execute select statements then adding in a drop table or other nastiness would be ineffectual. However, even if this protection wasn't thwarted at the nsdb level, the database-specific drivers that exist are no help. As far as I can tell, even when a separate selectProc is provided is it just a wrapper around execProc, meaning they all execute whatever sql is passed in and only use the different initial call to check what is returned and raise an error if it is wrong. I gather that these different interfaces were not intended as a security measure, but as a programmer convenience. So after all this I guess I'll shortly be writing a patch for this driver which checks if the statement begins with select and forces it to return NS_ROWS for that case. To whom should I send it? (Or Dossy could grant me cvs access.) And then there's nsdb - I think the more specific selectProc should be tried first for select operations, but since it's been this way for a while, would changing this break some other drivers (where the selectProc has never been called, or tested)? The postgres driver is at least aware of this judging by a comment that the select function is never called by the server, but how would the other drivers fare? -J -- AOLserver - http://www.aolserver.com/ To Remove yourself from this list, simply send an email to [EMAIL PROTECTED] with the body of SIGNOFF AOLSERVER in the email message. You can leave the Subject: field of your email blank.
Re: [AOLSERVER] nssqlite3 troubles
On Feb 7, 2008 9:32 AM, Jeff Rogers [EMAIL PROTECTED] wrote:
The problem is straightforward - the driver checks how many rows are
returned after executing a statement; 0 returns NS_DML and 0 returns
NS_ROWS. 'ns_db select' and '0or1row' expect NS_ROWS results, even if
there are no rows to be read (which is a perfectly valid result).
So how to fix it? There's no direct way to tell what kind of statement
was parsed in the sqlite api (unlike say, OCI) and checking how many
rows were changed will give bad results for an update affecting no rows.
sqlite3_stmt *st;
sqlite3_prepare_v2(handle, sql, length, st, tail)
if (sqlite3_column_count(st) == 0) {
/* DML */
}
--
AOLserver - http://www.aolserver.com/
To Remove yourself from this list, simply send an email to [EMAIL PROTECTED]
with the
body of SIGNOFF AOLSERVER in the email message. You can leave the Subject:
field of your email blank.
Re: [AOLSERVER] nssqlite3 troubles
And then there's nsdb - I think the more specific selectProc should be tried first for select operations, but since it's been this way for a while, would changing this break some other drivers (where the selectProc has never been called, or tested)? The postgres driver is at least aware of this judging by a comment that the select function is never called by the server, but how would the other drivers fare? The select proc is only in the pg driver in order to support some ancient AOLserver 2 functionality that I doubt anyone else uses any more. It shouldn't appear in your sqllite3 driver, IMO. I suggest you implement stephen deasy's straightforward check that differentiates between queries that return rows (i.e. SELECT queries but usig SQL Lite's parser) to differentiate between NS_ROWS and NS_DML queries. As far as security goes, no one should allow for the direct execution of external SQL anyway, not even a SELECT. If someone's code breaks because they execute a DROP TABLE statement sent to their site via a query string or whatever, there's not much reason to have sympathy for them. -- AOLserver - http://www.aolserver.com/ To Remove yourself from this list, simply send an email to [EMAIL PROTECTED] with the body of SIGNOFF AOLSERVER in the email message. You can leave the Subject: field of your email blank.
Re: [AOLSERVER] nssqlite3 troubles
Stephen Deasey wrote:
sqlite3_stmt *st;
sqlite3_prepare_v2(handle, sql, length, st, tail)
if (sqlite3_column_count(st) == 0) {
/* DML */
}
Ah, that's much cleaner! I guess late at night I overlooked the
sqlite3_column* set of functions. Using sqlite3_column_name could be a
better approach for DbBindRow also, rather than relying on the per-row
callback being called. Although it may not matter much - does anyone
care about what columns are returned if there are no rows?
-J
--
AOLserver - http://www.aolserver.com/
To Remove yourself from this list, simply send an email to [EMAIL PROTECTED]
with the
body of SIGNOFF AOLSERVER in the email message. You can leave the Subject:
field of your email blank.
Re: [AOLSERVER] nssqlite3 troubles
[EMAIL PROTECTED] wrote: The select proc is only in the pg driver in order to support some ancient AOLserver 2 functionality that I doubt anyone else uses any more. Then should it be removed? (in someone's copious free time...) Only issue would be legacy sites that may use it. Of course, they could continue with their existing version of the driver, and they likely don't exist anyway? It shouldn't appear in your sqllite3 driver, IMO. Could you expand on this? Aren't any legacy AOL2 SQLLite sites, I imagine :) SQLLite's not that ancient, is it You've got way more experience with the db drivers than I do so I'm inclined to take your advice, but I'm curious why you think a unified exec is better than separate select/dml/generic functions. I haven't looked at the oracle db driver forever, but obviously the PG one doesn't differentiate between the two. You call PQExec or whatever it's called, and the library tells you what kind of query it executed. Aside from that, do you also think the generic function should be preferred over the specific function if both are defined? Let's put it this way - I know of no problems that have arisen from it being this way. OK, took a teensy bit of cleverness to get the SQLLite3 driver to work right for selects that return no rows, but not much. Yes, everyone should check their inputs to avoid this, but things sometimes slip through. Executing a query from an external source isn't simply a matter of checking your inputs, it's more like checking your brain into the asylum. It's just not in the same class as smuggling in an AND clause due to someone not checking that an integer's an integer or the like. Your select statements should say SELECT, and once you do that, no DROP or DELETE is going to happen no matter what someone does to attempt to smuggle in SQL. One of my few gripes about the ns_db interface is that you can only pass raw sql instead of being able to use bind variables. Check out the bindvar emulation for the pg driver in OpenACS mode. It puts a dead stop to sql smuggling because it quotes the values. WHERE foo = :var becomes WHERE foo = '123' which, in PG, works as well as an unquoted 123. But if var is something like 123 or 't' you get WHERE foo = '123 or ''t''' which is not what was hoped for by the bad guy. -- AOLserver - http://www.aolserver.com/ To Remove yourself from this list, simply send an email to [EMAIL PROTECTED] with the body of SIGNOFF AOLSERVER in the email message. You can leave the Subject: field of your email blank.
Re: [AOLSERVER] nssqlite3 troubles
On Thursday 07 February 2008 11:07, Jeff Rogers wrote:
[EMAIL PROTECTED] wrote:
The select proc is only in the pg driver in order to support some ancient
AOLserver 2 functionality that I doubt anyone else uses any more.
Then should it be removed? (in someone's copious free time...)
It shouldn't appear in your sqllite3 driver, IMO.
Could you expand on this? You've got way more experience with the db
drivers than I do so I'm inclined to take your advice, but I'm curious
why you think a unified exec is better than separate select/dml/generic
functions.
The ns_db API allows you to use ns_db exec exclusively, you don't need the
other API. If you use ns_db exec, you can provide error detection and
recovery closer to the application layer instead of embedding it in the
driver. Here is a simple example:
set ExecCode [ns_db exec $dbHandle $PreparedQuery]
set ReturnList [list]
set Level [incr level]
switch -exact -- $ExecCode {
NS_DML {
set ReturnList [list -1]
}
NS_ROWS {
set RowSet [ns_db bindrow $dbHandle]
set Rows 0
while {[ns_db getrow $dbHandle $RowSet]} {
incr Rows
set Array ${arrayTemplate}${Rows}
upvar $Level $Array A${Rows}
if {![info exists Cols]} {
set Cols [ns_set size $RowSet]
}
for {set i 0} {$i $Cols} {incr i} {
set A${Rows}([ns_set key $RowSet $i]) [ns_set value $RowSet
$i]
}
lappend ReturnList $Array
}
}
}
return $ReturnList
If an error occurs the whole thing errors out, but the caller can handle
certain things like ns_db 1row, when the row can only appear once, but is
missing, in other words, if you use ns_db 0or1row, you would still have to
check which you get, and with 1row you would have to catch the error. So
there is no benefit for these specialized select API. (They may be a little
more clear for those reading the code what you are trying to do, but that is
about it.)
tom jackson
Aside from that, do you also think the generic function should be
preferred over the specific function if both are defined?
I suggest you implement stephen deasy's straightforward check that
differentiates between queries that return rows (i.e. SELECT queries but
usig SQL Lite's parser) to differentiate between NS_ROWS and NS_DML
queries.
As far as security goes, no one should allow for the direct execution of
external SQL anyway, not even a SELECT. If someone's code breaks because
they execute a DROP TABLE statement sent to their site via a query
string or whatever, there's not much reason to have sympathy for them.
Yes, everyone should check their inputs to avoid this, but things
sometimes slip through. What I worry about is a high-profile
sql-injection vulnerability being discovered leading to manager-esque
types saying AOLserver is inherently insecure because of how it handles
database queries, so we can't use it. (It hasn't slowed down php tho.)
One of my few gripes about the ns_db interface is that you can only pass
raw sql instead of being able to use bind variables. It makes the
correct code more verbose and the simple code insecure. I wish I could
write my queries as
ns_db select $db select * from users where id = ? $user
instead of
ns_db select $db select * from users where id = [ns_dbquotevalue $user]
Yes, it's a straightforward wrapper function to do that (although you
wouldn't get the performance benefits from a stored prepared statement
that are theoretically possible) but having it as core functionality
would be nicer.
-J
--
AOLserver - http://www.aolserver.com/
To Remove yourself from this list, simply send an email to
[EMAIL PROTECTED] with the body of SIGNOFF AOLSERVER in the
email message. You can leave the Subject: field of your email blank.
--
AOLserver - http://www.aolserver.com/
To Remove yourself from this list, simply send an email to [EMAIL PROTECTED]
with the
body of SIGNOFF AOLSERVER in the email message. You can leave the Subject:
field of your email blank.
Re: [AOLSERVER] nssqlite3 troubles
Using sqlite3_column_name could be a better approach for DbBindRow also, rather than relying on the per-row callback being called. Although it may not matter much - does anyone care about what columns are returned if there are no rows? Well, should be faster, since the column names are invariant for any given rowset, i.e. you only have to compute them once, rather than recompute them for each row. -- AOLserver - http://www.aolserver.com/ To Remove yourself from this list, simply send an email to [EMAIL PROTECTED] with the body of SIGNOFF AOLSERVER in the email message. You can leave the Subject: field of your email blank.
Re: [AOLSERVER] nssqlite3 troubles
[EMAIL PROTECTED] wrote: The select proc is only in the pg driver in order to support some ancient AOLserver 2 functionality that I doubt anyone else uses any more. Then should it be removed? (in someone's copious free time...) It shouldn't appear in your sqllite3 driver, IMO. Could you expand on this? You've got way more experience with the db drivers than I do so I'm inclined to take your advice, but I'm curious why you think a unified exec is better than separate select/dml/generic functions. Aside from that, do you also think the generic function should be preferred over the specific function if both are defined? I suggest you implement stephen deasy's straightforward check that differentiates between queries that return rows (i.e. SELECT queries but usig SQL Lite's parser) to differentiate between NS_ROWS and NS_DML queries. As far as security goes, no one should allow for the direct execution of external SQL anyway, not even a SELECT. If someone's code breaks because they execute a DROP TABLE statement sent to their site via a query string or whatever, there's not much reason to have sympathy for them. Yes, everyone should check their inputs to avoid this, but things sometimes slip through. What I worry about is a high-profile sql-injection vulnerability being discovered leading to manager-esque types saying AOLserver is inherently insecure because of how it handles database queries, so we can't use it. (It hasn't slowed down php tho.) One of my few gripes about the ns_db interface is that you can only pass raw sql instead of being able to use bind variables. It makes the correct code more verbose and the simple code insecure. I wish I could write my queries as ns_db select $db select * from users where id = ? $user instead of ns_db select $db select * from users where id = [ns_dbquotevalue $user] Yes, it's a straightforward wrapper function to do that (although you wouldn't get the performance benefits from a stored prepared statement that are theoretically possible) but having it as core functionality would be nicer. -J -- AOLserver - http://www.aolserver.com/ To Remove yourself from this list, simply send an email to [EMAIL PROTECTED] with the body of SIGNOFF AOLSERVER in the email message. You can leave the Subject: field of your email blank.
