Where am I not doing this query correctly?

Here's my query:

$mysqli = new mysqli("localhost", "root", "", "showdown");
if($mysqli->connect_errno) {
echo "Failed to connect to MySql:" . $mysqli->connect_error;
}

$email="bruce@brucegust.com";
$password = "'or '1=1--";

$params = array($email, $password);

$stmt = $mysqli->prepare("SELECT * FROM registration where email = ? and password= ?");
if ($stmt->execute(array($email, $password))) {
  while ($row = $stmt->fetch()) {
    print_r($row);
  }
}

Open in new window


The error I'm getting is: Fatal error: Call to a member function execute() on a non-object in C:\wamp\www\Texaco\mysql_injection.php on line 13

I'm trying to use this code to parameterize my email and password variables in order to avoid SQL Injection. Every example is using an OOP approach and I'm still doing procedural, so I need to get up to speed on this.

Where am I blowing it?
brucegustPHP DeveloperAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

GaryCommented:
This is wrong
$password = "'or '1=1--";

Remember SQLi and PDO separate the statement from the values - they are not a single entity.
So the only things you should be passing into a prepared statement are the values

Apart from that, that line makes no sense anyway.
Your code is creating a statement equal to this

SELECT * FROM registration where email = 'bruce@brucegust.com' and password= 'or '1=1--


And in MYSQLi you should be binding the values e.g.

$stmt->bind_param($email, $password);

and then execute it.
0
brucegustPHP DeveloperAuthor Commented:
Hey, Gary!

If a hacker inputs 'or1=1--, the following query is exploited and the user is able to login without a legitimate password:

$email="bruce@brucegust.com";
$password = "'or '1=1--";

$query = "select * from registration where email = '$email' AND contestant_password = '$password'";
$result = mysqli_query($cxn, $query);
if(!$result) {
$error = mysqli_errno($cxn).': '.mysqli_error($cxn);
die($error);
}

$count=mysqli_num_rows($result);
if($count>0)
{
echo "yes";
}

That's where those values came from.

I'm trying to parameterize the values using a select statement that looks like this:

$stmt = $mysqli->prepare("SELECT * FROM registration where email = ? and password= ?");

Apparently, by setting up the select statement that way, any rogue code that hacker is attempting to insert into my select statement will be rendered harmless. At least, that's what I've been able to understand thus far based on what I've read (feel free to correct and / or add your input at any time).

My OOP chops are minimal, still this is a good opportunity to learn something besides a concept aimed at preventing SQL Injection, so...

How should that query look given the fact that I'm intentionally trying to test it given the scenario where my user / dirtbag has intentionally inserted ''or 1=1-- as their password in order to access my database?
0
GaryCommented:
Using prepared and bound statements eliminates injection - you do not need to worry about it (unless someone comes up with a way to crack it - in which case it would just need updating)

So you are trying to prevent something that cannot happen - stop writing your code the old way (MySQL_query)

With prepared statements mysqli and pdo send the information separately - it sends the statement part (select from) and then sends the data part ($email,$password etc)
The sql statement and data are not joined in the way MySQL_query did it.
0
CompTIA Network+

Prepare for the CompTIA Network+ exam by learning how to troubleshoot, configure, and manage both wired and wireless networks.

Chris StanyonWebDevCommented:
As Gary has already said, you're thinking too old-school. With mySQLi and PDO prepared statements you can't possibly end up with:

SELECT * FROM table WHERE password = "password" OR 1=1;

That said, you have some errors in your code that would prevent it from running anyway. The execute() method doesn't take arguments (the PDO version does!!). You need to bind the arguments before executing. As you are only check if a result exists, you will only ever return 1 record, so tweak it accordingly:

$email="bruce@brucegust.com";
$password = "'or '1=1--";

