...

View Full Version : Parametrizing SQL queries



Jimather
05-02-2011, 01:43 PM
Ok apologies in advance for my rather vague question but here goes..

I'm looking to build an essentially very simple website which needn't do much more than log users in, allow them to add records to a very simple database and allow them to view other records too.

What I have done is modified this login script here: http://www.vclcomponents.com/PHP/Tips_and_Tutorials/User_Management/PHP_Login_System_with_Admin_Features-info.html to give me the login system and I have also built on its functions to insert records for viewing.

Now it is quite old and doesn't seem to do much to prevent SQL injection.

I have read that the no.1 way of coding against sql injection is to parametize queries. There seems to be very little in the way of information about this and what little information is there seems to vary quite a lot in method/code.

What I am essentially asking is it A: is it possible to simply convert this code to parametized queries without a complete rewrite. B: How would I do it ? C: is there a more up to date login script (based in php and mysql) floating around which is based around parametrized queries and would be better for me to modify ?

bullant
05-02-2011, 01:56 PM
Basically all user inputs to a sql query need to first be validated and sanitised before inserting them into an sql query to help prevent sql injection.

A relatively simple process in preparing user inputs to an sql query is to first validate the input to ensure it contains no invalid characters that could be used to facilitate sql injection and then sanitise the input with mysql_real_escape_string() if using php.

