What characters can break a Query?

I’m trying to understand what mysqli_real_escape_string does?! :-/

What characters is it that will break an SQL query and or cause security issues?

Thanks,

Debbie

that’s actually a php question

a mysql query can’t be broken – a query is either syntactically valid, and will run, or it isn’t, and will result in an error message

i’ve flagged the thread so that a moderator will move it over

But when hackers perform “SQL Injections” isn’t that a MySQL/SQL issue caused by nefarious SQL? :-/

Debbie

The database stuff is OK. Just not what you intended because poor code let the query through.

no, it isn’t

it’s a php issue caused by nefarious php code allowing the hacker to submit perfectly valid SQL code like DROP TABLE students

see http://xkcd.com/327/

So back to my original question…

What characters - coming from PHP - can create problems for an SQL query?

And how does mysqli_real_escape_string help out with this?

Debbie

[fphp]mysqli_real_escape_string[/fphp] escapes the following characters NUL (ASCII 0),
, \r, \, ', ", and Control-Z.

You can avoid these issues altogether if you use prepared queries, which mysqli supports.

or PDO, which I prefer.
PHP: PDO - Manual

prepare+exec in either PDO or mySQLi beats the ever living tar out of building values into a query string like it’s still 2003… between auto-sanitizing of values, ability to re-use queries with different data sets, ability to pre-build your queries for multiple uses, and PDO’s ability to target more than just mySQL… it’s win/win/win/win/win.

… as opposed to procedural SQL which is just 100% miserable /FAIL/ – even if it is what the majority of deployed code is using… that’s more the fault of outdated legacy code and people with their heads still wedged up PHP 4’s backside than anything else.

Basically procedural SQL is the PHP equivalent of transitional doctypes :smiley:

I was leaning towards Prepared Statements…

Debbie

I’m not following you…

Are you saying Prepared Statements are bad and outdated too, or just building queries that I way that I was likely trying to do?

Debbie

Procedural SQL is what I was saying is bad – which means anything that starts with

mysql_, pg_, and is called as a procedure.

OBJECT based SQL calls – mySQLi or PDO as an object are good – they’re NOT procedural letting you limit scope, and include ‘prepared’ queries. Prepared queries GOOD.

mysqli->xxx, or pdo->xxx good. (objects with prepare/exec)

mysql_xxx, mssql_xxx, pg_xxx, etc, etc… bad. (outdated crap)

Problem is MOST of the software in circulation uses the latter due to it being outdated code built on 1990’s methodologies… that should have gone the way of the dodo in 2003, at the very latest by 2007. Anyone who can’t run at least php 5.2 by now needs to either take their host to task, or find another host.

If for no other reason than gaping well documented security holes.

Well, this is the code I had last night…


// Create a new Member Account.
// Build Query.
$q = "INSERT INTO member(email, pass, first_name, created_on)
		VALUES('$email', '$pass', '$firstName', NOW())";

// Run Query.
$r = @mysqli_query($dbc, $q);

