-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
  • 1
    \$\begingroup\$ Does this code even work? \$\endgroup\$
    – hjpotter92
    Commented Nov 8, 2015 at 9:26

2 Answers 2

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\$
6
  • 1
    \$\begingroup\$ Can you support your argument for PDO? Why is MySQLi not good? \$\endgroup\$
    – hjpotter92
    Commented Nov 8, 2015 at 15:44
  • \$\begingroup\$ code.tutsplus.com/tutorials/… \$\endgroup\$ Commented Nov 8, 2015 at 15:48
  • 1
    \$\begingroup\$ Links are not accepted as an explanation. \$\endgroup\$
    – Mast
    Commented Nov 8, 2015 at 15:49
  • \$\begingroup\$ Lol. Links are sources of information. you wanted arguments, read \$\endgroup\$ Commented Nov 8, 2015 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
    Commented Nov 8, 2015 at 15:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.