-2
\$\begingroup\$

I am aware that this code is vulnerable for SQL injections, but I don't know how to avoid it.

<?php
     session_start();
     $message="";
     if(count($_POST)>0) {
        require ''.$_SERVER['SERVER_NAME'].'/admin/settings.php';

        mysqli_connect($host, $user, $pass);
        mysqli_select_db($db);

        $username = mysqli_real_escape_string($_POST['user_name']);
        $password = mysqli_real_escape_string($_POST['password']);
        $result = mysqli_query("SELECT * FROM members WHERE username='" . $username . "' and password = '". $password."'");
        $row  = mysqli_fetch_array($result);

        if(is_array($row)) {
           $_SESSION["user_id"] = $row[id];
           $_SESSION["user_name"] = $row[username];
        } else {
           $message = "Invalid Username or Password!";
        }
    }//End of if

    if(isset($_SESSION["user_id"])) {
        header("Location:dashboard.php");
    }
?>
\$\endgroup\$
  • 1
    \$\begingroup\$ Does this code even work? \$\endgroup\$ – hjpotter92 Nov 8 '15 at 9:26
1
\$\begingroup\$

While I'm of 2 minds about procedural use of MySQLi, I highly support OOP style.

  1. Referring to my comment from earlier, all your calls to mysqli_* should be failing, as the procedural syntax requires the first parameter passed to the function to be the MySQL link resource.
  2. Instead of mysqli_select_db, you can pass the database to be selected as added parameter to the connect method.
  3. Do not use mysqli_real_escape..., and instead use prepared statements with parameters.
  4. You should avoid a SELECT * in queries. The developer should always know exactly which columns are necessary for the application and only those should be listed in the query.

You can switch to using PDO if are relatively new to PHP/programming. Although, if you've been developing for some time using the deprecated mysql_* calls, I'd recommend transitioning gradually to MySQLi and later to PDO.


As an example, consider the following OOP style usage of MySQLi:

$link = new mysqli($host, $user, $pass, $db);
$stmt = $link->prepare(
    "SELECT id, username
    FROM members
    WHERE username = ?
        AND password = ?"
    );
$stmt->bind_param( 'ss', $_POST['user_name'], $_POST['password'] );
$stmt->execute();
$stmt->bind_result( $id, $uname );
if( $stmt->fetch() ) {
    $_SESSION['user_id'] = $id;
    $_SESSION['user_name'] = $uname;
}
\$\endgroup\$
-1
\$\begingroup\$

First of all, use PDO and never attach parameters to a SQL query by concatenating strings. Second use filter_var functions instead of mysqli_real_escape_string and find more modern examples and tutorials about php. This code smells like 2004.

\$\endgroup\$
  • 1
    \$\begingroup\$ Can you support your argument for PDO? Why is MySQLi not good? \$\endgroup\$ – hjpotter92 Nov 8 '15 at 15:44
  • \$\begingroup\$ code.tutsplus.com/tutorials/… \$\endgroup\$ – Elzo Valugi Nov 8 '15 at 15:48
  • 1
    \$\begingroup\$ Links are not accepted as an explanation. \$\endgroup\$ – Mast Nov 8 '15 at 15:49
  • \$\begingroup\$ Lol. Links are sources of information. you wanted arguments, read \$\endgroup\$ – Elzo Valugi Nov 8 '15 at 15:53
  • \$\begingroup\$ Going through that page, the only plus points for PDO I see would be its support for more than one driver (not applicable to OP here), client side prepared statements (applicable for only MySQL 3.0 or earlier) and the named parameter support. Except for the named parameter support, I don't think there's any major benefit to switch to PDO. \$\endgroup\$ – hjpotter92 Nov 8 '15 at 15:59

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.