3
\$\begingroup\$

I am creating a social network and I've been told a few times a lot of things are outdated and prone to SQL injections so I am going over everything with a very fine comb and checking what's good and what's not good enough to launch on the web.

I have my login and signup pages and I want to know if it's good to hold sensitive things like: username, first and last name, email and password. And maybe an age.

Register.php:

<nav>

<form action="process2.php" method="post">

    <input type="text" id="luname" required name="username" placeholder="Username...">

    <input type="password" id="lpw" required name="pw" placeholder="Password..." >

    <button id="bt2" type="submit" name="signin">Login</button>

</form>

</nav>
<h1 style="font-size: 40px; color: #cce6ff;">Join TheSocial</h1>

<form action="process.php" method="post">

    <input id="uname" type="text" name="username" required placeholder="Username..."><p/>
        <br>
    <input id="pww" type="password" name="pw" required placeholder="Password..."><p />
        <br>
    <input id="cpw" type="password" name="pw2" required placeholder="Confirm Password..."><p />
        <br>

    <button type="submit" id="bt1" name="Register">Register</button>

</form>

process.php(signup page):

<?php

ini_set('display_errors', 1);
error_reporting(E_ALL);

//Initializing variable
$pw = ""; 
//$pw2 = ""; //Initialization value; Examples
             //"" When you want to append stuff later
             //0  When you want to add numbers later
//isset()
$pw = isset($_POST['pw']) ? $_POST['pw'] : '';
$pw2 = isset($_POST['pw2']) ? $_POST['pw2'] : '';

$success = array(); //holds success messages

$username = trim( isset($_SESSION['username']) ? $_SESSION['username'] : "" );

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register'])) {

    // looks like a hack, send to index.php
    //header('Location: index.php');
    //die();
}

require 'config/connect.php';

$errors = [];

// and so on...
if ($pw !== $pw2) {
    $errors[] = "The passwords do not match.";
}

if (!$errors) {
    //An SQL statement template is created and sent to the database
    $stmt = $conn->prepare("SELECT * FROM users WHERE username=?");
    // This function binds the parameters to the SQL query and tells the database what the parameters are.
    $stmt->bind_param("s", $_POST['username']);
    // the database executes the statement.
    $stmt->execute();
    $row = $stmt->get_result()->fetch_assoc();

    if ($row && $row['username'] == $_POST['username']) {
        $errors[] = "Username exists";
    }
}
if (!$errors) {
    $pw = password_hash($pw, PASSWORD_BCRYPT, array('cost' => 14));

    $stmt = $conn->prepare("INSERT INTO users (username, pw) VALUES(?, ?)");
    $stmt->bind_param("ss", $_POST['username'], $pw );
    $stmt->execute();

    $_SESSION["username"] = $_POST['username'];
    header('Location: profile.php');
    //die();

} else {
    // The foreach construct provides an easy way to iterate over arrays. 
    foreach ($errors as $error) {
        echo "$error <br /> \n";
    }
    echo '<a href="index.php">Try again</a><br />';
}
?>

process2.php(login):

<?php

error_reporting(E_ALL);
ini_set('display_errors', 1);

require 'config/connect.php';

$username = trim( isset($_SESSION['username']) ? $_SESSION['username'] : "" );

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['signin'])) {
// looks like a hack, send to index.php
header('Location: index.php');
die();
}

if (empty($_POST["username"])) {
echo 'Fill in username to sign in. <a href= index.php>Try again</a><br />';
die();
}

if (empty($_POST["pw"])) {
echo 'Fill in password to sign in. <a href= index.php>Try again</a><br />';
die();
}

$sql = "SELECT pw FROM users WHERE username = ?";
$stmt = mysqli_prepare($conn, $sql);
if ( !$stmt ) {

    echo mysqli_error($conn);
    die();

}

$stmt->bind_param('s', $_POST['username']);
if ( !$stmt->execute() ) {
echo mysqli_error($conn);
die();
}
// we found a row with that username, 
// now we need to check the password is correct

// get the password from the row
$stmt->bind_result($hashed_pwd); // Binds variables to a prepared statement for result storage
$stmt->fetch(); // Fetch results from a prepared statement into the bound variables

if ( password_verify($_POST['pw'], $hashed_pwd) ) {
// password verified
$_SESSION["username"] = $_POST['username'];
header('Location: profile.php');

} else {
echo 'Incorrect username or Password. <a href= index.php>Try again</a><br />';
}
?>
\$\endgroup\$
  • \$\begingroup\$ What is password_verify? \$\endgroup\$ – vnp Jun 7 '20 at 20:48
  • \$\begingroup\$ To verify the hashed password \$\endgroup\$ – user13477176 Jun 7 '20 at 21:39
  • \$\begingroup\$ I figured that much. Can you provide the code? \$\endgroup\$ – vnp Jun 7 '20 at 21:53
  • \$\begingroup\$ That's really all there is \$\endgroup\$ – user13477176 Jun 7 '20 at 22:08
  • 1
    \$\begingroup\$ @vnp: Just read the manual: php.net/manual/en/function.password-verify.php \$\endgroup\$ – Roland Illig Jun 7 '20 at 23:16
1
\$\begingroup\$

Just a few remarks.

You constantly use $_POST across your code eg $_POST['username']

Example:

if (empty($_POST["username"])) {
    $errors[] = "Fill in username to sign up";
}