if (mysqli_affected_rows($dbc)==1){
	// Record Inserted.

But I am trying to rewrite my queries using Prepared Statement tonight…

Debbie

Yeah, one of the reasons I don’t like mySQLi – it still lets you use it procedurally if you want to.

As PDO I’d probably have:


$statement=$db->prepare('
	INSERT INTO member
	(email, pass, first_name, created_on)
	VALUES
	(:email, :pass, :first_name, NOW()
)';

if ($statement->exec(array(
	':email' => $email,
	':pass' => $pass,
	':first_name' => $firstName
)) {
	// Record Inserted.

Actually not true, rather than having $email, $pass and $firstname as separate variables, I’d have them already assigned as values to an array.

Lemme think, as mysqli that would be:


$statement=$db->prepare('
	INSERT INTO member
	(email, pass, first_name, created_on)
	VALUES
	(?,?,?, NOW()
)';

$statement->bindParam("s",$email);
$statement->bindParam("s",$pass);
$statement->bindParam("s",$firstName);

if ($statement->execute()) {
	// Record Inserted.

I think… mysqli really isn’t my bag of tea… too much dropping in and out of using objects and not using objects in a willy-nilly fashion. The inability to pass an array or use meaningful keys just seems like asking for it not to work.

But then, I’m currently writing stuff to target mysql, postgresql, mssql simultaneously.

DeathShadow,

If I post my “Create an Account” code here, could you check it out and let me know how it looks and if it is secure??

Debbie

Here is the code I have for my “Create an Account” page.

I’d appreciate feedback on how my code looks and if it is reasonably secure…


			<!-- MIDDLE COLUMN -->
			<div id="middle_1col">
					<form id="createAccount" action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
						<?php
							// Initialize Errors Array.
							$errors = array();

							// Connect to the database.
							require_once('private/mysqli_connect.php');


							if ($_SERVER['REQUEST_METHOD']=='POST'){
								// *************************************************************
								// HANDLE FORM.																								 *
								// *************************************************************

								// Trim all form data.
								$trimmed = array_map('trim', $_POST);

								// ********************
								// CHECK FORM DATA.		*
								// ********************

								// Check First Name.
								if (!empty($trimmed['firstName'])){
									if (preg_match('#^[A-Z \\'.-]{2,20}$#i', $trimmed['firstName'])){
										$firstName = mysqli_real_escape_string($dbc, $trimmed['firstName']);
									}else{
										$errors['firstName'] = 'Name must be 2-20 characters (A-Z \\' . -)';
									}
								}else{
										$errors['firstName'] = 'Please enter your First Name.';
								}

								// Check Email.
								// (Replacement for non-supported Email-Filter.)
								if (!empty($trimmed['email'])){
									if (preg_match('#^[A-Z0-9_\\+-]+(\\.[A-Z0-9_\\+-]+)*@[A-Z0-9-]+(\\.[A-Z0-9-]+)*\\.([A-Z]{2,7})$#i',
											$trimmed['email'])){
										$email = mysqli_real_escape_string($dbc, $trimmed['email']);
									}else{
										$errors['email'] = 'Enter a valid E-mail address.';
									}
								}else{
										$errors['email'] = 'Please enter your E-mail address.';
								}

								// Check Password.							/* TBD */
								if (!empty($trimmed['pass1'])){
										if ($trimmed['pass1'] == $trimmed['pass2']){
											$pass = mysqli_real_escape_string($dbc, trim($trimmed['pass1']));
										}else{
											$errors['pass'] = 'Your Passwords did not match.';
										}
								}else{
										$errors['pass'] = 'Please enter your Password.';
								}

								// Check for Errors.
								if (empty($errors)){
									// Form data clean.
									// Check Email availablity.

									// Build query.
									$q = 'SELECT email FROM member WHERE email=?';

									// Prepare statement.
									$stmt = mysqli_prepare($dbc, $q);

									// Bind variable.
									mysqli_stmt_bind_param($stmt, 's', $email);

									// Execute query.
									mysqli_stmt_execute($stmt);

									// Transfer result set from prepared statement.
									mysqli_stmt_store_result($stmt);

									// Check Email Availability.
									if (mysqli_stmt_num_rows($stmt)==0){
										// Email available.

										// Create a new Member Account.
										// Build query.
										$q = "INSERT INTO member(email, pass, first_name, created_on)
														VALUES(?, ?, ?, NOW())";

										// Prepare statement.
										$stmt = mysqli_prepare($dbc, $q);

										// Bind variable.
										mysqli_stmt_bind_param($stmt, 'sss', $email, $pass, $firstName);

										// Execute query.
										mysqli_stmt_execute($stmt);

										// Verify Insert.
										if (mysqli_stmt_affected_rows($stmt)==1){
											// Insert Succeeded.
											echo '<fieldset>';
											echo '<legend>Create an Account</legend>';
											echo '<p id="accountResults">Congratulations!<br /><br />
																Your account has been created, and a confirmation e-mail was sent to: "' . $email . '"<br /><br />
																Please click on the link in this e-mail to activate your account.</p>';
											echo '</legend>';
											echo '</fieldset>';
										}else{
											// Insert Failed.
											echo '<fieldset>';
											echo '<legend>Create an Account</legend>';
											echo '<p id="accountResults">You could not be registered due to a system error.</p>';
											echo '</legend>';
											echo '</fieldset>';
										}// End of VERIFY INSERT.

										// Close the statement.
										mysqli_stmt_close($stmt);

										// Close the connection.
										mysqli_close($dbc);

									}else{
										// Email taken.
										echo '<fieldset>';
										echo '<legend>Create an Account</legend>';
										echo '<p id="accountResults>This email has already been registered.<br />
														If you forgot your password, use the link to the right to have
														your password sent to you.</p>';
										echo '</legend>';
										echo '</fieldset>';
									}// End of CHECK EMAIL AVAILABILITY.

									// Do not return to Payment Form!!!
									exit();
								}// End of CHECK FOR ERRORS.
							}// End of HANDLE FORM.
						?>

						<!-- DISPLAY CREATE ACCOUNT FIELDS -->
						<fieldset>
						<legend>Create an Account</legend>
							<ol>
								<!-- First Name -->
								<li>
									<label for="firstName">First Name:</label>
									<input id="firstName" name="firstName" class="text" type="text" maxlength="20"
												 value="<?php if(isset($firstName)){echo $firstName;} ?>" />
									<?php
										if (!empty($errors['firstName'])){
											echo '<span class="error">' . $errors['firstName'] . '</span>';
										}
									?>
								</li>

								<!-- Email -->
								<li>
									<label for="email">E-mail:</label>
									<input id="email" name="email" class="text" type="text" maxlength="40"
												 value="<?php if(isset($email)){echo $email;} ?>" />
									<?php
										if (!empty($errors['email'])){
											echo '<span class="error">' . $errors['email'] . '</span>';
										}
									?>
								</li>

								<!-- Password1 -->
								<li>
									<label for="pass1">Password:</label>
									<input id="pass1" name="pass1" class="text" type="password" maxlength="40" />
									<?php
										if (!empty($errors['pass'])){
											echo '<span class="error">' . $errors['pass'] . '</span>';
										}
									?>
								</li>

								<!-- Password2 -->
								<li>
									<label for="pass2">Re-enter Password:</label>
									<input id="pass2" name="pass2" class="text" type="password" maxlength="40" />
								</li>

								<!-- Create Account Form -->
								<li>
									<input type="submit" name="createAccount" id="createAccount" value="Create Account"/>
								</li>
							</ol>
						</fieldset>
					</form>
				</div>
			</div><!-- End of #MIDDLE -->

		</div><!-- End of #INNER -->
	</div><!-- End of #WRAPPER -->


Thanks,

Debbie

You need to alter the way you store the password. It should never be shored in its raw form. Run it thought a hashing function like sha1 or greater. As well as add a user-specific salt to it. In another thread I offered this quick solution. http://www.sitepoint.com/forums/4916166-post2.html It of course can and should be improved. It is just a starting point.

I left the Password section incomplete on purpose. (Once this runs and is Okay’ed, then I’ll decide on a RegEx and Hash for the Password.)

But how do my Prepared Statements look?

And the Form handling?

Does everything look secure?

I know my code isn’t sophisticated but as long as it is solid enough to use with other people’s data then it’s good enough.

Thanks,

Debbie

The queries are fine, there is a clear separation between user data and SQL. That is all it really is when talking prepared statements. Always make sure that user controlled data uses binding and your good.

I don’t understand what you mean…

Debbie

Most of my objections to your code have little to do with security, though one big one stands out:

require_once(‘private/mysqli_connect.php’);

ANY file can call that and poof, have access to your database. That’s bad, REALLY bad… but in an age where we have idiots like the nudnik’s over at wordpress making the username and mysql password DEFINE’s, I suppose that’s par for the course… Nobody really goes to the extent I do of making the passwords delete themselves on use and only allowing them to be called from my single index.php… which is probably why it’s so easy to pwn most CMS and forums.

Other stuff I’m seeing:

$_SERVER[‘REQUEST_METHOD’]==‘POST’

easily faked and does not verify it’s actually coming from the form/page it’s supposed to. This is why I usually have a hidden input “fromform” with a random hash in it. I store the hash and IP address in a database, de-activate them on use and expire them after 24 hours invalidating the form request. That way people can’t accidentally save the same form more than once and it makes many spambots fall flat on their faces. (since I check the IP addy so as to prevent multiple logins back-to-back-to-back).

Though that’s all outside the scope of your questions here.

One thing I wouldn’t do is all the procedural calls – you’re ok security-wise because you’re still using prepare and execute, but even passing $stmt by reference to the function introduces unnecessary overhead when you could just be calling $stmt’s METHODS.

The comments that say what the NAME OF THE FUNCTION already says too is a bit wonky and unneccessary. If you need to comment every line of code, there’s something wrong with the code.

Something I suggest all new coders read:
Six ways to write more comprehensible code

Also, if you have to indent 16 times, there’s probably something wrong with your code logic OR indentation method.

Just to illustrate, where you have:


$q = 'SELECT email FROM member WHERE email=?';

// Prepare statement.
$stmt = mysqli_prepare($dbc, $q);

// Bind variable.
mysqli_stmt_bind_param($stmt, 's', $email);

// Execute query.
mysqli_stmt_execute($stmt);

// Transfer result set from prepared statement.
mysqli_stmt_store_result($stmt);

I’d probably have:


$statement=$dbc->prepare('
	SELECT email
	FROM member
	WHERE email=?
');
$statement->bindparam($stmt, 's', $email);
$statement->execute();

I’m not sure why you’re calling store_result, since that makes a COPY that is returned – since you’re not doing

$result=mysqli_stmt_store_result($stmt);

calling store_result does absolutely nothing!

Also wondering about this:


                                        if (mysqli_stmt_affected_rows($stmt)==1){
                                            // Insert Succeeded.
                                            echo '<fieldset>';
                                            echo '<legend>Create an Account</legend>';
                                            echo '<p id="accountResults">Congratulations!<br /><br />
                                                                Your account has been created, and a confirmation e-mail was sent to: "' . $email . '"<br /><br />
                                                                Please click on the link in this e-mail to activate your account.</p>';
                                            echo '</legend>';
                                            echo '</fieldset>';
                                        }else{
                                            // Insert Failed.
                                            echo '<fieldset>';
                                            echo '<legend>Create an Account</legend>';
                                            echo '<p id="accountResults">You could not be registered due to a system error.</p>';
                                            echo '</legend>';
                                            echo '</fieldset>';
                                        }// End of VERIFY INSERT.

Those aren’t forms – why are you outputting everything as forms when they are not?

You’re also using MULTIPLE echo statements to do the job of ONE.


echo '
	<h2>Congratulations! Account Created.</h2>
	<p>
		A confirmation e-mail was sent to: "',$email,'"<br />
	</p><p>
		Please click on the link in that e-mail to activate your account.
	</p>';

… and if you’re using echo, use comma’s unless inside a conditional – periods require a string addition before it is sent to echo, while commas treats each one as a separate send… which means lower memory use and faster execution.

You’re starting to reach the point at which I would consider building a formHandler object – so you can handle all your forms from one routine… Tell ya what, I’ll extract some of the code from my WIP CMS and post it up here so you can see what I mean by that. Basically I do this by passing several values to the object during it’s ‘constructor’ letting me use one object and it’s methods to handle ALL forms on my site… including validation and output of the form.