Hi Jeremy, Thanks for your prompt reply to my email. Some comments below.
On Fri, 2009-10-30 at 08:35 -0700, Jeremy Evans wrote: > On Oct 30, 5:51 am, ast <[email protected]> wrote: > > Hi folks, > > > > I've been playing with sequel now for a few months, and overall, I > > think it's great. I've seen a couple of minor issues that I've worked > > around (might post those separately), but my biggest concern relates > > to the way the parameter substitution is handled in the dataset class. > > > > I've looked at this in the code, and I must say I'm nervous about what > > sequel is doing when it is creating the queries to send to the > > database. Normally, the best defense for SQL injection is to use > > prepared statements, but one of the areas I've found the most "messy" > > is the sequel support for prepared statements. > > I agree completely that prepared statement support is messy. Sequel > was not designed to handle prepared statements when I took over > maintenance, so the support was basically hacked in by extending > dataset instances with modules that change the behavior. The code can > be difficult to follow and isn't very fun to work with. This is one > of the reasons that I only recommend using it if you have profiled > your code and found a bottleneck, and you've benchmarked the prepared > statement/bound variable approach to make sure it is faster. It wasn't really meant as a criticism. I'm aware you inherited the project, and I think you've done some really great things to get it where it is now. I've been developing SQL-based applications for about 15 years now, and I used to work for Informix, so I generally start application design from the schema, the queries and use prepared statements by default. Old habits die hard. :) I do generally avoid doing things inside the database with stored procedures, because I attempt to keep things SQL92 compliant so I can more easily support multiple database back-ends. If I do encounter some performance issues, I'll consider rolling it into an appropriate SP at that stage (but I haven't needed to do this for a long time) ;) For people who aren't comfortable with SQL, I completely agree that your approach is a good one. Premature optimisation of code is an easy trap to fall into. > > My application is such that I'm not really interested in the Model > > capabilities, so I'm using the dataset directly. However, because of > > my database schema, there are a number of multi-table queries that > > I've written using standard SQL92 syntax. It seems that it is > > impossible to use these as prepared statements in sequel. Is this > > really true? > > No, it's not true. You can use static SQL with placeholders for > prepared statements. It's a bit unintuitive, though: > > DB['SELECT * FROM table WHERE column = ?', :$n].prepare > (:select, :blah).call(:n=>1) > > If you were doing this a lot, I'd use the master branch for named > placeholder support (or wait until 3.6.0 is released next week), then > have a method that automatically scans the string for placeholders. > Basically, an API like: > > DB.prepare_query(:blah, :select, "SELECT * FROM table WHERE column > = :value") > > That does something like: > > DB.dataset.with_sql("SELECT * FROM table WHERE column > = :value", :value=>:$value).prepare(:select, :blah) > > Then you could call it easily: > > DB.call(:blah, :value=>1) I'm actually already doing something similar now, but not using the sequel library. Part of the application needs to generate dynamic SQL queries, sometimes across tables, and supplies the appropriate parameters as an array. I'm doing a regex search/replace to populate the parameters, because I wasn't aware of anything like this in sequel. It seems like the above approach might be a better solution for this problem. Thanks for the suggestion. To be fair, because I was doing the above is when I really started to get nervous. It felt (rightly) like I was duplicating things within the library, but didn't have a good way to directly prepare & invoke prepared statements. Mostly this was my bad because I was really only going from the examples in prepared_statements.rdoc, and they didn't include the SQL variant of creating a dataset. Since I was coming to the library cold, I didn't make the connection that you could actually just embed the SQL there as well instead of using the filter methods. > > > I'm using both SQLite3 and PostgreSQL for the database back-ends, and > > I believe that I'm using the ruby-pg implementation. > > > > I generally really like the syntax provided by the library, but the > > only way that I can directly execute my multi-table queries is to use > > the "raw SQL" mode of the dataset (which, thankfully, uses SQL92 > > standard prepared statement placeholders). However, since the library > > does string substitutions on the parameter values, I have the concern > > that my application is not nearly as robust & secure as it would be if > > I were using the "real" prepared statement support of the underlying > > database engine. > > If you are really concerned, you can certainly adopt the approach I've > laid out above. > > I'm comfortable with the approach Sequel uses. It generally uses the > native adapter quoting methods where available, so as long as the > database adapters are quoting things correctly, I don't think there > are any security issues. Now, if there are problems with the native > adapter quoting methods, Sequel may be vulnerable, but that's a bug in > the database adapter, not in Sequel itself. Of course, using prepared > statements/bound variables can eliminate that as a method of attack. > Yeah, and since the database server does this directly if it supports prepared statments, I'd much prefer to push it down to this layer rather than relying on client-side code (either sequel or the native library) quoting behavior. This has a whiff of a hack to me. I would much rather not send any kind of parameter substitution via quoting to the database at all, actually. Again, that's not intended as a reflection on what you're doing, but rather just the way that people seem to be allergic to using prepared statements. As I said, I use them by default because then you don't have to worry about SQL injection. It's one of the most common security issues in Web applications, and using prepared statements is the simplest and best way to avoid the issue entirely, as you mentioned. > > Fundamentally, I guess my question boils down to: how extensively has > > the library been tested against all known SQL injection techniques? > > > > Currently, I've about 1,325 lines of a ~15K LOC application which > > depend on sequel. Because I like the majority of the syntax, I'm > > hesitant to change to a different database library, but my security > > concerns currently outweigh my affinity for the library. > > When in doubt, test. The integration test suite does a simple test to > make sure that strings with potential SQL injection points (i.e. ') > are quoted correctly. If you think more extensive tests would be > beneficial, I'd be happy to review patches. While I take your point, that's a whole class of tests I'd prefer not to write. If I can avoid a known security issue by adopting coding conventions that will ensure that my code doesn't allow SQL injection because I can desk check that no generated SQL statements are sent to the server which don't use prepared statements, I'm comfortable not re-testing the database server's prepared statement functionality. I try very much to leverage white box testing as much as possible. If I don't understand (or can't get to) things that may not have as much exposure and code coverage in the wild, then, absolutely, your only recourse is to write the tests for it. > I don't know of any SQL injection techniques that work on Sequel, > assuming you don't allow the users to create arbitrary symbols (which > is a DoS vulnerability in ruby anyway). Obviously, if you do > something like: > > DB["SELECT * FROM table WHERE column = '#{params[:value]}'"] > > then you are vulnerable, but that's not a correct use of the library. Absolutely. There's no way that is happening in the code I have. > > Before this application gets much further, I'm going to need to > > finalize on the database back-end API. Currently, I'd prefer to use > > sequel vs. going back to DBI, but DBI has a much more "natural" API > > for working with prepared statements, so I'm tempted to revert back to > > it in the near future. > > Usage of prepared statements is definitely more natural with DBI, and > I used DBI, I'd probably use prepared statements. Since you are > writing all the queries by hand anyway, it's easy to do. Sequel was > not built with prepared statements/bound variables in mind, and one of > the benefits of using it is that you don't need to write all of SQL by > hand. > > Honestly, I'm proud that I was able to get to the current level of > support for prepared statements, given the constraints I had. I'm not > aware of another database library that abstracts as much as Sequel > does with integrated support for prepared statements/bound variables > in the abstraction layer (there may be one, I just don't know about > it). Such as: > > DB[:table].filter(:column1=>:$v1, :column2=>:$v2).call > (:select, :v1=>1, :v2=>2) As I said, I can appreciate the effort that went in to getting things where they are, and please don't take any of the above as personal comments. I think Sequel is a really neat database library, and I've both written from scratch and used many different libraries in C, C++, Objective-C, Java, Ruby, Python and C# on multiple platforms over the years. I think Sequel is a great balance between what the database can do and the expressiveness of Ruby. > > Apologies for the slightly stream-of-consciousness post, but I'd > > appreciate any comments or thoughts on the above to help me decide how > > to proceed with the application. > > If your primary concern is security, and you don't trust how Sequel > literalizes values, hopefully either using the prepared statement > approach so you don't have to trust Sequel's literalization, or > writing tests to increase your trust level will make you comfortable > using Sequel in production. Again, thanks for your comments, feedback and suggestions. It certainly gives me some options as the rest of the application comes together and gets more stable. Cheers, ast -- Andrew S. Townley <[email protected]> http://atownley.org --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "sequel-talk" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/sequel-talk?hl=en -~----------~----~----~----~------~----~------~--~---
