Prevent logout CSRF, rip host, allow any image inside this Drupal

Ouch!

(thanks to everyone who's worked on this filter, this is going to be great when completed and deployed!)

Hm. I tried to write a test for this, but something in SimpleTest keeps re-writing my ../../filename.whatever to ../filename.whatever. :(

Oops.

I have completely rewritten this module. Now we are allowing http://drupal.com/files/foo.jpg as well as /files/foo.jpg. I believe it's somewhat easier to see what is happening: we just iterate over the images and change the src if it looks nasty.

Edit: we enforce images as well as a bonus.

I've applied chx's patch on http://issue-summaries-drupal.redesign.devdrupal.org/ (user/pass: drupal/drupal, then log in as bananas/bananas)

http://issue-summaries-drupal.redesign.devdrupal.org/node/1237686#commen... is my old test file.

Both relative and absolute URLs are working. scor's CSRF use case is stripped out.

This one uses the red X from misc for bogus images. Otherwise completely unchanged.

This is looking GREAT now, btw. Instead of showing nothing on invalid images, now it looks like this:

That'll at least point people that something's wrong, and where. Coupled with #1274888: Add filter tip to explain what's happening with images for documentation and I think this could really work!

+++ b/filter_image_local.module
@@ -33,125 +33,45 @@ function filter_image_local_filter($op, $delta = 0, $format = -1, $text = '') {
+  // Some preparation work. We do not want to run all this strlen for every
+  // image.
+  $base_root_length = strlen($base_root);
+  // Host, slash terminated. We add a slash to make sure you are not using
+  // http://exampleevil.com/files/foo.png.
+  $host = "$base_root/";
...
+      $src = $img['src'];
+      // Remove the host, if given.
+      if (substr($src, 0, $base_root_length + 1) == $host) {
+        $src = substr($src, $base_root_length);
+      }

One of the primary stated goals of the infra team was that they don't want images to contain a protocol or domain, in particular with regard to switching d.o to HTTPS soonish.

This code is going to support http://drupal.org/ when being on that protocol+host, but if the filter happens to be executed on a different, images are removed.

Same applies to https://drupal.org/, only vice-versa.

Admittedly, I should have also added an test assertion for that.

Solving neither is hard. Bonus: 8 lines less code. If you want the image itself to be an explanation I am afraid you need someone to draw it :)

Someone posted a very very big image in the test thread raising the question whether can we do anything to make it less obtuse. Why, of course we can. We already ran getimagesize over the image, using it is child's play. This will not resize the image server size -- this is a filter module and a filter module actually going out and changing user data is just so wrong. We can slap HTML attributes over it and that's it.

That is so effing bad-ass. Deployed on the sandbox for testing.

Heh. ;)

For context, I had asked about #1274928: Allow *any* images hosted on this domain?, and it turns out fixing that involves just removing code. Since the main reason not to allow images outside the /files directory was exploitation of /logout, and since as we can see at http://issue-summaries-drupal.redesign.devdrupal.org/node/1237686#commen... this now allows images to be referenced from the /misc directory (or the theme, or whatever), because the code now does validation around "is that thing actually an image?" I actually think we want that, because showcase graphics, etc. for example used to be checked into the theme (not sure if they still are) and they're referenced in handbook pages. But if this is contentious, we could always add that restriction back.

I want to have a crack at a bit better comments, and also roll in #1274888: Add filter tip to explain what's happening with images to this patch so there's just one bit of code to deploy.

For now, supper!

Yay for progress.

For the record, I'm fine with *not* making a single monolithic patch in here for everything. It's a sandbox. Just let chx, sun, and anyone else push fixes as appropriate. When we're all happy and ready to deploy, we're going to have a 6.x-1.0 release from a promoted official project to feed to jenkins and our bzr-based deployment system. Small patches for discrete issues++. ;)

Here's a patch that cleans up the comments a bit, so someone like me can understand the code flow a bit better now. chx checked them over and gave them a thumbs-up!

sun, I think it's probably time to commit this patch to the sandbox, assuming you don't violently disagree with the XML direction, so we can spin off other more targeted issues on things without bumping into conflicts on the re-write. Would that be ok?

@dww we already have a spinoff in #1275424-3: Deal with documentation role and documentation input format so no, we are not making this the kitchen sink :)

