-1
\$\begingroup\$

I want to insert rows in a database table with the precaution of SQL-injection. I'm using below flexible MySQL insert function, which enters a record in a given table. Is this the best approach or is there any other approach to do the same?

<?php
/**
 * This code writes data in database. It also takes care of sql injection case
 *
 * Entry function name : mainFunc
 * Entry function arguments : 
 * $payload = [
 *   'tableName' => 'table_name',
 *   'data' => ['column1_name' => column1_value, 'column2_name' => column2_value, etc]
 * ];
 */

Class AddDBEntry {
   private $dbName;
   private $dbUserName;
   private $dbPassword;
   private $dbHost;
   private $mysqli;

  // $stmt = The SQL Statement Object
  // $param = Array of the Parameters
  public function dynamicBindVariables($stmt, $params) {
    if ($params != null) {
      // Generate the Type String (eg: 'issisd')
      $types = '';
      foreach($params as $param) {
        if(is_int($param)) {
          // Integer
          $types .= 'i';
        } elseif (is_float($param)) {
          // Double
          $types .= 'd';
        } elseif (is_string($param)) {
          // String
          $types .= 's';
        } else {
          // Blob and Unknown
          $types .= 'b';
        }
      }

      // Add the Type String as the first Parameter
      $bind_names[] = $types;

      // Loop thru the given Parameters
      for ($i=0; $i<count($params); $i++) {
        // Create a variable Name
        $bind_name = 'bind' . $i;
        // Add the Parameter to the variable Variable
        $$bind_name = $params[$i];
        // Associate the Variable as an Element in the Array
        $bind_names[] = &$$bind_name;
      }

      // Call the Function bind_param with dynamic Parameters
      call_user_func_array(array($stmt,'bind_param'), $bind_names);
    }
    return $stmt;
  }

  public function mainFunc($payLoad) {
    $tableName = $payLoad['tableName'];
    $data = $payLoad['data'];

    $this->mysqli = new mysqli($this->dbHost, $this->dbUserName, $this->dbPassword, $this->dbName);

    if ($this->mysqli->connect_errno) {
      echo  "Failed to connect to MySQL: (" . $this->mysqli->connect_errno . ") " . $mysqli->connect_error;
      die();
    }

    $stmtString = 'insert into ' . $tableName . ' (' . implode(', ', array_keys($data)) . ')' . ' values ' . '(' . str_repeat('?, ', count($data) - 1) . '?)';

    $params = array_values($data);
    if (!($stmt = $this->mysqli->prepare($stmtString))) {
      echo "Unable to prepare statement";
      // stop here, $stmt is not set, cannot continue
      die();
    }

    $stmt = $this->dynamicBindVariables($stmt, $params);
    $stmt->execute(); 

    if ($stmt->affected_rows <= 0) {
      echo "Error in insert";
    }
    $stmt->close();
  }
}
\$\endgroup\$
  • \$\begingroup\$ where does payload come from? \$\endgroup\$ – Your Common Sense Dec 11 '17 at 11:42
  • \$\begingroup\$ from web request. \$\endgroup\$ – IJAJ MULANI Dec 11 '17 at 11:44
  • \$\begingroup\$ $addDBEntry = new AddDBEntry(); $payload = [ 'tableName' => 'users', 'data' => ['name' => 'test', 'mobile' => '7777777777', 'email' => 'test@gmail.com'] ]; $addDBEntry->mainFunc($sendData); \$\endgroup\$ – IJAJ MULANI Dec 11 '17 at 11:47
  • 2
    \$\begingroup\$ and how it is supposed to be safe from SQL injection if you put most of the payload directly into the query? \$\endgroup\$ – Your Common Sense Dec 11 '17 at 11:48
  • \$\begingroup\$ I think bind_param will take care of sql injection. Is this correct? \$\endgroup\$ – IJAJ MULANI Dec 11 '17 at 11:52
-2
\$\begingroup\$

There is a vast room for the improvement, but the main and the most critical problem is SQL injection. However, I have to admit, it is not that obvious, making too many PHP users prone to it. I even wrote a dedicated article, An SQL injection against which prepared statements won't help. It explains how to inject through column names, but in your case it's even simpler, as anyone could write any SQL right into $tableName, entering something like this:

