2

Pls is this code secure?

/* Create a new mysqli object with database connection parameters */
$mysqli = new mysql('localhost', 'username', 'password', 'db');

if(mysqli_connect_errno()) {
echo "Connection Failed: " . mysqli_connect_errno();
exit();
}

/* Create a prepared statement */
if($stmt = $mysqli -> prepare("SELECT priv FROM testUsers WHERE username=?
AND password=?")) {

/* Bind parameters
s - string, b - boolean, i - int, etc */
$stmt -> bind_param("ss", $user, $pass);

/* Execute it */
$stmt -> execute();

/* Bind results */
$stmt -> bind_results($result);

/* Fetch the value */
$stmt -> fetch();

echo $user . "'s level of priviledges is " . $result;

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

/* Close connection */
$mysqli -> close();
Malfist
  • 31,179
  • 61
  • 182
  • 269
Frank Nwoko
  • 3,816
  • 5
  • 24
  • 33
  • 7
    Pedantic note: Every single one of those comments is 100% useless and should be removed. They make the code harder to read, and add no extra information... I feel the same with object method calls (`$stmt->execute()`, there's no reason to separate the calls by whitespace)... Also, indent your code properly, it'll also help readability... – ircmaxell Dec 21 '10 at 14:19
  • yes, it's secure, but it's also boooooring – Your Common Sense Dec 21 '10 at 14:54

3 Answers3

7

As far as protection against mySQL injection is concerned: Yes. Mysqli's parametrized queries are safe against injection attacks.

If $user comes from an external source, you may want to add htmlentities() the echo statement to prevent users from signing up with a user name like <script>(some malicious code)</script>

Pekka
  • 442,112
  • 142
  • 972
  • 1,088
  • You would want to store the raw value in the database and do formatting on output, including htmlentities. This way you can change the way you manipulate the data if you need too. – Malfist Dec 21 '10 at 14:48
3

The call per se is secure. You might want to put this $mysqli = new mysql('localhost', 'username', 'password', 'db'); in a separate file, though, outside your public web directory.

cbrandolino
  • 5,873
  • 2
  • 19
  • 27
1

Addition to Pekka's comment: also use htmlspecialchars on $result in the echo statement.

Oli
  • 2,370
  • 2
  • 26
  • 42
  • good point, but presumably, the array of privilege levels is hardcoded can't be influenced from outside. (A htmlentities() on it doesn't hurt either way though.) – Pekka Dec 21 '10 at 14:49
  • That's an assumption you can't be sure of..what if the privileges are maintained by an insecure admin? Basically, if it's going to the screen, you need to escape it against XSS attacks.. – Oli Dec 21 '10 at 14:52