1

im trying to check if the username and the password put into the input fields are stored in my database. And if there is not then it should give an error message. I have a user in my database atm: username: alexander
password : alexalex

But however when I run my code to login it doesn't say that it is the right login.

<?php 
 try{
  $db = new PDO('sqlite:users.db');
 }
 catch(PDOException $err){}
 if(isset($_POST['username'])){
  $username = $_POST['username'];
  $password = $_POST['password'];

  try{
   $sql = "SELECT * FROM players WHERE username='$username' AND password='$password'";
   $st = $db->prepare($sql);
   $st->execute();
   if($st->fetchColumn() == 1){
    //SUCCES LOGIN
   }else{
    //FAILED LOGIN
     }
    }catch(PDOException $err){}
   }
   ...HTML CODE...
   <form action="" method="POST" class="login-form">
     <input type="text" id="one" name="username" placeholder="Username">
     <input type="password"  id="two" name="password" placeholder="Password">
     <input type="submit" value="LOGIN" class="three">
   </form>

Very thankful for anyone who can solve this login!

Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141
Aplex
  • 162
  • 2
  • 3
  • 11
  • **Never store plain text passwords!** Please use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. If you're using a PHP version less than 5.5 you can use the `password_hash()` [compatibility pack](https://github.com/ircmaxell/password_compat). Make sure that you [don't escape passwords](http://stackoverflow.com/q/36628418/1011527) or use any other cleansing mechanism on them before hashing. Doing so *changes* the password and causes unnecessary additional coding. – Jay Blanchard May 16 '16 at 17:41
  • [Little Bobby](http://bobby-tables.com/) says [your script is at risk for SQL Injection Attacks.](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php). Even [escaping the string](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) is not safe! Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) statements for [PDO](http://php.net/manual/en/pdo.prepared-statements.php). – Jay Blanchard May 16 '16 at 17:41
  • You should use `fetch()`, not `fetchColumn()` http://php.net/manual/en/pdostatement.fetch.php – Jay Blanchard May 16 '16 at 17:43
  • Thank you guys for the response! I am aware that my code is SQL vulnerable, was just trying to see if I could get the login to work first! :) – Aplex May 16 '16 at 17:48
  • If you don't have time to do it right the first time, when will you find the time to do it right later? – Jay Blanchard May 16 '16 at 17:50
  • Btw, that `==1` may fail you if you ever decide to check on only one condition or if there's more than one user of the same name. so use `>` or `>=` along with what @JayBlanchard said about using `fetch()`. – Funk Forty Niner May 16 '16 at 17:52
  • I guess you're right – Aplex May 16 '16 at 17:52
  • ^ he is right. If you're live with this or intending to go live, stop right there. You WILL get hacked. – Funk Forty Niner May 16 '16 at 17:57
  • So I switched out the fetchColumn() with fetch(), did not solve the problem. Any other thought or is it not enough to just change it to fetch() ? – Aplex May 16 '16 at 18:00
  • there you go, just as I suspected, did you not do what I said up there? about the `==1` to `>` or `>=` ? my tests were conclusive here. Where `==1` failed on me. – Funk Forty Niner May 16 '16 at 18:01
  • Have a look at the links supplied in the comments above. The code practically writes itself! – Jay Blanchard May 16 '16 at 18:06
  • and if either `>1` or `>=1` didn't work, then your query failed and you need to find out why that is. – Funk Forty Niner May 16 '16 at 18:12
  • Yeah, im sorry. I must be very stupid since I dont understand. Thank you for your time though. Appreciate it a lot! – Aplex May 16 '16 at 18:13
  • 1
    You're welcome. I'll write up an answer so you can mark it as solved. I'll give kudos to @JayBlanchard also. – Funk Forty Niner May 16 '16 at 18:17

1 Answers1

0

As noted by Jay Blanchard:

if($st->fetchColumn() == 1)

where you should have been using fetch().

However, that would still fail you with this conditional statement once changed to fetch():

if($st->fetch() == 1)

as that checks if there is only 1 and needs to also satisfy both conditions.

If there is more than one row of the same username, that will also fail you.

Therefore either use >1 or >=1.

Also as stated in comments. Use a prepared statement and password_hash() if this is a live site or intended to go live.

It's just a matter of time before your database gets hacked.

Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141