news; drop table users --

in a $payLoad['tableName']. So here goes the golden rule for the SQL injection prevention:

The golden rule

Although people just learned by heart that prepared statements is what protects them from SQL injection, the complete rule consists of two parts:

  1. A placeholder to substitute any data goes to the query
  2. A white list for the everything else.

So now you can tell that your code lacks the other part.

The implementation

In a nutshell, a white list is a very simple thing to implement - just write down all the possible choices directly in your code and choose values to be added in the SQL query from this list only.

However, for such a generic function like yours, it could be a tedious task, to list all the possible table and column names. On the other hand, if you write all this stuff down, it won't be different from writing just a regular query, but that's sort of opposite to your initial goal. However "safety first" is typically the desired side to err on, and so I would recommend to reduce the flexibility, for sake of security. So for the moment I would recommend to avoid using the mainFunc() function at all, and instead using a series of hard-coded queries.

Improvements.

  • creating a new connection every time you are running a query is a big NO. Although it doesn't matter for a primitive page that runs just a single query, but it will become a disaster for any site that is a little more complex than that. Create just a single connection, and then use it for all queries during the script's lifetime.
  • function dynamicBindVariables is rather useless, because:
    • every current PHP version lets you to bind parameters right away
    • 99% of time you can use just "s" for any variable bound
  • error reporting: Completely flawed. To get the idea refer to my article PHP error reporting. In short, just allow mysqli to generate errors by itself, but never echo an error message out.

So I would make your class like this:

Class DB
{
    public function __construct($host, $user, $pass, $db, $charset)
    {
        mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
        $this->mysqli = new mysqli($host, $user, $pass, $db);
        $this->mysqli->set_charset($charset);
    }

    public function query($sql, $params, $types = "")
    {
        $types = $types ?: str_repeat("s", count($params));
        $stmt = $this->mysqli->prepare($sql);
        $stmt->bind_param($types, ...$params);
        $stmt->execute();
        return $stmt;
    }
}

used like this for SELECT

$db = new DB("localhost", "root", "", "test", "utf8");
$row = $db->query("SELECT ? bar", ['foo'])->get_result()->fetch_assoc();

or like this for insert

$db = new DB("localhost", "root", "", "test", "utf8");
$data = [$username, $email, $password];
$row = $db->query("INSERT INTO users VALUES (NULL, ?,?,?)", $data);

although it is not that overly automated as you wanted, but it's 100% safe from SQL injection, given table and column names are hard-coded in SQL, while the data is bound through a prepared statement.

\$\endgroup\$
  • 1
    \$\begingroup\$ good answer +1... however, if memory serves (pls correct me if i'm wrong) ... the mysql driver only allows a single query at a time, so anything after the semicolon is treated as a second query and ignored. in order for an injection to occur it would have to be before the semicolon... right? \$\endgroup\$ – I wrestled a bear once. Dec 11 '17 at 16:56
  • 2
    \$\begingroup\$ i tried to be nice about correcting you and even +1'd but you're being salty and providing misleading info so i -1'd. injection will never occur the way you've illustrated it. multiple statements are ignored by mysql. \$\endgroup\$ – I wrestled a bear once. Dec 11 '17 at 20:47
  • \$\begingroup\$ Hi @YourCommonSense, I appreciate your work. I understood a lot of thing from your answer. But I tried the example you have given and it didn't work. $db = new DB("localhost", "root", "", "test", "utf8"); $row = $db->query("SELECT ? users", ['name'])->get_result()->fetch_assoc(); and O/P is below ` array(1) { ["users"]=> string(6) "mobile" } \$\endgroup\$ – IJAJ MULANI Dec 12 '17 at 10:37
  • \$\begingroup\$ @IJAJMULANI Given the O/P it worked all right. However, it's just an example that is supposed to be used as is, just to demonstrate that this code works. You better use one of your regular queries, it should work just like you expect. something like SELECT mobile from users WHERE username = ? \$\endgroup\$ – Your Common Sense Dec 12 '17 at 10:44

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.