if ($stmt = $mysqli->prepare("SELECT count(email) AS total FROM registration where email = ? and password= ? LIMIT 1")) {
	
	$stmt->bind_param("ss", $email, $password); //bind the data to the query
	$stmt->bind_result($total); //bind the result to the $total variable

	$stmt->execute(); //execute the query
	$stmt->fetch(); //fetch the result = it will only ever be 1 or 0
	
	if ($total) {
		echo "We have a match";
	} else {
		echo "We don't have a match";		
	};

	$stmt->close();	
}

Open in new window

0
brucegustPHP DeveloperAuthor Commented:
Chris and Gary!

First off, thanks for your time. Rather than just copying and pasting, I want to try and explain back to you what it is that you're suggesting I do and why it works the way that it does.

PDO stands for PHP Data Objects. It's a relatively new addition to the PHP family and it allows for a more secure and organized approach to querying a database - something that's especially relevant when you're trying to code in a way that prevents SQL Injection.

When looking at the code that initially started with, it was fundamentally flawed in that I was attempting to combine a procedural dynamic with an OOP / PDO approach.

PDO is a class. A class is like a football team and the object(s) are the individual players and their respective rolls. The code that you've documented above is combing the PDO class with OOP approach to coding.

The difference between OOP and procedural programming is that with OOP, instead of every piece of data being generated by a standalone query, I now have the option of grabbing the same information from one query written only one time. In other words, there's an "object" being referred to for the information rather than a solitary query that, hypothetically may have to be written and re-written multiple times within a single page.

With PDO, I'm taking things a step further in that I'm introducing an additional step in that rather than a traditional query, I'm "preparing" that query by establishing some basic scaffolding and then I'm going to "bind" my variables to that query.

What's cool about all this, apart from it being a more efficient and organized approach, is that my variables are being processed differently than if I were running the same query using a procedural dynamic.

With a procedural approach, something sinister like ' or 1=1 is going to be processed as a part of the query itself, hence a hacker's ability to access my database. But by using PDO, because of the way the variables are being bound to the query, they'll be processed as simple text and a hacker will fail to trigger a SQL command.

Is that correct?
0
brucegustPHP DeveloperAuthor Commented:
Gary, what is "ss" in $stmt->bind_param("ss", $email, $password); //bind the data to the query
0
GaryCommented:
Gary, what is "ss" in $stmt->bind_param("ss
I don't know, it wasn't my code, it was Chris' - probably just a typo

Yes you have got it. The scaffolding is a good analogy - once the query is prepared it can be used multiple times.
Where this comes into its own would be if you were executing thousands of queries - all you need to do is set your bound variables and execute, you don't need to prepare the statement again and again and again.
Also remember even though you can do the binding and execution separately for each query you can also bind multiple values (think multiple rows of queries - a hundred inserts) and execute in one go - super fast.
When I transferred to PDO I took an sql processing page from 15 minutes to 2 minutes - this was running thousands of queries.
0
Chris StanyonWebDevCommented:
Hey bruce.

You've kind of got it. In PHP you use a library to access the mySQL database. The big three are mysql, mysqli and PDO. mysql is being dropped (deprecated) so that leaves you with the 2.

The mysqli library can be used like the old mysql library in a procedural, but it can also be used in an Object Oriented manor.

PDO is purely an Object Oriented library. The code we've been using is the mysqli library, but done in the object way. PDO is very similar.

The libraries are classes, that once initiated become objects. Think of a class as a blueprint, and the object is the 'thing' created from the blueprint. Once you create an object you can execute lots of in-build methods (functions) and access lots of in-built properties.

By binding the values to a prepared query, you prevent the injection. The 'ss' in the bound parameters is an abbreviation of the data types the query will expect, so in this:

$stmt->bind_param("ss", $email, $password);

You are telling your query that it will receive 2 strings (ss) which are passed from $email and $password. The other abbreviations available are i (integer), d (decimal), and b (blob), so if your values included an integer, a decimal and a string you'd have:

$myQty = 7;
$myName = "Chris";
$myCost = 12.48;

//bind an integer, a string and a decimal
$stmt->bind_param("isd", $myQty, $myName, $myCost);

As Gary said, by preparting a query, you do that once, and then bind and execute as many times as you need:

$stmt = $mysqli->prepare("INSERT INTO yourTable (firstName) VALUES (?)");
$stmt->bind_param("s", $firstName); 

$firstName = "Chris";
$stmt->execute();

$firstName = "Bruce";
$stmt->execute();

$firstName = "Gary";
$stmt->execute();

Open in new window

Hope that all makes sense :)
0
brucegustPHP DeveloperAuthor Commented:
Guys, this is awesome!

