Will this prevent SQL Injections?

Question

UPDATE 1:

I get the following error:

Fatal error: Cannot pass parameter 2 by reference in /var/www/page1.php on line 42 Call Stack: 0.0008 341836 1. {main}() /var/www/page1.php:0

When using:

if( $_SERVER['REQUEST_METHOD'] == 'POST') {
    $fields = array(
        'col1'  => 'cb1',
        'col2'  => 'cb2',
        'col3'  => 'cb3',
        'col4'  => 'cb4',
    );
    $parts = array();
    foreach($fields as $dbfield => $field)
        $parts[] = '`' . $dbfield . '` = :' . $dbfield;
    $dbh = new PDO('mysql:host=localhost;dbname=database', 'user', 'pass');
    $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $sth = $dbh->prepare('UPDATE `table1` SET ' . join(', ', $parts) . ' WHERE `id `= :id');
    // temp simulation value
    $id = 1;
    $sth->bindParam(':id', $id, PDO::PARAM_INT, 4);
    foreach($fields as $dbfield => $field)
        $sth->bindParam(':' . $dbfield, isset($_POST[$field]) ? 1 : 0, PDO::PARAM_INT, 1);
    $sth->execute();
}

ORIGINAL QUESTION:

Will the following code prevent SQL injections?

<?php
    if( $_SERVER['REQUEST_METHOD'] == 'POST' ) {
        // all the way upto 50
        $fields = array('col1'=>'cb1', 'col2'=>'cb2', 'col3'=>'cb3', 'col4'=>'cb4');

        $update = '';

        foreach($fields as $dbfield => $field) {
            if ($update) $update.= ',';

            $update.= ' '.$dbfield.'=';

            $update .= isset($_POST[$field]) ? 1 : 0;
        }

        $DBH = new PDO( "mysql:host=localhost;dbname=database", "user", "pass" );
        $DBH -> setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

        $STH = $DBH -> prepare( "update table1 set " . $update . " where id = :id" );

        // temp simulation value
        $id = 1;

        $STH -> bindParam( ':id', $id, PDO::PARAM_INT, 4 );

        $STH -> execute();
    }
?>

<html>
    <head>
        <title></title>
    </head>

    <body>
        <form method="post">
            <input type="checkbox" name="cb1" />
            <input type="checkbox" name="cb2" />
            <input type="checkbox" name="cb3" />
            <input type="checkbox" name="cb4" />
            <!-- all the way to 50 -->

            <input type="submit" value="submit" />
        </form>
    </body>
</html>

Will the code **prevent SQL injections**? Like, all over the world? Probably not. Is it succeptible to SQL injection attacks? Also probably not, since the only foreign data that goes in your query is the `id` value which is passed via a prepared statement. — Kerrek SB, Jul 19 '11 at 18:43
@Kerrek, the point is that it took me a while to clear my head and grok the code. — Johan, Jul 19 '11 at 19:47

chaos · Accepted Answer · 2011-07-20T19:30:38.643

3

Well, in your particular case, SQL injection cannot go through this code because you're only constructing your update out of pre-known column names, 1s and 0s. The overall way you're constructing that update would be a very dangerous practice to repeat in other contexts, though. The thing is, you're using PDO, but then forcing it to send a raw string with no escaping of your data, which defeats everything PDO can do to try to protect you.

In order to actually use what PDO does for you against SQL injection, you need to write the query with ? or named placeholders for data and use bindParam to supply the values.

An example of dynamic construction using binding of named parameters might look like:

if( $_SERVER['REQUEST_METHOD'] == 'POST') {
    $fields = array(
        'col1'  => 'cb1',
        'col2'  => 'cb2',
        'col3'  => 'cb3',
        'col4'  => 'cb4',
    );
    $parts = array();
    foreach($fields as $dbfield => $field)
        $parts[] = '`' . $dbfield . '` = :' . $dbfield;
    $dbh = new PDO('mysql:host=localhost;dbname=database', 'user', 'pass');
    $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $sth = $dbh->prepare('UPDATE `table1` SET ' . join(', ', $parts) . ' WHERE `id `= :id');
    // temp simulation value
    $id = 1;
    $sth->bindParam(':id', $id, PDO::PARAM_INT, 4);
    foreach($fields as $dbfield => $field) {
        $value = isset($_POST[$field]) ? 1 : 0;
        $sth->bindParam(':' . $dbfield, $value, PDO::PARAM_INT, 1);
    }
    $sth->execute();
}

edited Jul 20 '11 at 19:30

answered Jul 19 '11 at 18:42

chaos

122,029
33
303
309

Do I need to start another question to get an answer on how to do this properly, or can I continue here now that I know this is not secure? – oshirowanen Jul 19 '11 at 18:43
3

