php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Doc Bug #60398 mysql_real_escape_string description is wrong and decieving
Submitted: 2011-11-27 10:26 UTC Modified: 2011-12-05 16:41 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:0 (0.0%)
Same OS:0 (0.0%)
From: col dot shrapnel at gmail dot com Assigned:
Status: Wont fix Package: Documentation problem
PHP Version: Irrelevant OS: irrelevant
Private report: No CVE-ID: None
View Developer Edit
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: col dot shrapnel at gmail dot com
New email:
PHP Version: OS:

 

 [2011-11-27 10:26 UTC] col dot shrapnel at gmail dot com 
Description:
------------
---
From manual page: http://www.php.net/function.mysql-real-escape-string#refsect1-
function.mysql-real-escape-string-description
---

I am writing in account of the mysql_real_escape_string() description, which 
current phrasing is erroneous and decieving, leading thousands of PHP 
programmers to confusion and make them writing the code that actually _allows_ 
injection.

It says at the moment
---
This function must always (with few exceptions) be used to make data safe before 
sending a query to MySQL.
---
Which is obviously wrong, as the function doesn't make data whatever "safe". 
And "few exceptions" statement is not an excuse as it explains nothing. 

Based on this very description, many people having an idea of injection 
protection limited to just "escape all your data" and actually allow an 
injection as a result.

I insists on the different phrasing, says (with obvious grammar or styling 
check):

---
This function must always be used to process every string (i.e. piece of data 
enclosed in the single quotes) added to the query.

Note that this function doesn't make any data "safe" as it's just escaping 
special characters in the strings only and thus it is useless to protect other 
data types, such as numbers or identifiers.
---

Same goes for the note, saying 
---
If this function is not used to escape data, the query is vulnerable to SQL 
Injection Attacks.
---
"data" again! 
So, it is just false statement, as even if the function were used, there are 
circumstances under which your query remains vulnerable.

Also, in account of the only purpose of this function, in should be explicitly 
noted that only prior call to mysql_set_charset() will make the 
mysql_real_escape_string different from mysql_escape_string() - i.e. make it " 
taking into account the current character set of the connection".


Test script:
---------------
$data = "1 union select password from users"
$data = mysql_real_escape_string($data);
$sql  = "SELECT title FROM news WHERE id=$data";

Expected result:
----------------
The $data become "safe" and stopped injection. 

Actual result:
--------------
The code above didn't make the data "safe" and didn't stop the injection.

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2011-12-03 04:36 UTC] frozenfire@php.net
-Status: Open +Status: Wont fix
 [2011-12-03 04:36 UTC] frozenfire@php.net 
Hi. Let me first say that I agree with you wholeheartedly. The mysql extension 
needs to be deprecated and removed as soon as possible, because it encourages 
new programmers to make bad design decisions in favour of quick, sloppy code.

However, this is not a documentation problem. So long as the project retains the 
mysql extension as an active extension, and does not provide any notice-type 
errors warning against its use, there is nothing to be done from the 
documentation perspective.

In short, yes you're right, but no we can't do anything about it.
 [2011-12-05 15:07 UTC] col dot shrapnel at gmail dot com 
Errr... I beg my pardon, but are you sure you commented the right bug?
First, I didn't say that the whole extension needs to be deprecated and removed. I never mentioned the extension at all, but one function only.
Next, I see no logic in your statement. If it was already deprecated - yes, there would be no point in improving it, I admit that. But as long as it's still active - why not to improve the documentation a bit? At least for ones who have to run a legacy code? 

I'd even dare to say that there is nothing wrong neither with mysql extension nor escaping. It has it's bad reputation mainly because of the improper use, partially inspired by the documentation page I am referring to.
 [2011-12-05 15:11 UTC] hello at apfelbox dot net 
Afaik, the mysql extension will be deprecated in PHP 5.4. You are advised to either use mysqli or PDO instead.
 [2011-12-05 16:41 UTC] frozenfire@php.net 
I'm sorry if my point wasn't clear. What I meant was that there is no point in 
writing a bunch of extra documentation to explain that the mysql extension in 
intrinsically and inextricably flawed.

We could add many warnings about how insecure the use of the extension is, and 
we could go to many lengths to expound on why and how it could *potentially* be 
used safely, but the reality is that it's not worth it.

So, I'm not saying your suggestion is bogus; just that nobody is likely to want 
to address it, since the sensible solution is to tell people to use a PDO or 
MySQLi.
 [2011-12-08 19:10 UTC] col dot shrapnel at gmail dot com 
@frozenfire
Sorry, but you still don't get the point.   
There is nothing wrong with mysql extension, save for the manual page I am talking 
about.
Mysql ext is pretty secure by itself. The only it's vulnerability is a decieving 
mysql_real_escape_string description, telling users to escape their data. As soon 
as it gets corrected, saying that this function is applicable only for strings, 
the matters become better.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Fri Dec 27 04:01:29 2024 UTC