current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

I posted this question here.

And an answer stated that I should not do:

$table_name = 'survey_'.$_POST['surveyid'];

because

It is easy for a hacker to exploit your site if you include $_GET or $_POST data directly in any SQL string.

Here is the code. Do you see any security exploits?

if(ctype_digit($_POST['surveyid']) && $_POST['surveyid']>0){

    $table_name = 'survey_'.$_POST['surveyid'];

    $query = 'CREATE TABLE '.$table_name.' (
            `responseid` INT NOT NULL AUTO_INCREMENT,
            `textarea1` TEXT NULL,
            `textarea2` TEXT NULL,
            `textarea3` VARCHAR(255) NULL,
            `drop_down1` VARCHAR(255) NULL,
            `drop_down2` VARCHAR(255) NULL,
            `bool1` BIT NULL,
            `bool2` BIT NULL,
        PRIMARY KEY (`responseid`))';
}

I don't see a vulnerability.... why is $_POST['surveyid'] vulnerable? It is being sanitized by ctype_digit...

share|improve this question
    
A small note: now your code is okay, but when you grow it, with more variables between that if and the assignation of $table_name, it might become a real problem forgetting why the code is there and if it's validated. I'd suggest doing the validation in the same line as the assignation. –  Francisco Presencia yesterday
add comment

2 Answers

up vote 12 down vote accepted

Since you validate that $_POST['surveyid'] contains at least one digit and contains only digits, your query is safe.

However, the CREATE TABLE operation that you are trying to do strikes me as a horrible thing to do. CREATE TABLE is a Data Definition Language operation, and DDL commands should be executed only in special situations.

Basically, if you routinely create a new table to store responses from each survey, your database schema will be an unmaintainable mess. I strongly recommend that you post your database schema and describe what you are trying to do in a question to http://dba.stackexchange.com to develop a sane schema that does not require new tables to be created routinely.

share|improve this answer
2  
Especially when the table name is a string of digits. Talk about a non-indicative name! –  Clockwork-Muse yesterday
add comment
up vote 4 down vote

  1. Usually what you need is a suvery_id attribute and usage of only one table:

    CREATE TABLE 'survey_result' (
    `response_id` INT NOT NULL AUTO_INCREMENT,
    `survey_id` INT NOT NULL,
    `textarea1` TEXT NULL,
    `textarea2` TEXT NULL,
    `textarea3` VARCHAR(255) NULL,
    `drop_down1` VARCHAR(255) NULL,
    `drop_down2` VARCHAR(255) NULL,
    `bool1` BIT NULL,
    `bool2` BIT NULL,
PRIMARY KEY (`responseid`))';

    I suppose you'll need proper indexes too (based on your queries).

  2. Note that I renamed responseid to response_id since this format is easier to read (especially if the name contains three or more words).

  3. Having indexed column names, like

    `textarea1` TEXT NULL,
`textarea2` TEXT NULL,
`textarea3` VARCHAR(255) NULL,

    does not suggest flexible database schema. Number of input boxes on surveys are likely to change. Here is another design approach on Stack Overflow: Schema design for when users can define fields, but you can find others, search for survey system database schema or something similar. Knowledge about database normalization also useful.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.