2
\$\begingroup\$

I'm new to web development and this is my first website. I was wondering if my login and registration system is secure. I was also wondering how to handle viewing parts of a webpage when the user isn't logged in (ie. hide a section if the user isn't logged in). Sorry in advance if there's irrelevant stuff in the code.

Login.php

<?php


define('DB_HOST', '127.0.0.1');
define('DB_NAME', 'users');
define('DB_USER','root');
define('DB_PASSWORD','db_password');
$con=mysql_connect(DB_HOST,DB_USER,DB_PASSWORD) or die("Failed to connect to MySQL: " . mysql_error());
$db=mysql_select_db(DB_NAME,$con) or die("Failed to connect to MySQL: " . mysql_error());

LogIn();

function LogIn()
{

    $log_username = mysql_real_escape_string($_POST['username']);
    $log_password = mysql_real_escape_string($_POST['password']);

    $query = "SELECT password FROM users WHERE username = '$log_username'";
    $storedPassword = mysql_query("SELECT password FROM users WHERE username = '$log_username'");
    $row = mysql_fetch_row($storedPassword);
    $storedSalt = mysql_query("SELECT salt FROM users WHERE username = '$log_username'");
    $saltrow = mysql_fetch_row($storedSalt);
    $options = [
        'cost' => 12,
    ];

    $hash = password_hash($row, PASSWORD_BCRYPT);
    $data = mysql_query ($query)or die(mysql_error());
    if($data)
    {
        if (password_verify($log_password, $row[0])) 
        {
            //echo "Valid login";
            session_start(); 
            $_SESSION["uname"] = $log_username;

        }
        else 
        {
            echo 'Invalid username or password.';
        }
    }
}
?>

Register.php

<?php


define('DB_HOST', '127.0.0.1');
define('DB_NAME', 'users');
define('DB_USER','root');
define('DB_PASSWORD','db_password');

$con=mysql_connect(DB_HOST,DB_USER,DB_PASSWORD) or die("Failed to connect to MySQL: " . mysql_error());
$db=mysql_select_db(DB_NAME,$con) or die("Failed to connect to MySQL: " . mysql_error());

if(isset($_POST['submit']))
{
    NewUser();
}

function NewUser()
{
    $reg_email = mysql_real_escape_string($_POST['email']);
    $reg_username = mysql_real_escape_string($_POST['username']);
    $reg_password = mysql_real_escape_string($_POST['password']);
    $reg_repeatpassword = mysql_real_escape_string($_POST['repeatpassword']);
    $reg_email = mysql_real_escape_string($_POST['email']);

    $reg_hash = mysql_real_escape_string($_POST['password']);
    //$options = [
    //  'cost' => 12,
    //];
    $hash = password_hash($reg_hash, PASSWORD_BCRYPT);

    if($reg_password != $reg_repeatpassword)
    {
        echo "Passwords do not match";
    }
    else
    {
        $query = "INSERT INTO users (id, username, password, salt, email) VALUES ('', '$reg_username','$hash','','$reg_email')";
        $data = mysql_query ($query)or die(mysql_error());
        if($data)
        {
            //echo "Successfully registered";
            echo '<script type="text/javascript">alert("Registration successful.");</script>';
        }
    }
}

?>

Index.php

<body>
    <div class="container" align="center">
        <?php
        if(!isset($_SESSION['uname'])) 
        {
            echo "<div style='margin-bottom: 200px; text-align: center;'>Please log in to view uploads.<br>";   echo "</div>";
            echo "<div class='container2' align='center'>";
                    echo "<form action='/login.php'  method='post' enctype='multipart/form-data' >";
                        echo "<label for='username'>Username: </label>";
                        echo "<input type='text' id='username' name='username'>";
                        echo "<br>";
                        echo "<label for='password'>Password: </label>";
                        echo "<input type='password' id='password' name='password'>";
                        echo "<div id='lower'>";
                            echo "<input type='submit' id='submit' value='Log in'>";
                        echo "</div><!--/ lower-->";
                    echo "</form>";
                echo "</div>";
        }
        else
        {   
                echo "<div style='margin-bottom: 50px; text-align: center;'>"; echo "<h3>"; echo "Welcome, ". $_SESSION['uname']; echo "</h3>"; echo "</div>";
                echo "<div class='logoutbutton' align='center'>";
                echo "<form action='/uploads' class='logoutbutton' method='post' enctype='multipart/form-data'><input type='submit' id='submit' value='View uploads'></form>";
                echo "<form action='/logout.php' class='logoutbutton' method='post' enctype='multipart/form-data'><input type='submit' id='submit' value='Log out'></form>";
                echo "</div>";

        }?>

    </div>

    <!--Version 3.2--> 
    </body>
\$\endgroup\$
1
\$\begingroup\$
  • You are using deprecated mysql_* functions. The very first thing you should do is update to msyqli_* or, better yet, PDO. Ideally you would also move towards using parametrized prepared statements for these queries.
  • It is good that you use password_hash() and password_verify(), however I don't know why you would want to specifically use bcrypt unless you alwasy wanted to enforce this encryption in the future. I would simply consider leaving this parameter emtpy and using PHP default ecnryption, whihc is subject to change over time. This would allow your application to take advantage of potentially better encryption methodologies as they become default with PHP version changes, without sacrificing backwards compatibility.
  • You are doing no validation of the POSTed values at all. I could literally POST empty values for every field and create a new user in the database.
  • I am not understanding what your scripts are supposed to be outputting. You are not creating valid HTML documents anywhere, just HTML and/or javascript fragments.
  • Don't output error messages to the end user. Log them. Generate user-friendly UI for presenting errors and be a inspecific about the unerlying platform. Don't give the user visibility to things such as "Failed to connect to MySQL". This is information an attacker can user against you.
  • Get out of the habit of putting passwords into your code. These should ideally be derived from environmental configuration.
  • Since you are using password_hash() You should not be having a different salt field in your database. The single string generated by password_hash() includes the salt (as well as the information needed by password_verify() to split apart the salt from the main hash).
  • Don't use root mysql user for your applications. For every application you have, you should create a MySQL user with appropriate permissions on only the resources that application needs.
  • Much of your code is happy path. You just assume things are going to work. For example, what is your username lookup returns 0 results (a very common state for a login).
  • When doing login lookup, you should consider returning all pertient fields from the database that you may need to use and store in "user" object or data structure. No need to query the database on e field at a time.
  • Get rid of your unused code. For example $query and $options variable sin login. Leaving around extra cruft in your code is going to make it harder to maintain.
  • "Session" and "login" are not the same thing. I don't understand why you conditionally start a session only upon login. "Session" is used for server-side storage or data related to the session only and should not be used to convey login state. You do need to understand best practices about regenerating session id's and destroying session state based on login state. Review http://php.net/manual/en/features.session.security.management.php for good starting point on managing sessions securely in PHP.
  • Why are you using password_hash() at all in your login script? All you should be doing is getting the user record based on the use name and then using password_verify() to compare the record vs. login input.
  • Stylistically, you are not doing a good job of separating your PHP logic from your HTML display logic. This can sometimes be a challenge in PHP, but even the simple step of dropping in and out of PHP (while something should be minimized) can make you code much easy to read (and probably much easier to edit in your IDE. Compare your index page to the version I have made below. How much easier to read is this? Even notice how the StackExchange code highlighting works way better with code that is properly split apart like this.

Example:

<body>
    <div class="container" align="center">

 <?php
 if(!isset($_SESSION['uname'])) {
 ?>

         <div style='margin-bottom: 200px; text-align: center;'>Please log in to view uploads.<br></div>
         <div class='container2' align='center'>
             <form action='/login.php'  method='post' enctype='multipart/form-data'>
                 <label for='username'>Username: </label>
                 <input type='text' id='username' name='username'>
                 <br>
                 <label for='password'>Password: </label>
                 <input type='password' id='password' name='password'>
                 <div id='lower'>
                     <input type='submit' id='submit' value='Log in'>
                 </div>
                 <!--/ lower-->
             </form>
         </div>
<?php
} else {
?>   
         <div style='margin-bottom: 50px; text-align: center;'>
             <h3>Welcome, <?= $_SESSION['uname'] ?></h3>
         </div>
         <div class='logoutbutton' align='center'>
         <form action='/uploads' class='logoutbutton' method='post' enctype='multipart/form-data'>
             <input type='submit' id='submit' value='View uploads'>
         </form>
         <form action='/logout.php' class='logoutbutton' method='post' enctype='multipart/form-data'>
             <input type='submit' id='submit' value='Log out'>
         </form>
     </div>

<?php    
}
?>

    </div>
<!--Version 3.2--> 
</body>

Since you are new to development, let me point you to a great resource for learning PHP - http://www.phptherightway.com/ .From what you are showing it looks like you are using very old, very poor examples of how to work with PHP.

\$\endgroup\$
  • \$\begingroup\$ I appreciate the feedback and helping me strive to be a better programmer \$\endgroup\$ – Taylor Swift Jan 24 '17 at 21:29

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.