Problem

  • It took several weeks of active development, multiple security team reviews, and overall, years to allow stupidly simple images in user input onto drupal.org.
  • 99.9% of all Drupal site builders and administrators aren't even aware of possible security issues.

Goal

  • Allow end-users to use images on Drupal sites.
  • Ensure that Drupal sites are secure.

Details

Proposed solution

  1. Move the local image input filter into core for Drupal 8.
  2. Keep it as a separate filter, since its functionality is distinct from the HTML filter functionality. (It is possible to combine it with alternative HTML filters like HTMLPurifier, and it's even possible to run the filter on Markdown input [after Markdown processing].)

Notes

  • This patch moves the functionality into Drupal core. A direct copy and paste without changes, except for API changes since D6.
  • In order to make this happen for D8, I'd highly recommend to get this base functionality in first. Optimize and revise the user experience, along with a possible integration into HTML Filter (Limit allowed HTML tags) in a separate issue, afterwards.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

+++ b/core/modules/filter/filter.module
@@ -1678,5 +1686,58 @@ function _filter_html_escape_tips($filter, $format, $long = FALSE) {
+  $local_dir = getcwd() . '/';

Forgot to replace this getcwd() with DRUPAL_ROOT.

Status: Needs review » Needs work

The last submitted patch, drupal8.filter-html-image-secure.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
sun’s picture

This patch is complete in terms of features and tests. RTBC, anyone?

sun’s picture

Status: Needs review » Needs work

The last submitted patch, drupal8.filter-html-image-secure.1.patch, failed testing.

Everett Zufelt’s picture

Issue tags: +Accessibility

+ $alt_text = t('Only local images are allowed.');

I'm not sure that this is the best alt text, hopefully others will provide feedback.

When drafting alt text the question is: what text would be required if the image were missing. Interestingly here we are drafting alt text about an image that represents a missing image (which is missing for a particular reason).

sun’s picture

Also, this should probably be themeable:

+++ b/core/modules/filter/filter.module
@@ -1678,5 +1686,58 @@ function _filter_html_escape_tips($filter, $format, $long = FALSE) {
+    // Replace invalid images with an error indicator.
+    $image->setAttribute('src', $base_path . 'misc/watchdog-error.png');
+    $image->setAttribute('title', $alt_text);
+    $image->setAttribute('alt', $alt_text);
andrewmacpherson’s picture

Re: comment #7

That's a really interesting scenario, Everett. Here are some thoughts of mine.

  1. Indicate which image has been removed, e.g. using the file name.
  2. Indicate the reason the image has been removed (or replaced), e.g. security.
  3. Indicate how the problem can be corrected, e.g. images must be local.
  4. Some of this information is supplemental, rather than alternative. The alt text isn't going to be perceivable by sighted content editors who aren't using AT, but the reasons for removal are relevant to all. Could we use a title attribute here? Or an additional link to some help text? longdesc might have been useful, but it was removed in HTML5.
  5. Include the image filename in the title attribute, to avoid duplicate titles.
  6. For a drupal input filter, more guidance can be included in the filter tips. Perhaps dynamically generate this based on the text format's configuration.

So for the local image filter, we might have something like this:

  <img src="image-removed.jpg" alt="image001.jpg removed" title="image001.jpg has been removed. For security reasons, only images from the local domain are allowed. See the filter tips for more details." />

[Edit:] on further thought, the text presented would depend on whether we're looking at a preview in an edit tab, or the view tab.

mgifford’s picture

Can someone re-roll this patch?

mgifford’s picture

The solution in #9 looks good to those of us at the A11ySprint. Changing tag.

sun’s picture

Status: Needs work » Needs review
FileSize
8.19 KB

Re-rolled against HEAD.

Includes better alt/title attribute as suggested in #9, but without the original image filename/URL.

sun’s picture

Status: Needs review » Needs work
Issue tags: +Needs accessibility review

The last submitted patch, drupal8.filter-html-image-secure.12.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
8.28 KB

Updated test for new $modules property.

sun’s picture

Removed irrelevant information about error condition from filter tip.
Fixed comments, use proper PHP multibyte wrapper functions, and strict string comparison.
Added a CSS class to aid in styling, and an alter hook for the invalid image DOMElement.
Replaced alter hook with a theme function.
Cleaned-up test comments.

sun’s picture

FWIW, I'm pretty happy with this code now.

Includes a theme function as mentioned in #8 (albeit a bit special).

Still comes with tests, and passes them. :)

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Tagging.

sun’s picture

Feedback, anyone?

mgifford’s picture

I'd like to see this in D8. Will note that the bulk of the description is actually going into the title rather than the alt text.

+  $image->setAttribute('alt', t('Image removed.'));
+  $image->setAttribute('title', t('This image has been removed. For security reasons, only images from the local domain are allowed.'));

I'm also tagging this as security. Ultimately this should get more review from that team rather than the accessibility folks.

I applied this patch, but my tests of it didn't seem to give me the effect I wanted. I still saw the LOL cat I pasted into my full html (with that filter applied).

sun’s picture

Regarding security, this code is still identical to the contrib module, which went through extensive security team reviews already, due to our active usage on drupal.org. Therefore, I don't think there's any need to redo that audit.

I applied this patch, but my tests of it didn't seem to give me the effect I wanted. I still saw the LOL cat I pasted

That is odd. Are you sure that you configured the filter first? (Alternatively, flushed [field/content] caches after enabling the filter?) The correct behavior is fully covered by tests, so there must have been something wrong in the manual testing.

mgifford’s picture

FileSize
17.03 KB

I should have been clear that I wasn't asking for this code to specifically have a security review, but that this has security implications (by tightening up Drupal's security). Maybe there's a way to reach out to others who are watching issues related to security.

I'm curious if there's a better explanation of the security problem than:
https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet

<img src="http://cdn.ebaumsworld.com/mediaFiles/picture/27619/886408.jpg" />
<img src="http://drupal8.oc/core/themes/bartik/logo.png"  title="d8 - local site" />
<img src="/core/themes/bartik/logo.png"  title="local" />
<img src="http://drupal7.oc/themes/bartik/logo.png/" title="d7 on same box" />

EDIT: Hit submit a bit early. I had yet to say that when I installed this from scratch & cleared caches then it all worked as expected. The code above was used in my testing where the results are attached below as a local image...

sun’s picture

Awesome! Thanks for the positive manual testing confirmation! :)