I've engaging a number of tutorials and online resources in an attempt to understand all this and right about the time I run into a question, I'll come back to this page and see that thing explained in greater detail.

Chris, I get the ss and the additional commentary you provided as far as decimal and integer is very much appreciated.

Going back to the theory behind all this: If I were to stay with my football analogy, my class is the playbook. The moment I instantiate that class, I'm calling a play and what was a "plan / class" now becomes an "action / object" in that I'm positioning my players on the field and getting them situated so they can actually advance the ball, correct?
0
Chris StanyonWebDevCommented:
I would tend to think of it as the class encompassing every aspect of a football team, and an object being a specific football team. The class includes properties such as Team Name, Stadium, Nickname, Players and methods such as GoForAttack the Goal (???). Now you create a specific team by instantiating the class:

$giants = new FootBallTeam("New York Giants", "MetLife Stadium");
$bears = new FootBallTeam("Chicago Bears", "Soldier Field");

You now have 2 objects of the FootBallTeam class. You can now call methods on each of these:

$giants->AttackTheGoal();
$bears->Punt();

[Apologies for my ignorance on your Football - to me a football is round and belongs in a goal!]

There's so much more to OOP than we an cover here, but it will help massively if you get a good basic understanding of it. You'll come across it all the time in PHP and not just for database stuff
0
brucegustPHP DeveloperAuthor Commented:
OK, got the theory, now here's my actual "practical application:"

include("carter_pdo.inc");

$email=trim($_POST['email']);
$password = trim($_POST['password']);

if ($stmt = $db->prepare("SELECT count(email) AS total, first_name, FROM registration where email = ? and password= ? LIMIT 1")) {
	
	$stmt->bind_param("ss", $email, $password); //bind the data to the query
	$stmt->bind_result($first_name); //bind the result to the $first_name variable
	$stmt->bind_result($total); //bind the result to the $total variable

	$stmt->execute(); //execute the query
	$stmt->fetch(); //fetch the result = it will only ever be 1 or 0
	
	if ($total) {
	$_SESSION['sv_email'] = $email;
	$contestant_email = $email;
	$contestant_first_name = stripslashes($first_name);
	} 
	else 
	{
	header("Location:contestants_wronglogin.php");
	exit();
	};
	
$link = "contestants_homepage.php";
}

Open in new window


My "carter_pdo" is:

<?php
//Connect to DB
$host = "localhost"; //Host Name
$port = '3306'; //Default MySQL Port
$dbname = "showdown"; //Database Name
$db_username = "root"; //MySQL Username
$db_password = ""; //MySQL Password

$dsn = "mysql:host=$host;port=$port;dbname=$dbname"; //Data Source Name = Mysql
$db = new PDO($dsn, $db_username, $db_password); //Connect to DB
?>

When I run this code I get an error that says:

Fatal error: Call to undefined method PDOStatement::bind_param() in C:\wamp\www\Texaco\contestants_validate.php on line 33

Based on what I could find online, it seems I'm not calling things in their proper sequence, but I can't see where it is that I'm needing to change something.

