Bugzilla – Full Text Bug Listing
| Summary: | [DB API] Run mysql_real_escape_string($_value, $_dbh) on the entire query, deprecate sqlSanitize ? | ||
|---|---|---|---|
| Product: | Community | Reporter: | Denis Roy <denis.roy> |
| Component: | Website | Assignee: | phoenix.ui <phoenix.ui-inbox> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | karl.matthias, nathan |
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
I noticed this when debugging the donate stuff i was working on. +1 +1 +1 +1 +1! Please! +1 :D Sanitizing the entire query just doesn't seem to be something that is done. It creates all kinds of nastiness will quotes etc. http://www.rexursive.com/pipermail/modspin-devel/2004-February/000008.html (In reply to comment #2) > Sanitizing the entire query just doesn't seem to be something that is done. It > creates all kinds of nastiness will quotes etc. > > http://www.rexursive.com/pipermail/modspin-devel/2004-February/000008.html > Ouch, that doesn't look good. How about this. >function sqlSanitize($_value, $_dbh=NULL) { >/** > * Sanitize incoming value to prevent SQL injections > * @param string value to sanitize > * @param dbh database resource to use > * @return string santized string > */ > if ($_dbh==NULL) { > $_dbh = $this->database( "eclipse", "" ); > } > > if(get_magic_quotes_gpc()) { > $_value = stripslashes($_value); > } > $_value = @mysql_real_escape_string($_value, $_dbh); > return $_value; >} I'm nulling out $_dbh, in the function call so it can be optional and allow for old code to work, AND use its own $_dbh if it wants to. If it is null, then i'm just calling the database() function with the eclipse database, chances are if we haven't used it yet, we will, this is an SQL function right :-) Makes using it easy, and solves the security issues. Thoughts? (In reply to comment #3) > >function sqlSanitize($_value, $_dbh=NULL) { > >/** > > * Sanitize incoming value to prevent SQL injections > > * @param string value to sanitize > > * @param dbh database resource to use > > * @return string santized string > > */ > > if ($_dbh==NULL) { > > $_dbh = $this->database( "eclipse", "" ); > > } > > > > if(get_magic_quotes_gpc()) { > > $_value = stripslashes($_value); > > } > > $_value = @mysql_real_escape_string($_value, $_dbh); > > return $_value; > >} Looks good, Nathan! Denis, sorry I wasn't thinking clearly and forgot about the 'database' function when you asked me about this yesterday. Yes, that's what it's for. :) That code will work well and not waste a query. On the other hand I don't think we ought to be supporting magic_quotes_gpc() anywhere. If you have it on for your dev box it should really be turned off in the config. +1 That looks good Based on Karl's feedback i've added a check in database() for get_magic_quotes_gpc() and if its enabled it will throw trigger_error("...") with a relevant error message.
This change is committed in 1.61 of app.class.php on phoenix.
Nathan, the current sqlSanitize() in App 1.61 has this:
if(isset($_dbh)) {
$_value = mysql_real_escape_string($_value, $_dbh);
}
It makes no attempt to get a dbh if none exists, which was your plan in comment 3. In its current state, the DB API-based queries will not get sanitized.
Right you are.
1.62 has this as the function
function sqlSanitize($_value, $_dbh=NULL) {
/**
* Sanitize incoming value to prevent SQL injections
* @param string value to sanitize
* @param dbh database resource to use
* @return string santized string
*/
if ($_dbh==NULL) {
$_dbh = $this->database( "eclipse", "" );
}
$_value = mysql_real_escape_string($_value, $_dbh);
return $_value;
}
I think we've fixed this as best we can. |
$App has a function called sqlSanitize, which takes a dbh as a parameter. With the DB API, we are no longer working with dbh's directly, so sqlSanitize has this hack: if(isset($_dbh)) { $_value = mysql_real_escape_string($_value, $_dbh); } else { # Establish a dummy connection $this->eclipse_sql("SELECT NOW()"); $_value = mysql_real_escape_string($_value); } It works, but it adds a useless query to the database. I'm wondering if we could simply mysql_real_escape_string() during the call to sql(). This would remove the need to sanitize each incoming parameter, and would forgive the sloppy coder who doesn't sanitize incoming parameters. # All in one query function function sql ($statement, $dbname, $logstring = null) { $dbh = $this->database( $dbname, $statement ); ---> # Run mysql_real_escape_string($statement, $dbh); here!! $result = mysql_query($statement, $dbh); $rowcount = 0; etc....