From my perspective, we just need an RTBC here.

As mentioned in the issue summary, let's not get into the architectural purity trap. I am fully aware of all related issues, which may or may not change the filter system to allow for such filters to run in a more sophisticated text processing phase. However, those issues are anything but complete, and they not even have remotely complete/committable patches. I am also aware that exposing this filter as an own, separate filter is suboptimal, but as mentioned and clarified in the issue summary, it would be wrong to limit this filter to the (Allowed) HTML filter.

The primary purpose is to increase security on Drupal sites, whatever it takes. What's good enough for Drupal.org is good enough for 200,000+ Drupal sites. Needless to say, the mere exposure of this filter increases sensibility for security.

mgifford’s picture

I'm happy to do it, but think it would be beneficial to get someone else involved first. @grendzy seems like a logical choice to give it one more look over. Agreed about making more sites secure. I'd done a review of security modules recently and totally missed Local image input filter.

sun’s picture

#16: platform.filter-image.16.patch queued for re-testing.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in Core.

webchick’s picture

Awesome, really happy to see this hit RTBC!

Unfortunately, we are currently over thresholds, so this patch can't be committed until we start closing down some criticals. :(

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, we are under thresholds, for the moment anyway! :)

This code was extremely thorough reviewed when we added it to Drupal.org, so I'm not too worried there. I eyeballed it again and it seems to be all the same stuff. This is a really useful addition, because it allows people to enable the img tag on their sites without worrying about being totally pwned. Therefore...

Committed and pushed to 8.x (along with a CHANGELOG.txt entry). Thanks! I expanded the commit message to credit everyone listed in the commit log at http://drupalcode.org/project/filter_html_image_secure.git/shortlog. I seem to remember some others, too, but don't recall atm.

Wim Leers’s picture

This is GREAT stuff. (If I could make that "GREAT" red, bold and blinking, I would.) Thank you, @sun et al :)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.