Where am I blowing it?
0
Chris StanyonWebDevCommented:
Right - you've started to mix 2 libraries together: you're connecting to your Database using PDO, and then using the mySQLi methods to access your data! You need to choose 1 and stick to it...(we've been using mySQLi up until now).

While they are similar, they are not the same.  Here's the parameter binding in the 2 libraries:

//mySQLi
$stmt->bind_param("s", $email); //bind the data to the query

//PDO
$stmt->bindParam(1, $email, PDO::PARAM_STR);

Aside from the connection library used, there's a couple of other issues with your code:

Your SQL has an extra comma after first_name. Should be:

SELECT count(email) AS total, first_name FROM registration where email = ? and password= ? LIMIT 1

You also need to bind the results at the same time and in order of fields:

$stmt->bind_result($total, $first_name);
0
GaryCommented:
To confuse you more ;o)

If you use PDO I prefer this methodology, rather than binding values seperately

if ($stmt = $db->prepare("SELECT count(email) AS total, first_name, FROM registration where email = :email and password= :password LIMIT 1")) {

	$stmt->execute(array(":email"=>$email, ":password" => $password));

Open in new window

0
brucegustPHP DeveloperAuthor Commented:
OK, I'm working on this. One other question: Why position the select query in the context of an if statement? Why would that be considered a "best practice?"
0
GaryCommented:
That's Chris' fault I copied his example ;o)
0
Chris StanyonWebDevCommented:
The IF statement is not in context of the Query statement - it's in context of the prepare() statement. You're checking to makes sure the query was 'prepared' correctly (i.e had no errors in it). If it was, then you can continue on and execute it...
0
GaryCommented:
Chris
I didn't think a prepare statement would throw an error if it was incorrect - only when the execute is executed.
0
Chris StanyonWebDevCommented:
Gary you're right.

The prepare() statement will return false (and fail the IF) when the database server can't prepare the statement. It doesn't check the actual SQL statement itself.

Can't actually think of how or why the prepare() statement would fail!!
0
GaryCommented:
Sorry, know what you mean. I thought you were talking about an invalid SQL statement.
0
brucegustPHP DeveloperAuthor Commented:
Gentlemen! We're making progress, but I'm still missing something. Check it out:

$email=trim($_POST['email']);
$password = trim($_POST['password']);

echo $password;

if ($stmt = $db->prepare("SELECT count(email) AS total, first_name FROM registration where email = ? and contestant_password= ? LIMIT 1")) {
	
	$stmt->execute(array(":email"=>$email, ":contestant_password" => $password));
	$row=$stmt->fetchObject();
	
	if ($total) {
	$_SESSION['sv_email'] = $row->email;
	$contestant_email = $row->email;
	$contestant_first_name = stripslashes($row->first_name);
	} 
	else 
	{
	//header("Location:contestants_wronglogin.php");
	//exit();
	};
	
$link = "contestants_homepage.php";
}

Open in new window


I went with Gary's suggestion just because it appeared to have less opportunity for blowing it.

This is what I get in response to the code I have attached:


drumst1ck
Warning: PDOStatement::execute() [pdostatement.execute]: SQLSTATE[HY093]: Invalid parameter number: parameter was not defined in C:\wamp\www\Texaco\contestants_validate.php on line 35

Notice: Undefined variable: total in C:\wamp\www\Texaco\contestants_validate.php on line 38

Where am I going south?
0
GaryCommented:
$email=trim($_POST['email']);
$password = trim($_POST['password']);

echo $password;

if ($stmt = $db->prepare("SELECT count(email) AS total, first_name FROM registration where email = :email and contestant_password= :contestant_password LIMIT 1")) {
	
	$stmt->execute(array(":email"=>$email, ":contestant_password" => $password));
	$row=$stmt->fetch();
	
	if ($row->rowCount()>0) {
	$_SESSION['sv_email'] = $row['email'];
	$contestant_email = $row['email'];
	$contestant_first_name = stripslashes($row['first_name']);
	} 
	else 
	{
	//header("Location:contestants_wronglogin.php");
	//exit();
	};
	
$link = "contestants_homepage.php";
}

