13

This is my code:

  $email= mysqli_real_escape_string($db_con,$_POST['email']);
  $psw= mysqli_real_escape_string($db_con,$_POST['psw']);

  $query = "INSERT INTO `users` (`email`,`psw`) VALUES ('".$email."','".$psw."')";

Could someone tell me if it is secure or if it is vulnerable to the SQL Injection attack or other SQL attacks ?

Gumbo
  • 643,351
  • 109
  • 780
  • 844
xRobot
  • 25,579
  • 69
  • 184
  • 304
  • 1
    I don't see how. mysqli_real_escape_string() is a completely different function than mysql_real_escape_string()... Maybe most people just didn't know that in 2015? – Ayelis Jul 19 '18 at 06:39

1 Answers1

18

Could someone tell me if it is secure or if it is vulnerable to the SQL Injection attack or other SQL attacks ?

No. As uri2x says, see SQL injection that gets around mysql_real_escape_string().

The best way to prevent SQL injection is to use prepared statements. They separate the data (your parameters) from the instructions (the SQL query string) and doesn't leave any room for the data to contaminate the structure of your query. Prepared statements solve one of the fundamental problems of application security.

For situation where you cannot use prepared statements (e.g. LIMIT), using a very strict whitelist for each specific purpose is the only way to guarantee security.

// This is a string literal whitelist
switch ($sortby) {
    case 'column_b':
    case 'col_c':
        // If it literally matches here, it's safe to use
        break;
    default:
        $sortby = 'rowid';
}

// Only numeric characters will pass through this part of the code thanks to type casting
$start = (int) $start;
$howmany = (int) $howmany;
if ($start < 0) {
    $start = 0;
}
if ($howmany < 1) {
    $howmany = 1;
}

// The actual query execution
$stmt = $db->prepare(
    "SELECT * FROM table WHERE col = ? ORDER BY {$sortby} ASC LIMIT {$start}, {$howmany}"
);
$stmt->execute(['value']);
$data = $stmt->fetchAll(PDO::FETCH_ASSOC);

I posit that the above code is immune to SQL injection, even in obscure edge cases. If you're using MySQL, make sure you turn emulated prepares off.

$db->setAttribute(\PDO::ATTR_EMULATE_PREPARES, false);
Scott Arciszewski
  • 33,610
  • 16
  • 89
  • 206
  • Saying "this is the best way" doesn't answer the question, "Is this enough?" – j_allen_morris Jul 07 '21 at 15:30
  • 2
    The answer is "No". The first sentence of my answer was "see this example" which proves it's not enough. – Scott Arciszewski Jul 14 '21 at 16:32
  • 3
    @ScottArciszewski - the citation you gave was for an answer about `mysql_real_escape_string()` which afaik is a different function than `mysqli_real_escape_string()`. Although your advice is not bad, I don't think this answer is correct. I've been building queries with user form input escaped with `mysqli_real_escape_string()` for over a decade and despite many thousands (millions?) of observed attempts at injection, not one has succeeded. If you believe it's not secure, I'd like to see a demonstration. – But those new buttons though.. Apr 07 '22 at 12:20
  • Always it is better to use the best practice what is available today. You can never guarantee anything about tomorrow because of it is enough today. – Muneer Aug 28 '22 at 18:19
  • as previously mentioned, this is adressing the wrong function. mysql_real_escape_string was depracted because it had vulnerabilities. What came after was mysqli_real_escape_string(). This is a misleading answer – Daniel Bengtsson Dec 22 '22 at 05:26
  • @EatenbyaGrue Don't both `mysql_real_escape_string()` and `mysqli_real_escape_string()` call through to the same [underlying C function](https://dev.mysql.com/doc/c-api/8.0/en/mysql-real-escape-string-quote.html)? I can't imagine they'd behave differently – Phil Mar 19 '23 at 22:43