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

jaypan’s picture

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.

sudeoo’s picture

Thanks Jay. I will post it there.

heine’s picture

dave reid’s picture

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.

greggles’s picture

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.