I made a posting here http://drupal.org/node/1448714, but reazlied it would be ideal to follow security team on email as well.
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 sane.
It seems following function is unsecure and we should enclose $_GET['q'] with check_plain() in Drupal Core itself.
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
Comment #1
damien tournoud commentedYou are looking at it the wrong way. Drupal does filtering and encoding on output.
In which context did you need to output something that comes from
arg()?Comment #2
heine commentedSee also http://drupal.org/node/28984
Comment #3
sudeoo commentedThanks Damien for prompt response. I am questioning the design here and asking the reason why it should not be done. I will explain my scenario:
I work for a company with high security requirements. We created a module that was outputting tabs based on current URL (as on the facebook profile page). We safely assumed reading current arguments through arg(0), arg(1) and arg(2) and then adding these arguments to the tabs.
The "HP webinspect" deep scan was run and it knocked us down by being able to insert XSS.
I could wrap arg(0) etc with check_plain and handle this case, but I saw the function arg() and wondered why $_GET['q'] was not wrapped when this could have been much safer.
Wouldn't it better to wrap in includes/path.inc and be safe all the time?
I am re-opening the status. Should I change the category to "feature request" then ?
Comment #4
heine commentedWhat if you use arg(x) in a query, in mail, in a URL segment or in json? Should we escape for all these contexts?
Might as well just return '' to save ourselves the trouble.
Comment #5
sudeoo commentedI think Damien answers that well. I was looking from the wrong perspective: "Drupal does filtering and encoding on output" and I was looking at it from the input perspective. Thanks Damein and Heine.
Comment #6
podarok