A better option to the above, although pretty safe if done properly, is to use sql prepared statements (http://dev.mysql.com/tech-resources/articles/4.1/prepared-statements.html)

This link on parameterised queries (http://msdn.microsoft.com/en-us/library/cc296201%28v=sql.90%29.aspx) might also help.

Jimather
05-02-2011, 02:06 PM
Basically all user inputs to a sql query need to first be validated and sanitised before inserting them into an sql query to help prevent sql injection.

A relatively simple process in preparing user inputs to an sql query is to first validate the input to ensure it contains no invalid characters that could be used to facilitate sql injection and then sanitise the input with mysql_real_escape_string() if using php.

A better option to the above, although pretty safe if done properly, is to use sql prepared statements (http://dev.mysql.com/tech-resources/articles/4.1/prepared-statements.html)

Thanks for the reply but I'm looking for advice specific to this code.

I know the basic principles of preventing sql injection and what prepared statements are, I'm just having difficulty with fully understanding if and how it is possible to take the login script I have purloined and work them into it without a complete rewrite.

by the way, I know I'm kinda asking a lot with this question and I fully expect it to go unasnwered but hey I thought I'd ask!

bullant
05-02-2011, 02:20 PM
ok no problem :)

The link you posted requires you to download the php login code to view it which I haven't done.

But assuming it accepts a user entered username and password then it should be relatively minimal additional code to first validate the entered username and password and then sanitising them with mysql_real_escape_string, if the code doesn't do that already, before passing the values to either a parameterised or non-parameterised query.

The last link I posted earlier provides some info on how to use parameterised queries which might help you tweak your code.

Jimather
05-02-2011, 02:48 PM
I posted a download link because it's actually quite a large script spread across several files, trying to explain exactly how it's setup would take me so long a person might as well just read the code.

Its built using classes which is why I think I'm having trouble trying to turn the queries into parameterized queries.

query's are built using information (connection info etc) setup in other classes and functions and it doesn't seem to be quite so simple to rewrite the code in a DRY manner so that it doesn't break everything else.


For example this is the function that inserts a new record (I have added this)




function addNewPrediction($userid, $prediction, $predictiondate){

$time = time();

$q = "INSERT INTO ".TBL_PREDICTIONS." (userid,prediction,predictiondate,predictionmade,flag) VALUES ('$userid', '$prediction', '$predictiondate', '$time', '0')";
return mysql_query($q, $this->connection);
}



Which works but is obviously unsecure.






Now in an attempt to just get it para entries working in some manner I have added all the connection info into the function when really I want to have that in one place for everything else to access:




function addNewPrediction($userid, $prediction, $predictiondate){

$time = time();


$db = new mysqli('localhost', 'root', 'password', 'newdb');

$stmt = $db->stmt_init();

if($stmt->prepare("INSERT INTO ".TBL_PREDICTIONS." (userid,prediction,predictiondate,predictionmade,flag) VALUES (?,?,?,?,?)")) {

$stmt->bind_param($userid, $prediction, $predictiondate, $time);

$stmt->execute();

$stmt->close();

};


}

That does not work. If I could understand why that doesn't work I think I would be halfway there to understanding even roughly how I need to alter this script.

Daniel Israel
05-02-2011, 07:37 PM
Couple things I notice at first glance (and I don't use this stuff much), is that:

1. First parameter should denote the types you are passing
2. You are declaring 5 parameters in your statement, but only binding 4.

Fou-Lu
05-02-2011, 07:39 PM
The datatypes are unknown. Great work converting to a prepared statement though, this is much better (and implicitly escapes as well) than the old school MySQL. I'll treat it in an OO fashion as you have.
I'd also recommend re-using a current MySQLi connection (passed via the method parameter) instead of opening it within the function (which if you do, don't open or close the $db).


function addNewPrediction($userid, $prediction, $predictiondate)
{
$time = time();
$db = new mysqli('localhost', 'root', 'password', 'newdb');
if (!mysql_connect_errno()) // Unfortunately, before 5.2.9/5.3.0 the connect_errno member is incorrect
{
// stmt is chained to the prepare, so you can avoid using it if you like:
if ($stmt = $db->prepare("INSERT INTO " . TBL_PREDICTIONS . " (userid, prediction, predictiondate, predictionmade, flag) VALUES (?, ?, ?, ?, ?)"))
{
$stmt->bind_param("sssss", $userid, $prediction, $predictiondate, $time, "0");
$stmt->execute();
$stmt->close();
}
$db->close();
}
else
{
throw new Exception(mysqli_connect_error());
}
}


I don't know what datatypes you have used I'm afraid. The earlier example you have indicates that they are all strings, but I doubt that they are. You should use explicit datatyping, since MySQL's implicit conversions is an option of the database itself (I myself configure MySQL to use strict datatyping all the time).

Jimather
05-03-2011, 12:02 AM
Thanks! will take a closer look at that soon as I can and let you know how I get on. Real life will be disturbing me for a while :)

Jimather
05-03-2011, 09:15 PM
Still can't get that code to work.

So I'm guessing that the "sssss" is defining the datatypes when binding the parameters there, do you have a reference for exactly which datatypes are which letter ? so I can adjust that and see if it works then.

Jimather
05-05-2011, 07:51 PM
function addNewPrediction($userid, $prediction, $predictiondate)
{

$time = time();

$mysqli = new mysqli("localhost", "root", "password", "newdb");

/* check connection */
if (mysqli_connect_errno()) {
printf("Connect failed: %s\n", mysqli_connect_error());
exit();
}

/* Create the prepared statement */
if ($stmt = $mysqli->prepare("INSERT INTO predictions (userid,prediction,predictiondate,predictionmade) VALUES (?, ?, ?, ?)")) {

/* Bind our params */
$stmt->bind_param('isii', $userid, $prediction, $predictiondate, $time);


/* Execute the prepared Statement */
$stmt->execute();

/* Echo results */
echo "Inserted {$prediction} into database\n";
printf("%d Row inserted.\n", $stmt->affected_rows);

/* Close the statement */
$stmt->close();

}
else {
/* Error */
printf("Prepared Statement Error: %s\n", $mysqli->error);
}

$mysqli->close();

}

$userid = '57';
$prediction = 'test';
$predictiondate = "9202823233";

addNewPrediction($userid, $prediction, $predictiondate);

Ok, this is a standalone test script and its close to working but not quite.

it APPEARS to run successfully and it will dutifully report that it was successful and one row was affected, but.... it hasn't. Nothing new shows up when I look at the table in phpmyadmin.

However deleting records through the same script works just fine, any ideas anyone ?

votter
05-05-2011, 10:03 PM
This can be deleted

Jimather
05-05-2011, 10:07 PM
This can be deleted

I don't quite understand :confused:


Oh and if anybody is interested, this reports '-1 row inserted' ?? minus 1 row ???


function addNewPrediction()
{


$link = mysqli_connect('localhost', 'root', 'password', 'newdb');

/* check connection */
if (!$link) {
printf("Connect failed: %s\n", mysqli_connect_error());
exit();
}

$stmt = mysqli_prepare($link, "INSERT INTO predictions VALUES (?, ?, ?, ?, ?, ?)");
mysqli_stmt_bind_param($stmt, 'isiiii', $userid, $prediction, $predictiondate, $time, $flag, $id);

$userid = '57';
$prediction = 'test';
$predictiondate = "9202823233";
$time = time();
$flag = '0';
$id = NULL;

/* execute prepared statement */
mysqli_stmt_execute($stmt);

printf("%d Row inserted.\n", mysqli_stmt_affected_rows($stmt));

/* close statement and connection */
mysqli_stmt_close($stmt);

/* close connection */
mysqli_close($link);
}



addNewPrediction();

votter
05-05-2011, 10:42 PM
My reply, not the topic. lol

Jimather
05-05-2011, 10:48 PM
My reply, not the topic. lol

Oh I see lol

Jimather
05-06-2011, 10:53 AM
Ah so -1 rows returned means that the query was invalid.

Now what is invalid about this frickin query :mad:, this is driving me insane

Fou-Lu
05-09-2011, 08:06 PM
Your statement does not capture any errors.
Add a clause to kill it:


mysqli_stmt_execute($stmt) or die(mysql_stmt_error($stmt));


You do have improper variable types, but offhand I cannot test to see what PHP will do. PHP is loose, so I'd expect an implicit cast before the bind:


mysqli_stmt_bind_param($stmt, 'isiiii', $userid, $prediction, $predictiondate, $time, $flag, $id);

$userid = '57';
$prediction = 'test';
$predictiondate = "9202823233";
$time = time();
$flag = '0';
$id = NULL;

4 of your six variables are strings, but only one is listed as a string. NULL should bind against the integer or a string, so that should be ok.

Checking the or die error is the fastest way to determine the error.

Jimather
05-30-2011, 03:40 PM
Your statement does not capture any errors.
Add a clause to kill it:


mysqli_stmt_execute($stmt) or die(mysql_stmt_error($stmt));


You do have improper variable types, but offhand I cannot test to see what PHP will do. PHP is loose, so I'd expect an implicit cast before the bind:


mysqli_stmt_bind_param($stmt, 'isiiii', $userid, $prediction, $predictiondate, $time, $flag, $id);

$userid = '57';
$prediction = 'test';
$predictiondate = "9202823233";
$time = time();
$flag = '0';
$id = NULL;

4 of your six variables are strings, but only one is listed as a string. NULL should bind against the integer or a string, so that should be ok.

Checking the or die error is the fastest way to determine the error.

Thanks for this, have been able to get it working now, still a long way to go yet though :D



EZ Archive Ads Plugin for vBulletin Copyright 2006 Computer Help Forum