I have this situation in which I have to create a table, but the user defines the column names and column definitions.

If this is being done via an http request, I would structure the cgi query parameters for table columns as a list of related pairs: one member of each pair would be the "column name" (which needs to match a very simple regex) and the other would be the data type (which needs to come from a closed set of known alternatives).

How you structure the user's input form to support that is up to you -- the cleanest approach would probably involve some javascripting that allows the user to accumulate one or more column definitions, each of which adds two related parameters to the request (a type-in for the column name, a pull-down menu for the data type) -- e.g. the query might contain params sort of like this:

...colname_1=foo&coltype_1=int&colname_2=bar&coltype_2=varchar...
[download]

When it comes to structuring the server-side activity to handle the request, of course, you can't assume that every request will come from your nifty input form page, but all you have to do is grep out the paired parameters, make sure that you have a usable name and a known data type in each pair, and assemble your SQL "create table ..." statement accordingly.

That all assumes that the user requirements for creating tables do not involve "special features" like uniqueness constraints on one or more given columns, foreign-key relations to other tables, particular character-set and/or collation specs on varchars, and so on. (To some extent, some of these features could be included as options in a more elaborate input form.)

In any case, by structuring the input task so that each user-specified column involves two separate user inputs (name and data type), I think you'll be making things easier and safer overall, for the user and for the server.

(updated 2nd paragraph to be more coherent)

I think that quote_identifier avoids the injection problem by itself, but I'm not sure, and it could depend on the database used.

In my opinion the real question is: do you really need to allow users to use ANY valid name for a table/column? For it is certainly easier allowing only a subset of valid names (say /[_A-Z][_A-Z0-9]*/i) than trying to foresee any possible attack strategy.

You (or future readers) may also wish to peruse some of the information here:

• SQL Injection
• sqlmap: automatic SQL injection tool
• SQL Injection Cheat Sheet
• SQL Injection Attacks by Example
• SQL injection - Wikipedia

Update 12/03/2010: See also bobby-tables.com: A guide to preventing SQL injection, courtesy of Anonymous Monk

Also, most databases have a fine grained access system. If your table creating program is running as a webservice, give the connection only permission to create tables - and nothing else.

But parsing of types shouldn't be that hard - specially if you don't allow to set all kinds of constraints. I would just check for allowable types, and reject anything that doesn't look like a valid type definition (instead of trying to detect anything that may be a problem).

I think this could work, but I'm not sure; does it detect all possible attacks and doesn't it break legal SQL?
Any help would really be appreciated

Alternatively,You can force a (non-escaped) literal ';' to be the end of the statement under all circumstances.

As an aside, is there some reason that you want minimal constraints on what the user can do (especially if this is a production environment)? Use taint mode for input and validate that input for ;, followed by any of the SQL commands that you don't want the user to execute occurring anywhere in the line. That way (given that there is a very small set of commands) you can throw an exception (and log it when they occur.

UPDATE: Corrected for the real question :-(

I have this situation in which I have to create a table, but the user defines the column names and column definitions.
Anyway, I know now that the column name is handled by quote_identifier(..), but the definition, for example, a user might give me: VARCHAR(100) ); DROP DATABASE mysql; --'

DBI does the quoting for you. You should never use sprintf or other string formatting because the likeliness that your quoting is not sufficient is too big. Other people have solved that issue for you decades ago once and forever.

$dbh->do('CREATE TABLE test (a ?, b INT)', undef, $SQL);
[download]

If not, have a look at http://search.cpan.org/~timb/DBI-1.613/DBI.pm#quote Or consolidate your SQL driver for quoting functions if you don't use DBI (Dare you!).

A few points.
• Not every database(driver) allows place holders in just any place you want.
• You do not want to quote your types. Even simple types usually consist of several (lexical) tokens, for instance char(3), which has 4 tokens. Quoting that makes it one token, and not valid SQL.
• Why the Dare you! at the end? There can be many reasons to prefer more closely following the companies database's API instead of using a greatest-common-divisor strategy. I've done rapid prototyping, trying out solution using Perl, which then later got implemented in the company's main code repo, which was written in C. I wrote my solutions using sybperl, and DBlib/CTlib calls, because those are the calls the Sybase C-libraries provided. DBI would not have been a good choice.

Thats what I was looking for.
On the other hand, I just tried to execute a possible SQL-injected query

$dbh->do( 'CREATE TABLE a (x INT) ; DROP TABLE a -- );' ) ;
// or
$dbh->do( 'CREATE TABLE a (x INT) ; DROP TABLE a' ) ;
[download]

Both give an error. Is it a rookie SQL mistake or what. I'm confused now!!

Generally: Don't try to detect injections, avoid them. Restrict every input to a set of save characters, abort whenever you see invalid input. Don't try to repair invalid input, doing so often opens another way for injections.

Even more generally for any DBI application: Let DBI handle the quoting for you. Use prepared statements with placeholders, DBI will take care of proper quoting, should the database wire protocol require any quoting. Use the quote_identifier() method to quote any identifier (column names, table names). DBI might not contain code to handle your specific database requirements, but the DBD you are using sure does.

Specifically for your problem: Don't allow free text input. You know (or can find out) what types are supported by your database. Use something like a dropdown list or radio buttons to let the user select one of the available types. Some types need additional data, like VARCHAR or NUMBER. You know when to ask for that, it depends on the selected type. Use one or two additional input fields, and accept only numbers there. NULL / NOT NULL: Use a checkbox. The same for the primary key constraint. Unique constraints need a name and a list of existing column names. Also no problem, ask for a name and offer a multi-select for the columns. The same rules apply for other constructs found in CREATE TABLE statement, like foreign keys. Finally, assemble all of the information bits into a big CREATE TABLE statement. Bonus points for detecting missing primary keys.

You did not say anything about your environment, so I assumed web or GUI. In text mode, you have to ask a lot more questions than you do now, but the principle stays the same: Ask for one piece of information at a time, and validate the input.

Alexander

$type =~ s/\s+/ /g;  # Liberal for inputs, strict for outputs.
$type =~ s/^\s//;
$type =~ s/\s\z//;
$type = uc($type);

$type =~ /
   ^
   (?: VARCHAR (?: \s* \( \s* [0-9]+ \s* \) )?
   |   CHAR    (?: \s* \( \s* [0-9]+ \s* \) )?
   |   ...
   )
   \z
/x
   or die;
[download]

You could also accept two fields, one of them a drop down. The user wouldn't have to know SQL.

if ($type eq 'VARCHAR') {
   if ($arg eq '')             { $sql = $type; }
   elsif ($arg =~ /^[0-9]+\z/) { $sql = "$type($arg)"; }
   else { die }
}
elsif ($type eq 'CHAR') {
   if ($arg eq '')             { $sql = $type; }
   elsif ($arg =~ /^[0-9]+\z/) { $sql = "$type($arg)"; }
   else { die }
}
...
else {
   die
}
[download]

This means that there is no parsing of the input parameters (other than ensuring that they match with the parameter's datatypes), so any text that is passed in for a particular parameter can never be executed.

Another advantage (on large systems) is that the procs encapsulate all the SQL, making it a lot easier to find offending (badly performing) queries, and tuning them independently of the client code.

I realize that this isn't always practical, but once everyone in the team knows how this works it's actually quite efficient, in particular on large systems (200+ developers, several thousand tables, three million+ lines of SQL code...)

Michael