You should collect the form fields to variables, sanitize them etc once, then reuse the variables in the rest of the code.

What I mean is something like this:

<?php

// init variables
$username = '';

// collect form fields
if (isset($_POST['username'])) {
    $username = trim($POST['username']);
}

// input validation
if (empty($username)) {
    echo "Username is empty";
}
?>

You have this line:

$username = trim( isset($_SESSION['username']) ? $_SESSION['username'] : "" );

but you are not doing anything with the variable ? And why trim a session variable ??? This makes no sense. Trimming the username is something that should be done on the POST request. But the username should be fixed after login.

You have this code:

$sql = "SELECT pw FROM users WHERE username = ?";
$stmt = mysqli_prepare($conn, $sql);
if ( !$stmt ) {

    echo mysqli_error($conn);
    die();

}

Note: instead of mysqli you could use PDO to make code more portable.

It's a bad idea to print raw error messages, not only because it does not look professional but it is information disclosure that can be used to leverage possible vulnerabilities. Nobody should gain insight into your tables or PHP code. If something goes wrong, show a generic message, handle the error and make sure you get notified one way or the other, then fix the error. Having an application-wide error handler would be nice.

Still, this code looks strange to me:

$stmt->fetch(); // Fetch results from a prepared statement into the bound variables

I haven't tested it, but what happens if no matching row is found ? You still try to fetch one row ? Have you tested your code with non-existent user names ?

The most interesting, and possibly the more critical thing you've not shown yet is the login form.

While this is not the most pressing issue here, I think you should consider learning a PHP framework to bring your skills up to date. The frameworks exist for a reason: to accelerate development, to produce reusable code, so that you don't reinvent the wheel and come up with poor solutions.

This is still the old way of coding. In 2020 I would not start a new project based on old development patterns. There is no added value and you are already accruing technical debt since the code can be considered outdated by today's standards.

Presentation could be improved too, just using tabulations would make the code more readable. This is important, because a proper outline of the code can make logical flaws or branching errors more visible. For example incorrectly nested ifs. I don't know how you feel, but I find it hard to parse poorly-formatted code, even when it's yours. You are tempted to skim code instead of concentrating on it because it is an eyesore.

I would like to end on a more positive note but I found out that many tutorials found online are outdated and dangerous. The worst is that the best ranking pages are those that promote bad/deprecated practices.

For instance when I type 'php secure login form' in a search engine this is what I get: How to Create a Secure Login Page in PHP with MySQL. Outdated code that contains SQL injections. This is exactly the stuff we are telling you to avoid.

  $email = trim($_POST['email']);
  $upass = trim($_POST['password']);
  $h_upass = sha1($upass);
if ($upass == ''){
     ?>    <script type="text/javascript">
                alert("Password is missing!");
                window.location = "login.php";
                </script>
        <?php
}else{
//create some sql statement             
        $sql = "SELECT * FROM  `tblmember` WHERE  `email` =  '" . $email . "' AND  `password` =  '" . $h_upass . "'";
        $result = mysqli_query($conn, $sql);

The worst is that the code was posted less than a year ago and is not some relic of the venerable past boosted by 20 years of SEO.

Note to self: do not assume good page ranking = credibility.

So it's no wonder you are struggling to find decent tutorials. I have found other examples that weren't nearly as bad but used mysql_escape_string (deprecated, then removed in PHP7).

A more reasonable example: PHP Login and Registration Script with PDO and OOP. But I still do not consider it satisfactory: it does not verify that the POST fields for username/passwords are set:

if(isset($_POST['btn-signup']))
{
   $uname = trim($_POST['txt_uname']);
   $umail = trim($_POST['txt_umail']);
   $upass = trim($_POST['txt_upass']);

That means the resulting variables are unset. That may not be a vulnerability but that is not so great.

You cannot assume that the form being submitted will be complete and not tampered with. You have to verify that every expected element is there and do not make lazy assumptions. Any server on the net is going to be subjected to incessant automated attacks. Script kiddies are after the low-hanging fruit. So you have to be paranoid.

I don't know why it is so difficult to find a decent example. Maybe it's because developers are expected to use frameworks - Laravel example

Yes there is a learning curve. But you will become more productive afterward and produce better.

You said you are building a social network, so this is an ambitious project and not a one-page script. It needs to be better structured. You need some kind of common code base.

In conclusion I think you should not even try to fix this code although it is good to understand the pitfalls. Relearn PHP, get up to speed with modern development tools.

\$\endgroup\$
  • \$\begingroup\$ So do you think Laravel would be a good framework to learn ? \$\endgroup\$ – user13477176 Jun 8 '20 at 1:11
  • \$\begingroup\$ It's not the only one but probably the most established today and it is free, open-source. Some others. Happy shopping. \$\endgroup\$ – Anonymous Jun 8 '20 at 1:17
  • \$\begingroup\$ I already told you. Don't sanitize user input. Validate it! \$\endgroup\$ – slepic Jun 8 '20 at 3:25
  • \$\begingroup\$ "You still try to fetch one row ?" Yes, this is how database APIs work. You fetch, and then if it returned anything \$\endgroup\$ – Your Common Sense Jun 8 '20 at 6:38
  • \$\begingroup\$ Is there a big problem with always using $_POST ? \$\endgroup\$ – user13477176 Jun 8 '20 at 15:04

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.