-1
\$\begingroup\$

One of my github issues is below. I am having some problems with my security, is there anything that I should or need to improve. Here is an example of code that I use, but throughout the script there are some problems.

 <?php
session_start();
error_reporting(0);
include '../config.php';
$pass = substr(sha1($_POST['passwordp'], PASSWORD_BCRYPT), -10);

$conn = new mysqli($hostname, $username, $password, $db_name);

$query = mysqli_query($conn, "SELECT * FROM `Administrators` WHERE username = '".addslashes($_POST['usernamep'])."' AND password='".$pass."'");

$query = mysqli_query($conn, "SELECT * FROM `Administrators` WHERE username = '".addslashes($_POST['usernamep'])."' AND password='".$pass."'");

 if (mysqli_num_rows($query) > 0) {
 $_SESSION['username'] = $_POST['usernamep'];
 header('Location: dashboard.php');
 } else {
 header('Location: index.php?alert');

 $query = mysqli_query($conn, "SELECT * FROM `Administrators` WHERE username = '".addslashes($_POST['usernamep'])."' AND password='".$pass."'");

$query = mysqli_query($conn, "SELECT * FROM `Administrators` WHERE username = '".addslashes($_POST['usernamep'])."' AND password='".$pass."'");

if (mysqli_num_rows($query) > 0) {
    $_SESSION['username'] = $_POST['usernamep'];
    header('Location: dashboard.php');
} else {
    header('Location: index.php?alert');
}
  }
 ?>

The link to the whole project is here (https://github.com/poKe-Websites/poKe-Multimedia-editor)

\$\endgroup\$
  • \$\begingroup\$ I removed the copied answer from here, as it did not have anything to do with your code. As to the code you posted, it seems to be an updated version of this question. Please re-read the answer there, as you did not really include any of the advise provided (eg regarding prepared statements or weak hashing). \$\endgroup\$ – tim Jan 26 '16 at 15:42
1
\$\begingroup\$

As already noted in this answer, you should really use prepared statements.

You should also not use simple sha1 hashing. Just because you added a PASSWORD_BCRYPT parameter to sha1 does not mean that you are using bcrypt. The second argument of sha1 is regarding raw output, not regarding the hashing itself. I provided a link to the correct function in my last answer, it's here: password_hash.

Apart from that:

  • Why do you have duplicate queries? You should remove 3 of them to simplify your code.
  • I'm also not sure why you check the validity of the credentials twice. After the first check you already know if the user provided valid username and password, there is no need to check again.
  • Your indentation is off, making it really hard to see where what if ends. You can use any IDE to properly format your code.
\$\endgroup\$
  • \$\begingroup\$ I don't understand what you are talking about. Please Clarify. Also downvote? \$\endgroup\$ – Thomas Wilbur Jan 26 '16 at 16:11
  • \$\begingroup\$ @ThomasWilbur I downvoted because your question wasn't asked well (your title did not describe what your code does, it contained my answer to a completely different question, etc), and because this is a follow-up, but you did not include any of the advise given in the original question (apart from fixing the SQL injection, this code seems much worse). \$\endgroup\$ – tim Jan 26 '16 at 16:22
  • \$\begingroup\$ K Pls help me with this. \$\endgroup\$ – Thomas Wilbur Jan 26 '16 at 16:25
  • \$\begingroup\$ Can you move this to a chat idk how \$\endgroup\$ – Thomas Wilbur Jan 26 '16 at 16:25
  • \$\begingroup\$ Also I need help with this \$\endgroup\$ – Thomas Wilbur Jan 26 '16 at 16: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.