RSS Feed tag injection problem

Rasmus

Joined: 2003-11-25
Posts: 46

Posted: Mon, 2010-01-11 04:27

Have a look at:

http://phpics.com/index.php/rss/feed/gallery/album/1/bogus_path/</xss>/

Do a view-source and look for: xss

Not really an XSS and I don't see it as a security issue since it isn't HTML and I doubt any browsers will run any injected Javascript there, but it probably still isn't a good idea to allow users to inject arbitrary xml tags into the Gallery3 feeds.

-Rasmus

bharat

Joined: 2002-05-21
Posts: 7985

Posted: Fri, 2010-01-15 03:46

What version of Gallery 3 are you using? I can't reproduce that with the latest version from Git. If you're not using the latest, can you upgrade and see if that resolves it for you?
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git

Rasmus

Joined: 2003-11-25
Posts: 46

Posted: Sun, 2010-01-17 22:47

Just did a git pull now. I am still seeing it. And looking at the awful spaghetti framework code it is not easy to see where it comes from, but I suppose it is from the Router::current_uri which I don't see any filtering on.

Rasmus

Joined: 2003-11-25
Posts: 46

Posted: Mon, 2010-01-18 06:28

Had a closer look. This is in the feed.mrss.php view. It uses $feed->uri which comes from this in the controller:

$feed->uri = url::abs_site(str_replace("&", "&amp;", url::merge($_GET)));

That line of code alone should send out all sorts of warning signals. Replacing & with & is not how you urlencode stuff that is going to end up in a link.

But, following along here, url::merge($_GET) returns $current_uri with all the GET args merged and concatenated to it:

public static function merge(array $arguments)
{
  if ($_GET === $arguments)
  {
    $query = Router::$query_string;
  }
  elseif ($query = http_build_query(array_merge($_GET, $arguments)))
  {
    $query = '?'.$query;
  }

  // Return the current URI with the arguments merged into the query string
  return Router::$current_uri.$query;
}

And tracking back to where Router::$current_uri is set, we see it comes from the find_uri() method in the Router. And since it is using PATH_INFO, it comes from this line of code:

  if (isset($_SERVER['PATH_INFO']) AND $_SERVER['PATH_INFO'])
  {
    Router::$current_uri = $_SERVER['PATH_INFO'];
  }

So, $current_uri is the raw PATH_INFO which in this case is "/rss/feed/gallery/album/1/bogus_path/</xss>/"

I see no filtering anywhere in this chain of following this PATH_INFO data through the framework soup.

It really feels like there are too many moving parts here and no clear barrier between dirty user data and data that ends up being used in views.

-Rasmus

bharat

Joined: 2002-05-21
Posts: 7985

Posted: Mon, 2010-01-18 19:14

Hey, you recommended this framework (back when it was CodeIgniter). We're just following your lead

You're right -- that particular line should have set off lots of warning bells when I did my code review. I did some forensics and realized why it didn't.. Kohana used to run htmlspecialchars() on the request path as part of the framework, but they did away with that in http://dev.kohanaphp.com/issues/1712 and we didn't come back and close the loop here. I wasn't seeing this on my various boxes because I'm using the default Suhosin setup with Debian and it's sanitizing the request path for me, so </xss> is converted to ??xss?. With Suhosin off, I am easily able to reproduce the problem.

We expect Kohana not to do XSS sanitization on the path, only on the get and post parameters, and those are validated a second time in all our controller and view code. In most cases this is not a problem because we build our own paths from known-good data. But specifically url::current() and url::merge() were vulnerable.

Filed as https://sourceforge.net/apps/trac/gallery/ticket/983
Fixed in http://github.com/gallery/gallery3/commit/0dc184e99f0ca607774a68257432a9a981f4d5b7

Let me know if you can think of a better solution. Also, I'd be curious to hear how you discovered this issue, especially if you've got some automated tools we can use
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git

Rasmus

Joined: 2003-11-25
Posts: 46

Posted: Fri, 2010-01-29 22:49

Blah, everyone seems to think I am a big fan of CodeIgniter/Kohana. I prefer Kohana the same way I prefer the shortest and least rusty of 10 nails that someone is about to hammer through my eyeball.

I found it by pointing my XSS scanner at it. But no, the tool isn't available.

By the way, http://gallery.menalto.com/node/93569 is still an issue in the current git code.

-Rasmus

bharat

Joined: 2002-05-21
Posts: 7985

Posted: Sat, 2010-01-30 20:12

Yeah the CodeIgniter guys made a big deal out of it. From a product perspective I can say that it sucks to write your own framework, and it's pretty hard to develop a portable framework with a thriving developer community without something to base it on. Kohana works pretty well for us.

Thanks for the XSS info. If you turn up any other XSS vulnerabilities with your scanner, please let us know!
---
Problems? Check gallery3/var/logs
bugs/feature req's | upgrade to the latest code | use git

