0

Good evening, I am just trying out something new (to me atleast) on how to login from different tables. Table1 has the cand_id (username) while Table2 has the pincode (password). The server doesn't display any error when I try to login but its not working. My code below:

if (isset($_POST['cand_id'])) {
    //escapes special characters in a string
    $cand_id = mysqli_real_escape_string($con,$cand_id);
    $pincode = mysqli_real_escape_string($con,$pincode);

    //Checking is user existing in the database or not
    $query = "SELECT cand_id 
              FROM candidates 
              WHERE cand_id='$cand_id' 
              UNION 
              SELECT pincode 
              FROM cand_login 
              WHERE pincode='$pincode'";
    $result = mysqli_query($con,$query) or die(mysql_error());
    $rows = mysqli_num_rows($result);

    if($rows==1) {
        $_SESSION['cand_id'] = $cand_id;
        // Redirect user to index.php
        header("Location: home.php");
    } else {
        echo "<div class='form'>
              <h3>cand_id/pincode is incorrect.</h3>
              <br/>Click here to <a href='log.php'>Login</a></div>";
    }
} else {
}

What do I need to make this work?

Dipu
  • 6,999
  • 4
  • 31
  • 48
  • I can see a syntax error now. Not sure about the 'no errors'... `} }else{` – ficuscr Jul 26 '18 at 20:54
  • I need a copy / paste SQL injection / mysql_ thingy... Anyone? – ficuscr Jul 26 '18 at 20:55
  • 1
    As you wish... Your code is vulnerable to [**SQL injection**](https://en.wikipedia.org/wiki/SQL_injection) attacks. You should use prepared statements with bound parameters, via either [**mysqli**](https://secure.php.net/manual/en/mysqli.prepare.php) or [**PDO**](https://secure.php.net/manual/en/pdo.prepared-statements.php). [**This post**](https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) has some good examples. – Alex Howansky Jul 26 '18 at 20:56
  • Quick question. Why do you have those details in two different tables? – Rotimi Jul 26 '18 at 20:56
  • I'm gonna copy that Alex ;) Thanks. – ficuscr Jul 26 '18 at 20:57
  • @AkintundeOlawale just wanted to try something new like i said – Onwu Bishop Gideon Jul 26 '18 at 20:58
  • @ficuscr code's been edited – Onwu Bishop Gideon Jul 26 '18 at 20:59
  • @OnwuBishopGideon help us help you. Are you not getting anything in the logs? What "does not work"? Test the SQL is returning what you expect independent of the code... Also, maybe look at `JOIN`? Union is usually a work around for other things. – ficuscr Jul 26 '18 at 21:04
  • @ficuscr i'm not getting errors. the idea is that any combination that matches available record in the tables should be enough to log users in – Onwu Bishop Gideon Jul 26 '18 at 21:08
  • @OnwuBishopGideon `var_dump` is your friend. – ficuscr Jul 26 '18 at 21:09
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/176832/discussion-between-ficuscr-and-onwu-bishop-gideon). – ficuscr Jul 26 '18 at 21:26

1 Answers1

3

Lots of things wrong here.

  • SQL injection. See comment above. Don't rely on the real_escape_string() functions to prevent SQL injection, they alone are not sufficient.
  • You're doing a UNION which would normally return two rows but you're explicitly checking for one row. If you change the code to check for two rows then you have even bigger problems:
    • Since you're pulling any row that matches the userid and then any row that matches the PIN, this would allow any user to login with any PIN.
    • UNION removes duplicates. Which means if a user has the same PIN as their userid, the query will get reduced to a single row and the login would fail.
    • If you use then UNION ALL to not remove duplicates, then you'll potentially get more than one row when people have the same PIN. I.e., as soon as somebody creates a PIN that somebody else has, then nobody with that PIN will be able to login.
  • You need to either JOIN the tables or use a WHERE clause on the cand_login table to ensure that the rows you're selecting pertain only to the one user that's logging in. Right now you're asking "Does this user exist, and does this PIN exist?" You need to ask "Does this user exist and does this user's PIN match what I was given?"
Alex Howansky
  • 50,515
  • 8
  • 78
  • 98
  • thanks. the idea is that any combination that matches available record in the tables should be enough to log users in – Onwu Bishop Gideon Jul 26 '18 at 21:12
  • That is not what that SQL query does though. Plug in nonsense for the `pincode` you will still get a `cand_id` back if `cand_id` exists. – ficuscr Jul 26 '18 at 21:14
  • @ficuscr so what i'm i missing?? – Onwu Bishop Gideon Jul 26 '18 at 21:15
  • @Alex could you append to answer and rewrite the query with a JOIN and WHERE with AND? Think you already explained why UNION is not the right thing here. Not sure on etiquette of editing others answers in that way... – ficuscr Jul 26 '18 at 21:16
  • will give it a try @ficuscr – Onwu Bishop Gideon Jul 26 '18 at 21:17
  • If you really want any user to be able to use any PIN (which I think is a terrible idea) then you can change your original query to a UNION ALL, make sure both fields on both tables have unique constraints, and change the row count check to look for exactly two rows. – Alex Howansky Jul 26 '18 at 21:19