2

I have an API server, and I need to put all get data into data base

i use this code after connect to database:

foreach ($_GET as $key => $value)
$_GET[$key] = mysql_real_escape_string($value);

Is my code safe?

Bruno Croys Felthes
  • 1,183
  • 8
  • 27
Ali Akbar Azizi
  • 3,272
  • 3
  • 25
  • 44
  • 2
    use PDO and you're safe. http://net.tutsplus.com/tutorials/php/why-you-should-be-using-phps-pdo-for-database-access/ – binarious Feb 04 '13 at 21:43
  • This belongs on http://codereview.stackexchange.com – WWW Feb 04 '13 at 21:44
  • 3
    That might be safe. Although keep in mind that the `mysql_*` functions are deprecated. What is striking me more is the changing of the value of some superglobal. That really makes me want to punch somebody in the face. – PeeHaa Feb 04 '13 at 21:45
  • thank u, but i write a lot of code , and it take a lot of time to edit it , – Ali Akbar Azizi Feb 04 '13 at 21:45
  • @PeeHaa I do it all the time just to make others twitch. In particular $_REQUEST = array_merge($_POST,$_GET); – nathan hayfield Feb 04 '13 at 21:46
  • thanks a lot , in future i using mysqli or PDO – Ali Akbar Azizi Feb 04 '13 at 21:47
  • 1
    @nathanhayfield You just gave a new meaning to the word evil. – PeeHaa Feb 04 '13 at 21:51
  • 1
    @CooPer Please do not write such crap, it has a high certainty of unwanted behavior (double escaping, empty strings when there is no database). Take some time to write quality code, it will reduce maintenance burden (you and others) and save time in the future. Use `filter_input` to retrieve GET fields and MySQLi or PDO with prepared statements. – Lekensteyn Feb 04 '13 at 21:53

2 Answers2

8

No, your code is not safe! Because we do not see how you put your data into your query - that's the most important thing.

You can do so many things wrong, like this:

$sql = "INSERT INTO {$_GET[table]} ({$_GET[column]}) VALUES ('{$_GET[value]}')";

Only the last value is securely escaped, the first two are not!

Also, mysql_real_escape_string() evaluates the encoding setting of an ongoing database connection. Have you connected to the database before? Have you set the encoding?

Last: Do not escape stuff before you really need to. Premature escaping leads to all kind of problems because the pre-escaped data might be used for something else at the same time.

Sven
  • 69,403
  • 10
  • 107
  • 109
  • +1. No sure really why you though put curly brackets there. We usually do this for evaluation, e.g. to get variable variables.. Using `PDO` and prepared statements is the best! – Ilia Ross Jan 24 '14 at 07:19
4

At the moment it is. But note, all mysql_ functions are deprecated and will be removed from PHP and won't be supported anymore. Which present it's own security hazards.

Consider using 

mysqli_real_escape_string

More info

http://php.net/manual/en/function.mysql-real-escape-string.php

Please read red block and note security comment about default charsets. Applies to both functions.

mvbrakel
  • 936
  • 5
  • 16
  • looking at the yellow box in the manual, is it really safe even now? I mean, based on given code we don't know what has been done with charsets, so do we know if it is safe or not? – eis Feb 04 '13 at 21:48
  • close enough for horseshoes or hand grenades. – nathan hayfield Feb 04 '13 at 21:49
  • No, sorry, I disagree. Code is not safe. – Sven Feb 04 '13 at 22:02
  • @Sven is right. Just using the string escaping function will accomplish nothing if the value is then used in an SQL integer expression `WHERE id=$id`. String escaping only works if the resulting values is used as string. Context is everything. – mario Feb 04 '13 at 22:06
  • With no context given one must make an assumption. Sven is right.. but i stayed within the code i was asked to look at. Who knows what else is possibly wrong.. The fact that he is looking into this makes him a better coder that 70% of the ppl asking questions here – mvbrakel Feb 04 '13 at 22:20
  • 1
    All we see is a foreach loop and a call to a function. Is this safe? Assuming "yes" is no responsible answer here. The minimum would be "I cannot say." – Sven Feb 04 '13 at 22:22
  • @Sven. If looking like that...I dare say THIS code is safe. Because, yes it is only a foreach loop and no query. If you try to be precise in the answers, making the assumption he is going to do a query is not valid. – mvbrakel Feb 04 '13 at 22:33
  • If I am wrong, the code is accidentially safe, but at least I stated something to pay attention to. If you are wrong, the code is unsafe, and you are not explaining why you think it is safe. And "Because you are not doing any queries in your code" is a bad explanation, because we both know the queries exist. OP statement: "I need to put all GET data into database". – Sven Feb 04 '13 at 22:50
  • Agreed, never said your statement isn't valid about security (i upped ur answer a second after u posted it). but let's stop the chatting. And continue to anwer questions. – mvbrakel Feb 04 '13 at 22:53