Rasmus Joined: 2003-11-25 Posts: 46	Posted: Mon, 2010-01-11 04:27
	Have a look at: http://phpics.com/index.php/rss/feed/gallery/album/1/bogus_path/</xss>/ Do a view-source and look for: xss Not really an XSS and I don't see it as a security issue since it isn't HTML and I doubt any browsers will run any injected Javascript there, but it probably still isn't a good idea to allow users to inject arbitrary xml tags into the Gallery3 feeds. -Rasmus
	XUser login Username: * Password: * Create new account Request new password
bharat Joined: 2002-05-21 Posts: 7985	Posted: Fri, 2010-01-15 03:46
	What version of Gallery 3 are you using? I can't reproduce that with the latest version from Git. If you're not using the latest, can you upgrade and see if that resolves it for you? --- Problems? Check gallery3/var/logs bugs/feature req's \| upgrade to the latest code \| use git

Rasmus Joined: 2003-11-25 Posts: 46	Posted: Sun, 2010-01-17 22:47
	Just did a git pull now. I am still seeing it. And looking at the awful spaghetti framework code it is not easy to see where it comes from, but I suppose it is from the Router::current_uri which I don't see any filtering on.

Rasmus Joined: 2003-11-25 Posts: 46	Posted: Mon, 2010-01-18 06:28
	Had a closer look. This is in the feed.mrss.php view. It uses $feed->uri which comes from this in the controller: $feed->uri = url::abs_site(str_replace("&", "&", url::merge($_GET))); That line of code alone should send out all sorts of warning signals. Replacing & with & is not how you urlencode stuff that is going to end up in a link. But, following along here, url::merge($_GET) returns $current_uri with all the GET args merged and concatenated to it: public static function merge(array $arguments) { if ($_GET === $arguments) { $query = Router::$query_string; } elseif ($query = http_build_query(array_merge($_GET, $arguments))) { $query = '?'.$query; } // Return the current URI with the arguments merged into the query string return Router::$current_uri.$query; } And tracking back to where Router::$current_uri is set, we see it comes from the find_uri() method in the Router. And since it is using PATH_INFO, it comes from this line of code: if (isset($_SERVER['PATH_INFO']) AND $_SERVER['PATH_INFO']) { Router::$current_uri = $_SERVER['PATH_INFO']; } So, $current_uri is the raw PATH_INFO which in this case is "/rss/feed/gallery/album/1/bogus_path/</xss>/" I see no filtering anywhere in this chain of following this PATH_INFO data through the framework soup. It really feels like there are too many moving parts here and no clear barrier between dirty user data and data that ends up being used in views. -Rasmus

bharat Joined: 2002-05-21 Posts: 7985	Posted: Mon, 2010-01-18 19:14
	Hey, you recommended this framework (back when it was CodeIgniter). We're just following your lead You're right -- that particular line should have set off lots of warning bells when I did my code review. I did some forensics and realized why it didn't.. Kohana used to run htmlspecialchars() on the request path as part of the framework, but they did away with that in http://dev.kohanaphp.com/issues/1712 and we didn't come back and close the loop here. I wasn't seeing this on my various boxes because I'm using the default Suhosin setup with Debian and it's sanitizing the request path for me, so </xss> is converted to ??xss?. With Suhosin off, I am easily able to reproduce the problem. We expect Kohana not to do XSS sanitization on the path, only on the get and post parameters, and those are validated a second time in all our controller and view code. In most cases this is not a problem because we build our own paths from known-good data. But specifically url::current() and url::merge() were vulnerable. Filed as https://sourceforge.net/apps/trac/gallery/ticket/983 Fixed in http://github.com/gallery/gallery3/commit/0dc184e99f0ca607774a68257432a9a981f4d5b7 Let me know if you can think of a better solution. Also, I'd be curious to hear how you discovered this issue, especially if you've got some automated tools we can use --- Problems? Check gallery3/var/logs bugs/feature req's \| upgrade to the latest code \| use git

Rasmus Joined: 2003-11-25 Posts: 46	Posted: Fri, 2010-01-29 22:49
	Blah, everyone seems to think I am a big fan of CodeIgniter/Kohana. I prefer Kohana the same way I prefer the shortest and least rusty of 10 nails that someone is about to hammer through my eyeball. I found it by pointing my XSS scanner at it. But no, the tool isn't available. By the way, http://gallery.menalto.com/node/93569 is still an issue in the current git code. -Rasmus

bharat Joined: 2002-05-21 Posts: 7985	Posted: Sat, 2010-01-30 20:12
	Yeah the CodeIgniter guys made a big deal out of it. From a product perspective I can say that it sucks to write your own framework, and it's pretty hard to develop a portable framework with a thriving developer community without something to base it on. Kohana works pretty well for us. Thanks for the XSS info. If you turn up any other XSS vulnerabilities with your scanner, please let us know! --- Problems? Check gallery3/var/logs bugs/feature req's \| upgrade to the latest code \| use git

RSS Feed tag injection problem

XUser login