0

I was trying to use prepared statements, but it always throws me an SQL syntax error at line 1. My goal is to make this as secure as possible. The appended code works.

The GET value is formatted as a string (for instance: nmcxxwakfe) and originally dynamically generated, so there's no way to only allow specific values.

try {
$db = new PDO("mysql:dbname=somedb;host=localhost", "person", "mysupersecretpw" );

$db->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );//Error Handling

$sql = "SELECT * FROM $value";
$result = $db->prepare($sql);
$result->execute();
$daten = $result->fetchAll();
} catch(PDOException $e) {
echo $e->getMessage();//Remove or change message in production code
}

Thanks for help!

  • What is `$value`? If it's a nonexistent variable, or empty, then you are sending the DB `SELECT * FROM `, which is indeed syntactically incorrect. See what `$sql` contains (e.g., with `echo`) before it is sent to the `$db->prepare()` function. – hunteke Feb 17 '18 at 07:28
  • if (isset($_GET['id'])) { $value = $_GET['id']; } My original code works, but it's an insecure implementation and I want to make it more safe. – Landstalker Feb 17 '18 at 07:31
  • why do intend on making the table name dynamic? That's risky,especially since its being gotten from the URL – Rotimi Feb 17 '18 at 07:31
  • @Akintunde007 I'm on board with where you're going, but given the tenor of Landstalker's question and knowledge base, I suggest we get to a working solution first. We can deal with security second, here. – hunteke Feb 17 '18 at 07:32
  • Yes, I'm writing a poll script and it fetches the corresponding poll from the database. – Landstalker Feb 17 '18 at 07:33
  • security should be your main priority. Its not about including insecure and obselete features in your app. Anyone can easily wipe out your database with the code you have – Rotimi Feb 17 '18 at 07:34
  • @hunteke: Umm, the code works. – Landstalker Feb 17 '18 at 07:34
  • Prepare cannot be used to replace elements like tables or columnnames. You have to check/escape the content of $value manually (if you really need to have dynamic tablenames). – Solarflare Feb 17 '18 at 07:35
  • @Landstalker ? Have I misunderstood your question? I thought the problem was MySQL was giving you a syntax error? Again, what -- exactly -- is the value of `$sql` before the `prepare()` call? – hunteke Feb 17 '18 at 07:35
  • My goal is to make this as secure as possible. The appended code works. – Landstalker Feb 17 '18 at 07:37

1 Answers1

0

For security, you're absolutely on target by looking toward prepared statements. However, the only variable in your code snippet is a table name, and that poses a problem: table names cannot be parameterized, at least in MySQL.

So, how would you validate that the passed table is valid? A whitelist. Two approaches might be:

  1. Hard code a list of valid tables in your script:

    $validTableNames = ['somename', 'othertablename', 'stattable'];

  2. Dynamically get them from the DB:

    $sql = "SELECT table_name FROM information_schema.tables where table_schema = '<your_database_name>'"
    $validTableNames = [];
    foreach ( $db->query( $sql ) as $row )
        $validTableNames[] = $row['table_name'];
    

And then, confirm that the passed input is a valid name:

if ( ! in_array($_GET['id'], $validTableNames) ) {
    // the passed table name is not valid.  Do what you will ...
}
hunteke
  • 3,648
  • 1
  • 7
  • 17
  • I like the pragmatic approach! It works; thank you! ^^ Btw: if you know about an even more secure method, it'd be nice to know. – Landstalker Feb 17 '18 at 07:56
  • @Landstalker security is rarely a checkbox, and is more often a question of making it too expensive for a cracker ("Not worth my while!"). With your contrived scenario, this is about as good as you're going to get. When you add more complexity and more variables, we might talk about different approaches, but for the question as posed, I doubt you'll find much better. – hunteke Feb 17 '18 at 08:00