+++ b/filter_image_local.module
@@ -33,125 +29,74 @@ function filter_image_local_filter($op, $delta = 0, $format = -1, $text = '') {
+  $local_dir = getcwd() . '/';

Let's rename to $drupal_root.

+++ b/filter_image_local.module
@@ -33,125 +29,74 @@ function filter_image_local_filter($op, $delta = 0, $format = -1, $text = '') {
+  @$htmlDom->loadHTML($text);
...
+    // Convert to XML and search for <img> tags. We use SimpleXML because
+    // working with DOM manipulation is extremely complicated and ugly.
+    $simplexml = simplexml_import_dom($htmlDom);
...
-function filter_image_local_dom_load($text) {
...
-  @$dom_document->loadHTML('<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"><html xmlns="http://www.w3.org/1999/xhtml"><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8" /></head><body>' . $text . '</body></html>');

Skipping the doctype, content-type, and charset will lead to string garbage under various conditions.

There was a reason for backporting this code from D7 -- we went through this entire pain already; it took plenty of brainpower and energy from some of the most skilled core developers to get it right and make it work under all circumstances.

At the very least, we should retain the dom loader helper. But TBH, I don't understand why we're even trying to implement the loading and serializing differently, given that we already know all of the pitfalls involved with it.

+++ b/filter_image_local.module
@@ -33,125 +29,74 @@ function filter_image_local_filter($op, $delta = 0, $format = -1, $text = '') {
+      $img['src'] = preg_replace('|^https?://' . $_SERVER['HTTP_HOST'] .'|', '', $img['src']);

Needs a preg_quote() on the hostname.

+++ b/filter_image_local.module
@@ -33,125 +29,74 @@ function filter_image_local_filter($op, $delta = 0, $format = -1, $text = '') {
+        // Remove the base path() to get the path relative to the Drupal root.
+	// Also, make sure we're dealing with a real image here.

Tab.

Oh dear god. I was completely unaware. Now, what sun means is that #374441: Refactor Drupal HTML corrector (PHP5) refactored the HTML corrector for the DOM extension and #542742: Filter: Create wrapper functions to load/serialize a DOM. moved it to wrapper functions. I never knew such unholy ugliness found its way into Drupal 7. We were working like on different planets because meanwhile, you know, simpletest was using the pattern I had used in here. Oh well. I will work on this a little bit.

Can you provide a test case on http://issue-summaries-drupal.redesign.devdrupal.org/ that breaks with the current code? I'm not real inclined to change the code back to the version that had a CSRF bug and was 4x as long and complicated.

With some effort I wrote a test script that combined with a cookie exporter managed to post invalid UTF-8. The text got truncated dead cold at that point. It might have been an earlier filter for sure. But, we see that invalid UTF-8 won't faze it. So what will?

Edit: I fixed the comment in the script by adding the close > to the first img. But then looking at the source of http://issue-summaries-drupal.redesign.devdrupal.org/node/1237686#commen... it seems that even posting invalid UTF-8 is impossible. I have reviewed what cURL sends by adding an echo $post to the drupalPost and I get something like this (edited for brevity): test+%3Cimg+src%3D%22%2Ffiles%2F..%2Flogout%22%3E%E1%E9%FB%F5%3Cimg+src invalid UTF-8 gets on the wire but it's truncated. Probably MySQL simply refuses to store it. So in our actual use case... things just work.

Edit: earlier I tried to post invalid HTML but that does not faze much the system either.

Latest patch is up on the staging server.

Latest patch is up on the staging server.

And committed to the sandbox? It's easier for me to review if it's in the sandbox as well.

http://htmlpurifier.org/ has unit tests for a bunch of XSS. We should probably include those (or many of them) into our testing - they could also help us with this issue.

Sorry, I do not have commit access to this project. :(

Renamed, tested, lovely!

Same as #29 with git config renames = copies for way easier review ;)

+++ b/filter_html_image_secure.module
@@ -32,33 +32,60 @@ function filter_image_local_filter($op, $delta = 0, $format = -1, $text = '') {
+  file_put_contents('/tmp/log', '');

Removed that prior to commit.

+++ b/filter_html_image_secure.module
@@ -32,33 +32,60 @@ function filter_image_local_filter($op, $delta = 0, $format = -1, $text = '') {
+    $image->setAttribute('src', preg_replace('|^https?://' . $_SERVER['HTTP_HOST'] .'|', '', $src));
...
+    file_put_contents('/tmp/log', substr($src, 0, $base_path_length). "\n". $base_path ."\n\n", FILE_APPEND);

+++ b/tests/filter_image_local.test
@@ -63,20 +63,59 @@ class FilterImageLocalWebTestCase extends DrupalWebTestCase {
+      $http_base_url .'/misc/druplicon.png' => base_path() . 'misc/druplicon.png',
+      $https_base_url .'/misc/druplicon.png' => base_path() . 'misc/druplicon.png',
...
+      $csrf_path .'/logout' => $red_x_image,
...
+      $comment .= '<img src="' . $image .'" testattribute="'. md5($image) .'" />';
...
+      foreach ($this->xpath('//img[@testattribute="'. md5($image) . '"]') as $element) {

Fixed the string concatenation here prior to commit.

+++ b/tests/filter_image_local.test
@@ -18,7 +18,7 @@ class FilterImageLocalWebTestCase extends DrupalWebTestCase {
-    parent::setUp(array('filter_image_local'));
+    parent::setUp('filter_html_image_secure');

Reverted to an array prior to commit.

+++ b/tests/filter_image_local.test
@@ -63,20 +63,59 @@ class FilterImageLocalWebTestCase extends DrupalWebTestCase {
+    $comment = '';
...
+    $edit = array(
+      'comment' => $comment,
     );
     $this->drupalPost('node/' . $this->node->nid, $edit, t('Save'));

Changed that into an array() + implode("\n", $comment) prior to commit in order to review POST data with the actual result.

+++ b/tests/filter_image_local.test
@@ -63,20 +63,59 @@ class FilterImageLocalWebTestCase extends DrupalWebTestCase {
+          $this->assertEqual((string) $element['title'], t("Only local images are allowed."));

Totally belongs into a separate follow-up issue that I'm already looking forward to dive into with @webchick ;) but yeah, in terms of an end-user perspective, this tooltip reads a bit "technical". ;)

Yeaaahhh!!! I will deploy this for testing as soon as I get to my hotel! :)

Cool, deployed.

Also left an update at the "mother" issue: http://drupal.org/node/528682#comment-4996148