-1

The following is a snippet of my code

if(isset($_POST['register'])){
      include("../includes/config.php");
      $otp = $_POST['otp'];
      $sentotp = $_SESSION['otp'];
    if(empty($_POST['otp'])){
      $error= '<b>OTP cannot be empty.</b>';
      }
    if($otp==$sentotp){
      $fname=$_SESSION['fname'];
      $lname=$_SESSION['lname'];
      $email=$_SESSION['email'];
      $pwd=$_SESSION['password'];
      $phno=$_SESSION['phno'];
      $result2=mysqli_query($db,"INSERT INTO `user`(`fname`, `lname`,`password`, `phno`,`email`) VALUES ($fname,$lname,$pwd,$phno,$email);");
      unset($_SESSION['fname']);
      unset($_SESSION['lname']);
      unset($_SESSION['email']);
      unset($_SESSION['password']);
      unset($_SESSION['phno']);
      //header('Location: ../src/index.php');
        }
    }

As we can see, fname,lname,password and email are string datatype. But for some reason the email is not getting inserted into my db. I have tried debugging by using the following query

      $result2=mysqli_query($db,"INSERT INTO `user`(`fname`, `lname`,`password`, `phno`) VALUES ($fname,$lname,$pwd,$phno);");

In this case, the query is getting executed. Before someone suggests that $_SESSION['email'] might be NULL, I would like to clarify that it's not. I have echoed the $_SESSION['email'] and it is returning a perfect string. Also, I have set the the length of the email field to 100, so length of the email should also be not an issue.

Funk Forty Niner
  • 74,372
  • 15
  • 66
  • 132
Bruce
  • 19
  • 1
  • 5
  • 5
    Your code is open to [SQL injection](https://stackoverflow.com/q/332365/2469308) related attacks. Even `mysqli_real_escape_string` [cannot completely safeguard](https://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string/12118602#12118602) against Injection. Please learn to use [Prepared Statements](https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) – Madhur Bhaiya Oct 04 '18 at 21:05
  • 3
    ^^ + string values are required to be quoted. and storing plain text passwords is also a big no-no –  Oct 04 '18 at 21:08
  • @IdontDownVote If you pass them as bind parameters, they shouldn't be quoted. And if you don't use binds, just adding quotes doesn't really fix it anyway. The query will break if one of the values contains some characters, like quotes. – GolezTrol Oct 04 '18 at 21:16
  • @GolezTrol i think you know that i know that, and the intent of my comment. –  Oct 04 '18 at 21:18
  • @IdontDownVote I can't tell by your comment that you know that. And now I wonder why you try to set Bruce on the wrong foot by giving him poor advice that will actually clash with the other (better) advice that was given before. – GolezTrol Oct 04 '18 at 21:20
  • @GolezTrol I apologize, but consider the context, my comment started with a (^^ + ) it was adding to the previous one. As to being new, wrong, just the account. –  Oct 04 '18 at 22:17

1 Answers1

-2

For debugging this, echo out or vardump the actual SQL text that's being sent to MySQL.

Break the code into two steps, first build the SQL string, and then execute the SQL string. Then add a debugging output between those two steps:

 $sqltxt = "INSERT INTO ... ";

 echo "sqltext=" . $sqltext ;

 $result2=mysqli_query($db,$sqltext);

Then we can take the actual SQL text to a different client, and work with it there. And we will find a problem... string literals need to be enclosed in single quotes.


Also, the mysqli_error function will retrieve the MySQL error message, after we've detected an error. In this case, mysqli_query will return FALSE if an error occurs. The code can actually inspect the value returned from mysqli_query, and determine if the statement execution was successful, rather than putting it's figurative pinky finger to the corner of its mouth, Dr. Evil style "I just assume it will all go to plan. What?"


Finally, the pattern in the code appears to be vulnerable to SQL Injection.

Values incorporated into a SQL text must be properly escaped.

The mysqli_real_escape_string function is custom written to do exactly that. That's what it is designed to do.

http://php.net/manual/en/mysqli.real-escape-string.php

A better pattern would be use prepared statements with bind placeholders

https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet

spencer7593
  • 103,596
  • 14
  • 107
  • 133