Open in new window

0
Chris StanyonWebDevCommented:
Few issues with you code, but it's certainly starting to look much better.

You are mixing anonymous parameters with named ones. To go with named ones, you include the parameter name in the query instead of question marks (preceded with a colon):

SELECT count(email) AS total, first_name FROM registration where email = :email and contestant_password= :contestant_password LIMIT 1

Because you're not binding the results, you never set the $total variable. You'll need to pull that from the row:

if ($row->total) {

You're also trying to read in the $row->email property but that was never included in your SQL Statement.

Here's my take on it:

$stmt = $db->prepare("SELECT email, first_name FROM registration where email = :email and contestant_password = :password");
		
$data = array(
	'email' => trim($_POST['email']),
	'password' => trim($_POST['password'])
);

$stmt->execute($data);

if ($row = $stmt->fetch(PDO::FETCH_OBJ)) {
	$_SESSION['sv_email'] = $row->email;
	$contestant_email = $row->email;
	$contestant_first_name = stripslashes($row->first_name);
} else {
	//No Match Found
}

$link = "contestants_homepage.php";

Open in new window

0
brucegustPHP DeveloperAuthor Commented:
Chris: I'm getting an error that says "Notice: Undefined variable: info in C:\wamp\www\Texaco\contestants_validate.php on line 47
NULL"

Gary, with your take, I'm getting: Call to a member function rowCount() on a non-object in C:\wamp\www\Texaco\contestants_validate.php on line 39

Both approaches make sense, but I'm still missing something. Where do I need to make some improvements?
0
Chris StanyonWebDevCommented:
Bruce,

Sorry, I left a debug line in from my test. I've edited the code above now, but you need to remove the following line from your code:

var_dump($info)
0
brucegustPHP DeveloperAuthor Commented:
Chris and Gary, I went back to the mysqli approach in order to achieve the kind of consistency y'all recommended and also to try and eliminate some of the errors being done by virtue of my not being as familiar with a purely PDO approach.

That said, I was able to get my code to work, save one final element. Here's the working code as far as the total number of rows being recognized and advancing the user to the homepage as opposed to being redirected to the "wrong login" page:

include("carter.inc");
$cxn = mysqli_connect($host,$user,$password,$database)
or die ("couldn't connect to server");

$email=trim($_POST['email']);
$password = trim($_POST['password']);

if ($stmt = $cxn->prepare("SELECT count(email) AS total, first_name, FROM registration where email = ? and password= ? LIMIT 1")) {
	
	$stmt->bind_param("ss", $email, $password); //bind the data to the query
	$stmt->bind_result($total, $first_name); //bind the result to the $first_name variable

	$stmt->execute(); //execute the query
	$row=$stmt->fetch(); //fetch the result = it will only ever be 1 or 0
	
	if ($total) {
	$_SESSION['sv_email'] = $row['email'];
	$contestant_email = $row['email'];
	$contestant_first_name = stripslashes($row['first_name']);
	} 
	else 
	{
	//header("Location:contestants_wronglogin.php");
	exit();
	};
	
$link = "contestants_homepage.php";
}

Open in new window


The only thing left is that I'm not able to grab the contestant's first name. I'm getting an error when I attempt to echo stripslashes($row['first_name']). It says that it's an undefined variable.

So I'm not retrieving the $row correctly. How do I repair that?
0
GaryCommented:
You have an erroneous comma here before the FROM
SELECT count(email) AS total, first_name, FROM
0
Chris StanyonWebDevCommented:
You are trying to pull the results into a variable called $row  by using the fetch() method. The fetch() method doesn't return a row - it returns a boolean (true/false)

The results are put into the variables you define in bind_column. You also seem to want to access the email from the row, but that's never in your SELECT Statement.

Your query needs to be:

SELECT count(email) AS total, email, first_name FROM registration where email = ? and password= ? LIMIT 1

Open in new window

You need to bind 3 variables:

$stmt->bind_result($total, $email, $first_name);

Open in new window

You need to drop the $row assignment:

$stmt->fetch();

Open in new window

And you need to use the variables to acces your data:

$_SESSION['sv_email'] = $email
$contestant_email = $email
$contestant_first_name = stripslashes($first_name);

Open in new window

You were so nearly there with the PDO examples (which in my opinion is easier to use than mySQLi)
0
GaryCommented:
You were so nearly there with the PDO examples (which in my opinion is easier to use than mySQLi)
Ditto
0
brucegustPHP DeveloperAuthor Commented:
Fellas, if you're willing, I want to do this right and not just get it done. If I'm hearing your counsel correctly, than PDO is the way to go.

That said, would you be willing to provide me an example of what I need using a PDO approach? I realize that I'm asking to be fed with a spoon, but I'm a pauper before kings, what can I say.

What I'll do, to honor your time, is take that example and then spend some time going through each step and explaining why it works just so you're not feeling like I'm cutting and pasting.

I'm like a puppy in front of the refrigerator waiting for it to open...
0
Chris StanyonWebDevCommented:
Here you go...
//Connect to the DB
$db = new PDO("mysql:host=localhost;dbname=yourDatabase", 'username', 'password');

//Prepare a select statment with 'named' parameters
$stmt = $db->prepare("SELECT email, first_name FROM registration where email = :email and contestant_password = :password");
		
//setup the data to be passed as the named parameter
$data = array(
	'email' => trim($_POST['email']),
	'password' => trim($_POST['password'])
);

//execute the query along with the data
$stmt->execute($data);

//if a result was returned (i.e there was a match) then assign that record to $row
if ($row = $stmt->fetch(PDO::FETCH_OBJ)) {
	//set some variables to values returned from the $row
	$_SESSION['sv_email'] = $row->email;
	$contestant_email = $row->email;
	$contestant_first_name = stripslashes($row->first_name);
} else {
	//No Match Found
}

$link = "contestants_homepage.php";

Open in new window

0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
brucegustPHP DeveloperAuthor Commented:
Smell that? That's the aroma of resolution, gentlemen!

line 2 - setting up your connection using a PDP method and not mysqli

line 5 - preparing your query. As an aside, this also serves as a way to avoid having to escape certain characters so I don't have to do mysqli_escape_string anymore

line 8 - grabbing my data. The only preparation I'm doing here is eliminating any unnecessary white space

line 14 - executing my query with the data that was specified on line 8

line 17 - PDO::FETCH_OBJ is similar to the way I've used mysqli_num_rows to see if there's a "match." Here I'm just looking to see if there's a pulse with this particular query.

line 18 - assigning my variable and BAM, good to go!

Thanks for the help, guys!

I've got to button my search query, so if you've got a minute, take a look at http://www.experts-exchange.com/Web_Development/Web_Languages-Standards/PHP/Q_28265026.html

Thanks again!
0
Chris StanyonWebDevCommented:
You're absolutely bang on except for the line 17 bit.

Line 17 is written in kind of shorthand so it's a little trickier to understand.

Basically, after you've executed a query, the fetch() command tries to pull a record from the query results. Each time you call fetch() it gets the next record. When it can no longer fetch a record (either because there were none to begin with) or it's reached the last record, then the fetch() command returns FALSE.

Line 17 uses this and it's probably easier to understand if it's broken into 2 lines

$row = $stmt->fetch(PDO::FETCH_OBJ); //fetch the record into $row or set it to false if we have no records
if ($row) { //if we don't have any records, $row would be false. If we do, $row has the database record

Open in new window

The (PDO::FETCH_OBJ) argument tells the fetch() function to return the data into an Object. We then access the columns in an OOP fashion: $row->columnName
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
PHP

From novice to tech pro — start learning today.