@chaos: While the code may be not the finest work, the code only has whitelisted column names, and either 1 or 0 as a value. SO your argument is false. The correct answer is: yes, there's no SQL injection, but you shouldn't use prepared statements this way. – Wrikken Jul 19 '11 at 18:45
@Wrikken: Yeah, I missed the 0/1 bit on my first pass. I've revised the answer to explain that it's safe here but thinking that means the practice would generalize safely would be a bad mistake. – chaos Jul 19 '11 at 18:48
@chaos, I have tried to do this properly using PDO parameters properly, but I am not sure how to achieve this? – oshirowanen Jul 19 '11 at 19:02
@oshirowanen: I've expanded my answer to include an example of dynamically constructing the query and binding the values using named parameters. – chaos Jul 19 '11 at 19:09
@chaos, I've tried your code, but it's giving me an error. Please see update 1 in the original question. – oshirowanen Jul 20 '11 at 18:15
@oshirowanen: Sorry, I forgot about the data argument to `bindParam()` being pass-by-ref. I've revised it; current version should work. – chaos Jul 20 '11 at 19:31
I've tried your example by changing bindParam to bindValue, but it does not work. – oshirowanen Jul 20 '11 at 20:22
@oshirowanen: Don't do that. :) Try the current version above. Notice that I changed the code around the `bindParam` usage. – chaos Jul 20 '11 at 21:12

score 1 · Answer 2 · answered Jul 19 '11 at 18:44

1

Since you aren't actually inserting foreign data into your table at all, I'd say you are pretty safe. (You should read others' responses though. I can't think of every possibility.)

However, I can't tell what all is a placeholder in your code for your simplified example. If you do choose to use actual user data, then you should start using PDO properly. As of now, you only have one placeholder. All of your fields don't have placeholders. You have effectively defeated the purpose of using a prepared query in the first place.

If you could describe what you are trying to do, perhaps we can suggest a cleaner solution.

answered Jul 19 '11 at 18:44

Brad

159,648
54
349
530

no, he commented on your code claiming some raw post was getting in there, but chaos has since seen the error of his ways and deleted it. Will delete mine too in that case. – Wrikken Jul 19 '11 at 18:48
@Brad, basically, I have a "settings" page which allows users to select which options they want to enable or disable. Looks like I will end up with lots of checkboxes. That is basically it, wanting to insert 0's or 1's into a database depending on which options are selected. How can I do this properly with PDO? – oshirowanen Jul 19 '11 at 19:01

score -1 · Answer 3 · edited May 23 '17 at 12:11

This is dynamic SQL:

 $STH = $DBH -> prepare( "update table1 set " . $update . " where id = :id" ).

Dynamic SQL cannot be made secure against SQL-injection except by checking the input $update against a whitelist of approved values.

See here for more info: How to prevent SQL injection with dynamic tablenames?

Is your code secure? (yes)
Your code inits a whitelist of values in array fields and injects a 1 if input matches and a 0 if it doesn't.
I see no SQL-injection opportunity there, but it does look like a code smell.

Suggestion to clear the code smell
Personally I'd restructure the table into:

Simple_settings
---------------
new_id integer auto_increment primary key 
user_id integer  <<-- optional can be used to check if a user is allowed to alter
                      the setting.
page_id integer  <<-- replaces your id 
settingname varchar(20)
onoff boolean

Now you can update your table using plain PDO:

UPDATE table1 onoff = :isfieldset WHERE settingname = :dbfield 
AND page_id = :id

If you have multiple settings to do you'll have to do them in a loop, but at least your code will be simple and your table will not have a zillion columns.
As an added bonus you don't need to change your table layout if the number of settings changes.
Note that if an evil users injects its own data into :dbfield the update will fail.
If you use this to insert, the rest of your code will not read that setting so you should be ok there as well.

One problem remains
If you have invalided checkboxes (because the current user is not allowed to change all settings) your code currently does not check for that, if there is some business logic, you will need to check that this user is allowed to enter those settings. You can do this in a before update trigger on the settings_table or use a (prior/added) select against a user_allowed_settings table to see which user is allowed to set what setting.

@Anomie have updated the answer to correct my mistake and explain my feelings regarding the code smell that lingers in my nose. — Johan, Jul 19 '11 at 19:46

score -2 · Answer 4 · answered Jul 19 '11 at 18:43

-2

No, because even though you're using parameterized queries/bind variables, the $update variable is still being formed from unsanitized user input.

answered Jul 19 '11 at 18:43

Peter

6,354
1
31
29

2

Where do you see this? The `$update` variable is formed purely from local data. It *depends* on user input, but its *value* is entirely deterministic. – Kerrek SB Jul 19 '11 at 18:45
@Kerrek, you conclude based on incomplete data. – Johan Jul 19 '11 at 18:50
2

@Johan: While the posted code could easily be changed to be unsafe, as posted `$update` is fine. – Anomie Jul 19 '11 at 18:52

Will this prevent SQL Injections?

4 Answers4