6
\$\begingroup\$

What do I need to do to prevent injections? In addition, if I have any statement/parameter errors, please tell me.

 <?
 $username  = 'username';
 $pass      = 'pass';
 $cpass     = 'cpass';
 $email     = 'email';
 $gender    = 'gender';
 $firstname = 'firstname';
 $lastname  = 'lastname';
 $phone     = 'phone';
 $address   = 'address';

 $username    = htmlspecialchars($username, ENT_QUOTES);
 $pass        = htmlspecialchars($pass, ENT_QUOTES);
 $cpass       = htmlspecialchars($cpass, ENT_QUOTES);
 $email       = htmlspecialchars($email, ENT_QUOTES);
 $gender      = htmlspecialchars($gender, ENT_QUOTES);
 $firstname   = htmlspecialchars($firstname, ENT_QUOTES);
 $lastname    = htmlspecialchars($lastname, ENT_QUOTES);
 $phone       = htmlspecialchars($phone, ENT_QUOTES);
 $address     = htmlspecialchars($address, ENT_QUOTES);

 $uLength     = strlen($username);
 $pLength     = strlen($pass);
 $cpLength    = strlen($cpass);
 $emailLength = strlen($email);
 if ($uLength >= 6 && $uLength <= 35) {

} else {

$errorMessage = $errorMessage . "Username must be between 6 and 35 characters" . "<BR>";

}
if ($emailLength >= 3 && $emailLength <= 42) {

} else {

$errorMessage = $errorMessage . "Email must be between 3 and 42 characters" . "<BR>";

}

if ($pLength >= 8 && $pLength <= 64) {

} else {

$errorMessage = $errorMessage . "Password must be between 8 and 64 characters" . "<BR>";

}
if ($cpLength >= 8 && $cpLength <= 64) {

} else {

$errorMessage = $errorMessage . "Confirmation password must be between 8 and 64 characters" . "<BR>";
}
$user_name    = 'u904609109_dev';
$pass_word    = '_____';
$database     = 'u904609109_users';
$server       = 'mysql.2freehosting.com';

$con = mysqli_connect($server, $user_name, $pass_word, $database);
if (mysqli_connect_error()) {
    die('Connect Error: (' . mysqli_connect_errno() . ') ' . mysqli_connect_error());
}

echo 'Connect Error:' . $mysqli->host_info . "\n";

$result = mysqli_query($con, $database);
if (!$result) {
    die("Failed to load " . mysqli_error($con));
}

$dblink = "SELECT * FROM users WHERE (username= '" . $username . "')";
$dblink->bind_param('username', $username);
if ($dblink) {
}

$result2  = mysqli_query($result, ($dblink));
$num_rows = mysqli_num_rows($result);

if ($num_rows > 0) {

    $errorMessage = "Username already taken";

} else {
}
$dblink = $mysqli_query("SELECT * FROM users WHERE (email = '" . $email . "')");
$dblink->bind_param('username', $username);
if ($dblink) {
}
$result2  = mysqli_query($result, $dblink);
$num_rows = mysqli_num_rows($result);

if ($num_rows > 0) {

    $errorMessage = "Email already taken";

} else {
}
$username = mysqli_real_escape_string($username);
$pass = mysqli_real_escape_string($pass);
$cpass = mysqli_real_escape_string($cpass);
$gender = mysqli_real_escape_string($gender);
$email = mysqli_real_escape_string($email);
$firstname = mysqli_real_escape_string($firstname);
$lastname = mysqli_real_escape_string($lastname);
$address = mysqli_real_escape_string($address);
$phone = mysqli_real_escape_string($phone);

$username= $_POST[($username)];
$pass= $_POST[($pass)];
$cpass= $_POST[($cpass)];
$gender= $_POST[($gender)];
$email= $_POST[($email)];
$firstname= $_POST[($firstname)];
$lastname= $_POST[($lastname)];
$address= $_POST[($address)];
$phone= $_POST[($phone)];

mysqli_select_db($database, $con);


$sql = "INSERT INTO users(username, pass, cpass, gender, email, firstname, lastname, address, phone) VALUES ('" . $username . "', '" . $pass . "', '" . $cpass . "', '" . $gender . "', '" . $email . "', '" . $firstname . "',  '" . $lastname . "', '" . $address . "', '" . $phone . "')";

$result1 = mysqli_query($con, $sql);
if (!$result1 || mysqli_query($con, $sql)) {
    die('Invalid query: ' . mysqli_error());
}
echo htmlentities($sql);

header("Location: finished.html"); {
    mysqli_close($con);
}
}
?>
\$\endgroup\$
5
  • 2
    \$\begingroup\$ What if I set my user name to 0 OR <> 0; DELETE FROM users; ? \$\endgroup\$ – Phrancis May 22 '14 at 16:21
  • \$\begingroup\$ If you're willing to put in the work I can show you how this would be better done using MySQL PROCEDURE but you need write access to the database. Let me know. I don't know much about PHP so I can't speak to your PHP script but I'm good with SQL. \$\endgroup\$ – Phrancis May 22 '14 at 16:28
  • \$\begingroup\$ well I have JavaScript preventing any invalid values, which can be disabled rofl. and I will \$\endgroup\$ – Orenrocco May 22 '14 at 16:36
  • \$\begingroup\$ Yes, I know I stated that \$\endgroup\$ – Orenrocco May 22 '14 at 16:54
  • 2
    \$\begingroup\$ This code appears to be garbled. All the DB access is inside the failure branch for the password length check. Can you fix it so we can review working code? Also please fix the indentation so we can read it more easily. \$\endgroup\$ – Anonymous May 22 '14 at 17:24
10
\$\begingroup\$

Give me parameterized SQL or give me death!

