Is this PDO database connection/ insertion secure from injection?

ageattack · September 18, 2014

<?php

if(isset($_POST['submit'])){

$uname = $_POST['username'];
$pword = $_POST['password'];

/*** mysql hostname ***/
$hostname = 'localhost';

/*** mysql username ***/
$username = 'root';

/*** mysql password ***/
$password = 'anty90';

try {
    $link = new PDO("mysql:host=$hostname;dbname=gambling", $username, $password);
    /*** echo a message saying we have connected ***/
    echo 'Connected to database<br />';

    /*** INSERT data ***/
    $stmt = $link->prepare("INSERT INTO gamb(username, password) VALUES (?, ?)");
	
	try{
	$stmt->execute(array("$uname", "$pword"));
	} catch(PDOException $e){
	
		echo "Exception caught: $e";
	
	}

    /*** echo the number of affected rows ***/
    //echo $count;

    /*** close the database connection ***/
    $link = null;
    }
catch(PDOException $e)
    {
    echo $e->getMessage();
    }
  
}
?>

<html>

<form action='home.php' method='post'>

<input type="text" name="username" >
<input type="password" name="password" >
<input type="submit" name="submit" value="submit">

</form>

</html>

I'm new to databse programming so I was just wondering if this was vulnerable to sql injection or not.

CroNiX · September 18, 2014

It's good that you are using prepared statements. There are 2 things I would change though:

1) Store your db credentials in a separate file, and then include it on any page you use the db on. This way if the info changes, you just update one file instead of all files that you use db in. You might also just include the $link statement there too.

2) Don't output db errors to the screen, unless this is only for development server. This exposes info about your db and structure that might be useful to a malicious user. It also does the user absolutely no good to know about your db errors. Log them to a file or send an email with the error or something, and just let the user know "an error occurred" or something more generic.

Jacques1 · September 18, 2014

This is not a prepared statement!

By default, PDO just takes the parameters, escapes them and literally inserts them into the query template. So this is really just a kind of auto-escaping. You might as well escape the values yourself.

If you want actual prepared statements, you need to set the PDO::ATTR_EMULATE_PREPARES option to false:

$link = new PDO("mysql:host=$hostname;dbname=gambling", $username, $password, array(
    PDO::ATTR_EMULATE_PREPARES => false    // important! turn off fake prepared statements
));

And, yeah, do get rid of all those useless (and harmful) try statements. I have no idea what you're trying to achieve with them. Printing the error messages on the screen is already the default behaviour on a development system. There's absolutely no point in writing that down. Even worse, now the error messages are always printed on the screen, even if you publish the website on the internet.

Edited September 18, 2014 by Jacques1

ageattack · September 18, 2014

Thank you cronix for your input, I'll definitely do what you have suggested. However Jacques1, I'm not too sure what your explaining? Is the way I am doing still secure or no?

Jacques1 · September 18, 2014

No, it's not secure. At least not as secure as it should be.

Like I already said, this is not a prepared statement. The code just escapes the values. If the escape mechanism breaks (due to encoding issues, for example), then you end up with no security at all. See this example attack.

ageattack · September 18, 2014

Ok thanks, I guess I'll do my research on 2nd order injection to ensure security.

jazzman1 · September 18, 2014

In addition to these as has been mentioned above you should always be closing the statement as soon as it's not longer needed. If two applications are running and one is trying to modify something like update or delete table / column records or so, while the second one is accessing those objects as far as I know you will receive an mysql error.

Jacques1 · September 19, 2014

There's no such problem. UPDATE and DELETE queries lock the row or table, and if other processes need to access the same data, they wait until the lock is released.

Also note that it's not possible to explicitly close a prepared statement in the PDO extension. The best you can do is kill the object in order to trigger the destructor.

jazzman1 · September 19, 2014

By closing the statement I meant to kill the statement (bad habit from linux machines), but not the entire object as in the example above. If you want to execute the statement more than once along with others, we can use closeCursor(). This is your example, so I wasn't able to run this script without PDOStatement::closeCursor() and I am interested on how you did

<?php

$database_options = array(
    PDO::ATTR_EMULATE_PREPARES => false
    , PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
    , PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC
);
$database = new PDO('mysql:host=localhost;dbname=test;charset=utf8', 'root', '', $database_options);



show_prepared_statements();

// *One* prepared statement
$stmt = $database->prepare('SELECT 1 = :x');
// ... getting executed 10 times
for ($i = 1; $i <= 10; $i++)
{
    $stmt->execute(array('x' => $i));
}

echo '<br>';
show_prepared_statements();



function show_prepared_statements()
{
    global $database;

    $statements = $database->query("SHOW SESSION STATUS LIKE 'Com_stmt_%'");
    foreach ($statements as $statement)
    {
        echo $statement['Variable_name'] . ': ' . $statement['Value'] . '<br>';
    }
}

Edited September 19, 2014 by jazzman1

Jacques1 · September 19, 2014

You're getting an error about an unbuffered query, right? That's an issue of an unfetched result set, not the statement itself.

By default, PHP automatically fetches the entire result set into memory. However, if you (unwillingly) disable this, make a query, don't fetch all rows an then make another query (like the code above), you're getting an error.

