247 bytes, text/html
74 bytes, text/html
1.17 KB, patch
Joe Drew (not getting mail): review+
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:220.127.116.11) Gecko/2008070208 Firefox/3.0.1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:18.104.22.168) Gecko/2008070208 Firefox/3.0.1 I was notified by an user ("Reelix") about a weird behavior with img tags. I wasn't able to find a duplicate, so I thought it might be better to file a report here, despite you probably being aware of the issue. If the request for the image is answered with a response redirecting to a mailto: address, then the default email editor opens. i.e. <img src="<page that redirects to mailto:foo@bar>" /> will cause firefox to open the default email application. or more verbatim: a t.html --snip <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> <html> <head> <title>mail test</title> </head> <body> <img src="t.php" /> </body> </html> --snap Where t.php redirects to mailto:firstname.lastname@example.org?subject=Irritating%20popup&body=I%20d o%20not%20think%20image%20tags%20should%20be%20able%20to%20t his . Opens the email editor. I don't think that image tags should be able to open any external applications and/or cause popups. Thanks for looking into it, cheers ~H Reproducible: Always Steps to Reproduce: 1. write a page with an image tag like <img src="foo.php" /> 2. have foo.php redirect to mailto:email@example.com (header('location:mailto:firstname.lastname@example.org');) 3. View the page Actual Results: The external email editor opens. Expected Results: Broken image.
Confirming. This definitely looks like something that should be forbidden. Cc'ing dveditz for his professional opinion.
Confirmed on windows vista and mac: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20080903034741 Minefield/3.1b1pre Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a2pre) Gecko/20080819152019 Minefield/3.1a2pre sorry for bugspam
I am very sorry about being a pain, but is there any movement on this? Cheers, ~H
Created attachment 340966 [details] Using mailto link in an image This is a test with a image that has a mailto: link as the source, you can see that there is a broken link in this case. So, the question is do we want to detect the redirect and show a broken image in this case or not?
Doesn't look like there's been much movement here (though dveditz didn't get cc'd per comment 2... he's cc'd now). I'm going to set this as sg:low for now, since this is more of a DoS/annoyance than anything.
I have to say: this is potentially a very bad annoyance. Spammers could use this easily to bypass the popup blocker. Current measures like noscript wouldn't help a bit.
Sorry for being a pest, but we ("phpBB") keep getting reports about this being used for spam/popups with an increasing frequency. Cheers, ~H
Hi, I'm sorry, but I have to give this its semi-annual bump. It is by now commonly used in spam entries on forums. I am by now tempted to contact the media about the issue. I'm sure that there are many pressing problems, but this renders the popup blocker useless and is developing into a real problem. Cheers, all the best, ~H
Confirmed on Ubuntu 9.10 with Firefox 3.5.7 opening Empathy mail client.
 The Evolution mail client, not Empathy.
I'm going to take a stab at this.
Comment on attachment 426017 [details] [diff] [review] fix Don't hardcode protocol names. Instead, I think you want to check for the URI_DOES_NOT_RETURN_DATA protocol handler flag. (Something like http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsViewSourceHTML.cpp#1074)
Or maybe URI_NON_PERSISTABLE is better.
In fact, this should be using the same code as nsNoDataProtocolContentPolicy::ShouldLoad. Heck, ideally it should be _calling_ that ShouldLoad.... As well as perhaps other content policy methods. Sadly, content policy doesn't really mesh that well with redirects.
My first patch copied code from nsNoDataProtocolContentPolicy::ShouldLoad, in fact. I agree that the ideal fix would be to call that function, but we lack the context we need to do so during the redirect. Shall we go forward, then, with the patch suggested in comment 14?
> My first patch copied code from nsNoDataProtocolContentPolicy::ShouldLoad No, it copied _part_ of that code. The fast-path. You either need to copy all the code or just copy the non-fast-path part (depending on how performance-critical we think image redirects are). I'm not entirely convinced that the fast-path is even important in ShouldLoad there, btw.
Created attachment 426038 [details] [diff] [review] fix v2
So questions: 1) Why the original URI and not the URI? 2) Does this not leave the issue for stylesheet loads, script loads, etc?
(In reply to comment #20) > 1) Why the original URI and not the URI? I don't have a reason why other originalURI was being used in another check in this function already. It makes more sense to use URI since that will take into account URI resolution. > 2) Does this not leave the issue for stylesheet loads, script loads, etc? It does leave this problem for those other types of loads. I think the reason this bug was filed with images in mind is that this is a particular problem for forums and such who allow users to post images, but not necessarily those other types of content.
Ah, I see. OK, then this is ok for now but we should get a followup bug filed to tag loads that shouldn't permit no-data stuff somehow and just have the global redirect observer enforce this restriction.... or something.
Comment on attachment 426038 [details] [diff] [review] fix v2 r=me if you use the URI instead of the originalURI.
Created attachment 426047 [details] [diff] [review] fix v3 Addresses bz's comments.
Boris, re: comment 22, there is bug 456957, which is to make Content Policy work with redirects. Is that sufficient, or shall I open up a follow-up bug to tag loads (channels?) with dont-allow-nodata and have a redirect observer set up to enforce those?
should imagelib really use a DOM error code?
I have no attachment to that particular error code. I'm open to other suggestions.
Comment on attachment 426047 [details] [diff] [review] fix v3 I'd probably just go with NS_ERROR_ABORT, or if you prefer a new error code added to ImageErrors.h.
Created attachment 426262 [details] [diff] [review] fix v4 Changed error code. Carrying forward Boris' r+.
> there is bug 456957, which is to make Content Policy work with redirects. Is > that sufficient, I'd prefer a separate bug with narrower scope to handle just the no-data thing, I think.
Filed follow-up bug 545871.
Comment on attachment 426262 [details] [diff] [review] fix v4 a=beltzner for all branches, why aren't there tests? I bet you'll add some tests. :)
Comment on attachment 426262 [details] [diff] [review] fix v4 This missed the deadline.
Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:22.214.171.124pre) Gecko/20100315 Namoroka/3.6.2pre (.NET CLR 3.5.30729) using attached testcase. Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:126.96.36.199pre) Gecko/20100311 Shiretoko/3.5.9pre (.NET CLR 3.5.30729).
mustlive posted on bugtraq that other browsers are affected http://www.securityfocus.com/archive/1/511327
I would argue that the report by mustlive is less critical/ of a vulnerability. Posting images is a common feature for "normal" users on blogs, forums etc. The img src vector allowed circumventing the popup blocker and DoS on such sites. By contrast, being able to post an iframe with an arbitrary source is enough for conducting far worse attacks on the site and its visitors - including DoS, but extending to attacks on common plugins, XSS and worse. Cheers, ~H