1

I do not think that this has been posted before - as this is a very specific problem.

I have a script that generates a "create table" script with a custom number of columns with custom types and names.

Here is a sample that should give you enough to work from -

$cols = array();
$count = 1;
$numcols = $_POST['cols'];
while ($numcols > 0) {

    $cols[] = mysql_real_escape_string($_POST[$count."_name"])." ".mysql_real_escape_string($_POST[$count."_type"]);
    $count ++;
    $numcols --;
}
$allcols = null;
$newcounter = $_POST['cols'];
foreach ($cols as $col) { 
    if ($newcounter > 1)
        $allcols = $allcols.$col.",\n";
    else
        $allcols = $allcols.$col."\n";
    $newcounter --;
};
$fullname = $_SESSION['user_id']."_".mysql_real_escape_string($_POST['name']);
$dbname = mysql_real_escape_string($_POST['name']);
$query = "CREATE TABLE ".$fullname." (\n".$allcols." )";
mysql_query($query);
echo create_table($query, $fullname, $dbname, $actualcols);

But for some reason, when I run this query, it returns a syntax error in MySQL. This is probably to do with line breaks, but I can't figure it out. HELP!

Alfo
  • 4,801
  • 9
  • 38
  • 51
  • Which part returns the error? What was the error? Did you debug `echo $query;`? Post these things with your question and you'll receive better help. – Jason McCreary Sep 24 '11 at 01:47
  • If you feel like the line breaks are a problem why don't you remove them? – Tarek Fadel Sep 24 '11 at 04:06
  • `mysql_real_escape_string` doesn't work with dynamic table/column names. See this question on how to make this code secure: http://stackoverflow.com/questions/5811834/how-to-prevent-sql-injection-with-dynamic-tablenames – Johan Sep 24 '11 at 05:11

1 Answers1

3

You have multiple SQL-injection holes
mysql_real_escape_string() only works for values, not for anything else.
Also you are using it wrong, you need to quote your values aka parameters in single quotes.

$normal_query = "SELECT col1 FROM table1 WHERE col2 = '$escaped_var' ";

If you don't mysql_real_escape_string() will not work and you will get syntax errors as a bonus.
In a CREATE statement there are no parameters, so escaping makes no sense and serves no purpose.

You need to whitelist your column names because this code does absolutely nothing to protect you.

Coding horror

$dbname = mysql_real_escape_string($_POST['name']); //unsafe

see this question for answers:
How to prevent SQL injection with dynamic tablenames?

Never use \n in a query
Use separate the elements using spaces. MySQL is perfectly happy to accept your query as one long string.
If you want to pretty-print your query, use two spaces in place of \n and replace a double space by a linebreak in the code that displays the query on the screen.

More SQL-injection
$SESSION['user_id'] is not secure, you suggest you convert that into an integer and then feed it into the query. Because you cannot check it against a whitelist and escaping tablenames is pointless.

$safesession_id = intval($SESSION['user_id']);  

Surround all table and column names in backticks `
This is not needed for handwritten code, but for autogenerated code it is essential.

Example:

CREATE TABLE `table_18993` (`id` INTEGER .....

Learn from the master
You can generate the create statement of a table in MySQL using the following MySQL query:

SHOW CREATE TABLE tblname;

Your code needs to replicate the output of this statement exactly.

Community
  • 1
  • 1
Johan
  • 74,508
  • 24
  • 191
  • 319