4
\$\begingroup\$

I am figuring out how I could make the following query more secure: I have already used mysqli_real_escape_string but I doubt that adds much to security.

I have looked into the using prepared statements but I also have to take into account the following line:

$check_customer = mysqli_num_rows($run_c); 

My solution:

   $c_email = mysqli_real_escape_string($con,$_POST['email']);
        $c_pass = mysqli_real_escape_string($con,$_POST['pass']);
        $couponCodeLogin = mysqli_real_escape_string($con,$_POST['couponCodeLogin']);
$couponCodeLoginAmount = mysqli_real_escape_string($con,$_POST['couponCodeLoginAmount']);

        $sel_c = "select * from customers where customer_pass='$c_pass' AND customer_email='$c_email'";

        $run_c = mysqli_query($con, $sel_c);
        $check_customer = mysqli_num_rows($run_c); 
        if($check_customer==0){

        echo "<script>
        document.getElementById('loginError').innerHTML = 'Password or email is incorrect, please try again.'
        </script>";
        exit();
        }


        if($check_customer>0){
                    $crs_id = $_GET['crs_id'];

     $insert_c = "insert into customers (customer_email,coupon_code_login,coupon_code_login_amount) values ('$c_email','$couponCodeLogin','$couponCodeLoginAmount')";
                $run_c = mysqli_query($con, $insert_c); 


                $_SESSION['userCoupon'] = $_POST['couponCodeLoginAmount'];

        $_SESSION['customer_email']=$c_email; 
                        $_SESSION['userCouponName'] = $_POST['couponCodeLogin'];


        echo "<script>window.open('coursePayment.php?crs_id=$crs_id','_self')</script>";

        }
        else {
        $_SESSION['customer_email']=$c_email; 


        echo "<script>window.open('coursePayment.php?crs_id=$crs_id','_self')</script>";

        $_SESSION['userCoupon'] = $_POST['couponCodeLoginAmount'];
                $_SESSION['userCouponName'] = $_POST['couponCodeLogin'];

        }
    }

and

        $fname= mysqli_real_escape_string($con,$_POST['fname']);
        $lname = mysqli_real_escape_string($con,$_POST['lname']);
        $email = mysqli_real_escape_string($con,$_POST['email']);
        $pnumber = mysqli_real_escape_string($con,$_POST['pnumber']);       
 $couponCodeRegister=mysqli_real_escape_string($con,$_POST['couponCodeRegister']);
$couponCodeRegisterAmount = mysqli_real_escape_string($con,$_POST['couponCodeRegisterAmount']);

        $pass = mysqli_real_escape_string($con,$_POST['pass']);
        $cname = mysqli_real_escape_string($con,$_POST['cname']);
        $cposition = mysqli_real_escape_string($con,$_POST['cposition']);


         $insert_c = "insert into customers (customer_fname,customer_lname,customer_email,customer_number,customer_pass,customer_cname,customer_cposition,coupon_code_register,coupon_code_register_amount) values ('$fname','$lname','$email','$pnumber','$pass','$cname','$cposition','$couponCodeRegister','$couponCodeRegisterAmount')";

        $run_c = mysqli_query($con, $insert_c); 


        $insert_email = "select * from customers";
        $run_email = mysqli_query($con, $insert_email);

        $find_email = mysqli_fetch_array($run_email);
        $demail = $find_email['customer_email'];     

        if($email!= $demail)
       {
         $crs_id = $_GET['crs_id'];

         $_SESSION['userCoupon'] = $_POST['couponCodeRegisterAmount'];
         $_SESSION['userCouponName'] = $_POST['couponCodeRegister'];
         $_SESSION['customer_email']=$email; 

         echo "<script>
         document.getElementById('registerError').innerHTML = 'Account has been created successfully, Thanks!'
         </script>";
         echo "<script>window.open('coursePayment.php?crs_id=$crs_id','_self')</script>";

       }
\$\endgroup\$
1
\$\begingroup\$

Security

SQL injection

I am figuring out how I could make the following query more secure: I have already used mysqli_real_escape_string but I doubt that adds much to security.

It actually adds a whole lot of security. If used correctly (always use quotes around escaped variables (check for int when not using strings), don't use certain multi-byte character sets), your code will most likely be completely secure.

The thing is, it's actually quite hard to use it correctly (quotes are easily forgotten, you have to always know if a value is numeric or string, it's easy to forget calling mysqli_real_escape_string for one variable, etc), and it results in ugly code (which will be hard to check for security bugs).

TL;DR: Your code should be secure against SQL injection, but you really should use prepared statements instead.

XSS

Your code is open to XSS in multiple places. Don't echo user input without sanitizing it in some way.

When echoing variable data into HTML code, you can just use htmlspecialchars($your_string, ENT_QUOTES, 'UTF-8'); to encode dangerous characters.

When echoing into JavaScript, you need to escape dangerous characters (especially \, ', and " if you echo in quoted strings, all non-alphanumeric characters if you echo somewhere else). The answers to this question seem to suggest that calling json_encode is sufficient to protect everything, but I have not tested this and if I were you, I would research and test it thoroughly.

For further information, see this guide by owasp.

JavaScript and Usability

You rely on JavaScript a lot, even for some basic functionality. This isn't just bad for security (see above), but also for usability. Personally, I think the basics of a website should function even if JavaScript is disabled by the user (they might do this for all kinds of reasons, among them security; NoScript for example currently has 2 million users, a portion of which would probably use a competing service instead of allowing JavaScript for you).

I also think that there are very few situations where opening a new window/tab is a good idea (I mainly know it from ads, and generally close any automatically opened tabs on reflex).

I would think about either just posting it as a link, or including the page directly in the success page.

Duplicate Code

You have quite a bit of duplicate code. For example, in your first code block, the block inside the if($check_customer>0) and the else block are the same, just that the first block also has a query.

It could be rewritten as:

    if($check_customer>0){
        $insert_c = "insert into customers (customer_email,coupon_code_login,coupon_code_login_amount) values ('$c_email','$couponCodeLogin','$couponCodeLoginAmount')";
        $run_c = mysqli_query($con, $insert_c); 
    }        

    $_SESSION['customer_email'] = $c_email; 
    $_SESSION['userCoupon'] = $_POST['couponCodeLoginAmount'];
    $_SESSION['userCouponName'] = $_POST['couponCodeLogin'];

    // TODO sanitize user input
    $crs_id = $_GET['crs_id'];
    echo "<script>window.open('coursePayment.php?crs_id=$crs_id','_self')</script>";

You have a very similar code block in your second code block, so reuse this code there as well by extracting it to a function.

Misc

  • Uppercase your SQL keywords, so it's easier to separate them from variable input. This increases readability. Eg SELECT * FROM customers WHERE customer_pass[...].
  • Don't select *, but just the concrete values you need.
  • Your indentation is off, which decreases readability (can easily be fixed by any IDE).
  • Your spacing is also off (can easily be fixed by any IDE).
  • Your naming is somewhat odd. What does c mean in cname, insert_c, etc? And what does d mean in demail? I would avoid naming schemes like these, especially if you don't handle them uniformly (is the c at the end or the beginning?)
\$\endgroup\$
1
\$\begingroup\$

You need to learn how to use prepared statements, aka queries. That way, user input is never part of the SQL command.

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

http://prash.me/php-pdo-and-prepared-statements/

\$\endgroup\$

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.