Sure, you can use closeCursor() as a workaround. But the real question is: Why on earth are buffered queries disabled on your machine? Maybe you're using the old libmysqlclient library instead of mysqlnd. Look for "PDO Driver for MySQL" in phpinfo().

Edited September 19, 2014 by Jacques1

jazzman1 · September 19, 2014

You're getting an error about an unbuffered query, right?

That wasn't the error I think. Give me 10 minutes to fixed my local DNS server, b/s at that moment I don't have an access to mysql

Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[28000] [1045] Access denied for user 'lxc'@'lxc.centos.local'

PS: It's weird, now it's running without any errors. I will try to find tomorrow morning the error from the mysql_error log file and I'll post the result, but there were errors for sure.

Edited September 19, 2014 by jazzman1

jazzman1 · September 19, 2014

Yeah, you are right. I found it - "SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active" , then I googled it and found Bill Karwin's solution very useful for me. I think at that time ( more than a mount and something ago ) the server version was 5.1.x, so this testing server version is 5.5.38. Anyway....sorry, if this is an off topic sor somebody

Jacques1 · September 19, 2014

Like I said, you should actually fix the error, not use some workaround to make PHP shut up. It's hardly a solution to clutter your entire code with PDO::closeCursor() calls.

There's obviously an issue with the MySQL driver, because it doesn't support buffered queries.

jazzman1 · September 19, 2014

Based on Bill Kawin's cite:

The MySQL client protocol doesn't allow more than one query to be "in progress." That is, you've executed a query and you've fetched some of the results, but not all -- then you try to execute a second query. If the first query still has rows to return, the second query gets an error.

Here's my original code tested a mount and half ago. I've got an error without calling PDO::closeCursor().

<?php

ini_set('display_startup_errors',1);

ini_set('display_errors',1);

error_reporting(-1);

$database_options = array(
    PDO::ATTR_EMULATE_PREPARES => false
    , PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
    , PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC
);

$database = new PDO('mysql:host=lxc.centos.local;dbname=test;charset=utf8', 'lxc', 'password', $database_options);

show_prepared_statements();

// *One* prepared statement
$stmt = $database->prepare('SELECT 1 + 1 FROM DUAL');

// ... getting executed 10 times
for($i = 0; $i < 10; $i++){
    
    $stmt->execute();
    
}

$rs1 = $stmt->fetch();

//$stmt->closeCursor();

echo '<br>';

show_prepared_statements();

echo "<br>";

echo '<pre>' . print_r($rs1, true) . '</pre>';

function show_prepared_statements() {

    global $database;

    $statements = $database->query("SHOW SESSION STATUS LIKE 'Com_stmt_%'");
    foreach ($statements as $statement) {
        echo $statement['Variable_name'] . ': ' . $statement['Value'] . '<br>';
    }
}

Result:

Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active. Consider using PDOStatement::fetchAll(). Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute.' in /var/www/html/public_html/pdo/log.php:45 Stack trace: #0 /var/www/html/public_html/pdo/log.php(45): PDO->query('SHOW SESSION ST...') #1 /var/www/html/public_html/pdo/log.php(35): show_prepared_statements() #2 {main} thrown in /var/www/html/public_html/pdo/log.php on line 45

Mysql server version - 5.5.38

Php - 5.4.29

Edited September 19, 2014 by jazzman1

Jacques1 · September 19, 2014

Did you check your phpinfo() as explained in #10?

jazzman1 · September 19, 2014

This is all I have related to mysql.

[lxc@lxc-centos ~]$ php -i | grep mysql

/etc/php.d/mysql.ini,
/etc/php.d/mysqli.ini,
/etc/php.d/pdo_mysql.ini,
mysql
MYSQL_SOCKET => /var/lib/mysql/mysql.sock
MYSQL_INCLUDE => -I/usr/include/mysql
MYSQL_LIBS => -L/usr/lib64/mysql -lmysqlclient 
mysql.allow_local_infile => On => On
mysql.allow_persistent => On => On
mysql.connect_timeout => 60 => 60
mysql.default_host => no value => no value
mysql.default_password => no value => no value
mysql.default_port => no value => no value
mysql.default_socket => /var/lib/mysql/mysql.sock => /var/lib/mysql/mysql.sock
mysql.default_user => no value => no value
mysql.max_links => Unlimited => Unlimited
mysql.max_persistent => Unlimited => Unlimited
mysql.trace_mode => Off => Off
mysqli
MYSQLI_SOCKET => /var/lib/mysql/mysql.sock
mysqli.allow_local_infile => On => On
mysqli.allow_persistent => On => On
mysqli.default_host => no value => no value
mysqli.default_port => 3306 => 3306
mysqli.default_pw => no value => no value
mysqli.default_socket => no value => no value
mysqli.default_user => no value => no value
mysqli.max_links => Unlimited => Unlimited
mysqli.max_persistent => Unlimited => Unlimited
mysqli.reconnect => Off => Off
PDO drivers => mysql, sqlite
pdo_mysql
pdo_mysql.default_socket => /var/lib/mysql/mysql.sock => /var/lib/mysql/mysql.sock

Sign In

Is this PDO database connection/ insertion secure from injection?

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information