Seriously. Use prepared statements! Don't use mysqli_query as that makes your code vulnerable to SQL Injection. By using prepared statements, you won't need the mysqli_real_escape_string calls anymore.


This code looks nasty:

$result1 = mysqli_query($con, $sql);
if (!$result1 || mysqli_query($con, $sql)) {
    die('Invalid query: ' . mysqli_error());
}

First you perform the query. If the query fails (i.e. it returns a false-ish value), you die, but if the query works then you call the query again. And if the second query is successful, then you call die.

THAT MAKES NO SENSE!


This code can't possibly be working:

$dblink = "SELECT * FROM users WHERE (username= '" . $username . "')";
$dblink->bind_param('username', $username);

And again, don't concatenate SQL queries! Use prepared statements!


Use better variable names!

What is $result1 and what is $result2? Describe what the result is for in the variable name.


You have a whole bunch of empty blocks, such as this:

if ($emailLength >= 3 && $emailLength <= 42) {
    
} else {
    
$errorMessage = $errorMessage . "Email must be between 3 and 42 characters" . "<BR>";
    
}

Also work on your indentation. Fixing the indentation, the if-logic and the string concatenation, and I suggest that you write your HTML tags with lowercase, this can be:

if ($emailLength < 3 || $emailLength > 42) {
    $errorMessage .= "Email must be between 3 and 42 characters<br>";
}
\$\endgroup\$
3
  • \$\begingroup\$ Preach it Simon! Now I know who to go to when I have PHP questions lol. \$\endgroup\$ – Phrancis May 22 '14 at 22:03
  • \$\begingroup\$ @Phrancis I am far from an expert in the PHP language though! There are a lot of users on this site who are better than I am, I was just first to answer this time. \$\endgroup\$ – Simon Forsberg May 22 '14 at 22:46
  • \$\begingroup\$ I have not touched PHP since I was fourteen or fifteen. During that time and then I have been studying electrical and computer engineering. However, I did do some C-sharp coding during that time for my own Ultima Online server core scripts. I originally stopped PHP because I had screwed up a social network I was developing or the hosting site did by shutting down. I took a break and learned about BASIC, and others but when I came across WINE. The idea sparked into my head, what if you web browser that could run applications and call POSIX on the fly you could have dual operating systems. HARD! \$\endgroup\$ – Orenrocco May 23 '14 at 1:10
2
\$\begingroup\$

Let me offer at least a partial answer in regards to MySQL and prepared statements. Someone else can address PHP once your code is cleaner.

You would exec the following code within your MySQL instance. This needs done only once.

-- start creating procs

DELIMITER //   

-- username check proc    
DROP PROCEDURE IF EXISTS sp_uCheck//
CREATE PROCEDURE sp_uCheck(p_username IN VARCHAR(35))    
BEGIN    
    SELECT * FROM users WHERE username = p_username;    
END//   

-- email check proc    
DROP PROCEDURE IF EXISTS sp_emailCheck//
CREATE PROCEDURE sp_emailCheck(p_email IN VARCHAR(42))    
BEGIN    
    SELECT * FROM users WHERE email = p_email;    
END//    

-- insert user proc   
DROP PROCEDURE IF EXISTS sp_newUser//
CREATE PROCEDURE sp_newUser(p_username IN VARCHAR(35), p_pass IN VARCHAR(64), p_cpass IN VARCHAR(64), p_gender IN VARCHAR(10), p_email IN VARCHAR(42), p_firstname IN VARCHAR(20), p_lastname IN VARCHAR(20), p_address IN VARCHAR(50), p_phone IN INTEGER)    
BEGIN    
    INSERT INTO users((username, pass, cpass, gender, email, firstname, lastname, address, phone)    
    VALUES (p_username, p_pass, p_cpass, p_gender, p_email, p_firstname, p_lastname, p_address, p_phone);    
END// 

DELIMITER ;

-- stop creating procs

Now, you would need to make a few changes to your PHP.

This line:

$dblink = "SELECT * FROM users WHERE (username= '" . $username . "')";

Becomes:

$dblink = "CALL sp_uCheck('" . $username . "')";

This line:

$dblink = $mysqli_query("SELECT * FROM users WHERE (email = '" . $email . "')");

Becomes:

$dblink = $mysqli_query("CALL sp_emailCheck('" . $email . "')");

And this section:

$sql = "INSERT INTO users(username, pass, cpass, gender, email, firstname, lastname, address, phone) VALUES ('" . $username . "', '" . $pass . "', '" . $cpass . "', '" . $gender . "', '" . $email . "', '" . $firstname . "',  '" . $lastname . "', '" . $address . "', '" . $phone . "')";

Becomes:

$sql = "CALL sp_newUser('" . $username . "', '" . $pass . "', '" . $cpass . "', '" . $gender . "', '" . $email . "', '" . $firstname . "',  '" . $lastname . "', '" . $address . "', '" . $phone . "')";

You get the idea. This way you protect to an extent your database structure from malicious users and make it more difficult to inject. There are also ways PHP can help achieve this but I don't know much about them, would rather someone else speak to that.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you, sorry my php database connector is so messed up. I have not touched php in 2 years and I have lost my way \$\endgroup\$ – Orenrocco May 22 '14 at 22:30
  • 1
    \$\begingroup\$ This does not really solve the problem of malicious input in $email or $username for example, or do I miss something? \$\endgroup\$ – Jonas Schäfer May 23 '14 at 6:32
  • \$\begingroup\$ Not completely, it makes it more difficult since the user doesn't know what the MySQL query look like. You could build other stuff into MySQL proc like setting a different delimiter etc. There is likely other stuff you can do within PHP to make it more difficult. But as Simon rightly pointed out, "don't concatenate SQL queries!" and that is what I was addressing. \$\endgroup\$ – Phrancis May 23 '14 at 13:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.