Hello All,
I have been using arg() function (example arg(0), arg(1)) for many years without realizing that it is unsecured from XSS attacks and it needs to be enclosed within check_plain() to make it sage.
It seems following function is unsecure and we should enclose $_GET['q'] with check_plain():
function arg($index = NULL, $path = NULL) {static $arguments;
if (!isset($path)) {
$path =$_GET['q'];
}
if (!isset($arguments[$path])) {
$arguments[$path] = explode('/', $path);
}
if (!isset($index)) {
return $arguments[$path];
}
if (isset($arguments[$path][$index])) {
return $arguments[$path][$index];
}
}
as follows for XSS attack prevention:
function arg($index = NULL, $path = NULL) {static $arguments;
if (!isset($path)) {
$path =check_plain($_GET['q']);
}
if (!isset($arguments[$path])) {
$arguments[$path] = explode('/', $path);
}
if (!isset($index)) {
return $arguments[$path];
}
if (isset($arguments[$path][$index])) {
return $arguments[$path][$index];
}
}
Any reason why we do not put check_plain here and leave it for developers to enclose their args with check_plain?
thanks a lot,
Sudeep
Comments
This would be a question
This would be a question better addressed in the Drupal issues queue (http://drupal.org/project/issues/search/drupal). The core developers apparently rarely come here (I've almost never seen them), this is area is more for questions on module development more than core coding issues.
You definitely bring up a good point though.
Contact me to contract me for D7 -> D10/11 migrations.
Thanks Jay. I will post it
Thanks Jay. I will post it there.
Crosslinking issue
#1449898: Function Arg() to be made XSS secure
Drupal filters on output, not in function calls and on input
See http://drupal.org/node/28984 - it's always been the responsibility of developers to always treat variables and URL input as untrusted and escape output properly.
check_plain is an appropriate
check_plain is an appropriate filter for data that will be sent to a browser, but the arg values are used in many different places including inserts into the database. It would be inappropriate to insert check_plained data into the database.
Please see Why does Drupal filter on output for more discussion of this point.
--
Morris Animal Foundation