Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
<img src="/files/../logout" />
is allowed and would log people out.
Comment | File | Size | Author |
---|---|---|---|
#30 | filter_image_local.29-git-rename.30.patch | 10.94 KB | sun |
#29 | 1274838_29.patch | 18.1 KB | chx |
#25 | 1274838_dom_with_bonus_red_x.patch | 2.96 KB | chx |
#24 | 1274838_dom.patch | 2.49 KB | chx |
#22 | x.txt | 1.72 KB | chx |
Comments
Comment #1
webchickOuch!
Comment #2
dww(thanks to everyone who's worked on this filter, this is going to be great when completed and deployed!)
Comment #3
webchickHm. I tried to write a test for this, but something in SimpleTest keeps re-writing my ../../filename.whatever to ../filename.whatever. :(
Comment #4
webchickOops.
Comment #5
chx CreditAttribution: chx commentedI 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.
Comment #6
webchickI'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.
Comment #7
chx CreditAttribution: chx commentedThis one uses the red X from misc for bogus images. Otherwise completely unchanged.
Comment #8
webchickThis 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!
Comment #9
scor CreditAttribution: scor commentedmaybe add a infotip on the X image saying the image path was wrong / not recognized (otherwise I'd think I uploaded the wrong image).
Not sure that works if the @src is http and the host is https. Unfortunately cannot test since devdrupal.org does not support https.
Comment #10
sunOne 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.
Comment #11
chx CreditAttribution: chx commentedSolving 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 :)
Comment #12
chx CreditAttribution: chx commentedSomeone 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.
Comment #13
webchickThat is so effing bad-ass. Deployed on the sandbox for testing.
Comment #14
chx CreditAttribution: chx commentedComment #15
webchickHeh. ;)
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!
Comment #16
dwwYay 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++. ;)
Comment #17
webchickHere'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?
Comment #18
chx CreditAttribution: chx commented@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 :)
Comment #19
sunLet's rename to $drupal_root.
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.
Needs a preg_quote() on the hostname.
Tab.
Comment #20
chx CreditAttribution: chx commentedOh 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.
Comment #21
webchickCan 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.
Comment #22
chx CreditAttribution: chx commentedWith 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.
Comment #24
chx CreditAttribution: chx commentedComment #25
chx CreditAttribution: chx commentedComment #26
webchickLatest patch is up on the staging server.
Comment #27
gregglesAnd 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.
Comment #28
webchickSorry, I do not have commit access to this project. :(
Comment #29
chx CreditAttribution: chx commentedRenamed, tested, lovely!
Comment #30
sunSame as #29 with git config renames = copies for way easier review ;)
Comment #31
sunRemoved that prior to commit.
Fixed the string concatenation here prior to commit.
Reverted to an array prior to commit.
Changed that into an array() + implode("\n", $comment) prior to commit in order to review POST data with the actual result.
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". ;)
Comment #32
webchickYeaaahhh!!! I will deploy this for testing as soon as I get to my hotel! :)
Comment #33
webchickCool, deployed.
Also left an update at the "mother" issue: http://drupal.org/node/528682#comment-4996148
Comment #34.0
(not verified) CreditAttribution: commentedfix code markup