0

I'm taking over someone else's PHP project, and when looking at the code, I've found many of these:

$query = "INSERT INTO `".$_POST["tablename"]."` SET `cust_id`='".$_POST["cust_id"]."' , `cust_email`='".$_POST["cust_email"]."' ,`".$_POST["field"]."`='".mysql_real_escape_string($_POST["value"])."'";

mysql_query($select);

Now, as far as I know, using mysql_ functions is not a good thing, due to safety and due to, well, they're deprecated... So I guess these all have to be changed to mysqli_ or PDO? But what about the $_POST["things"]? He only escaped one POST variable, why not the others?

Edit: After marking this post as duplicate: I know this is a common question, and has been asked before. It is because it is such common knowledge, that I am asking this specifically. Perhaps I was missing something...

Borniet
  • 3,544
  • 4
  • 24
  • 33
  • It's obvious that your predecessor didn't know [how to prevent SQL injection in PHP](http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection-in-php). But the code is designed in such a way that it cannot be secure--accept table names from POST? – Álvaro González Apr 26 '13 at 08:27
  • 2
    He's not a very clever guy? This question couldn't be more localized. – George Apr 26 '13 at 08:28
  • 1
    "He only escaped one POST variable, why not the others?" — Incompetence? – Quentin Apr 26 '13 at 08:28
  • 1
    To be fair, I don't think that this question is a dupe of that for which it has been closed (for the reasons given in @AlvaroGVicario's comment: SQL identifiers, such as the table and field names which are passed through `$_POST`, cannot be parameterised). See [this answer](http://stackoverflow.com/a/10926520) for some ideas on what you can do instead. – eggyal Apr 26 '13 at 08:34
  • The code given in the question is insecure in every way it could be. Un-escaped post data going directly into the query is bad... using the old mysql_xx() functions... and having the tablename specified with a post variable - ouch; don't do that. Its a good thing you already know enough to recognise the problems. My guess is that you're going to be kept very busy for quite a long time fixing up the codebase you've been left with, if that's just a two-line sample of it. – Spudley Apr 26 '13 at 08:53

1 Answers1

3

So let me clarify the answers: No! This query isn't secure AT ALL.

Marshall
  • 329
  • 1
  • 7