STR: 1) Start Firefox with a new profile (no ad blocker!) 2) Open http://www.nownews.com/2012/10/14/340-2862863.htm (or the reduced testcase) 3) Observe the rendering of the Flash ad between the menu and the article Result: The Flash ad is displayed as random symbols in an area with broken scrollbars. (see my screenshot) m-c good=2012-08-07 bad=2012-08-08 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1bbc0b65dffb&tochange=e55638d4037a Suspected bug: Bug 745030 - Refactor nsObjectLoadingContent Reporter: http://forums.mozillazine.org/viewtopic.php?f=23&t=2572583

The <Embed> doesn't specify a type="application/x-shockwave-flash" and the SWF file is being served as text/plain. I guess that we ended up doing file extension sniffing before... this feels like a dup of a bug from a few weeks ago, also.

This is a dupe of bug 798026. This can occur on 15 as well, so I suspect that bug 745030 simply added a few more edge cases that can hit it. The root of the problem is we should be detecting the stream as binary, regardless of the text/plain MIME. Extension sniffing only occurs if we dont get a MIME, but since we don't remap this to application/octet-stream, we think we have a valid one.

We'll decide on tracking over in bug 798026 which is also nominated.

Reopening. This is a totally different situation from bug 798026, because <embed> vs <object> have completely different behavior. In this case we should be using the file extension, but we're not. Why not?

Loading this testcase I never get a call to IsPluginEnabledByExtension. Looks like nsObjectLoadingContent::UpdateObjectParameters only calls that now when binaryChannelType. Worse yet, there should really be no way for us to instantiate a non-SVG subdocument for an <embed>. We don't have eSupportDocuments in our caps here. So how are we getting a subdocument?

So we land in nsObjectLoadingContent::UpdateObjectParameters, only look at the channel type, get a typeHint of null, and set caseOne and caseTwo both to false. Then we set newMime to the channelType and and press on. So what the old code used to do, in OnStartRequest, was this: 408 if ((channelType.EqualsASCII(APPLICATION_OCTET_STREAM) && 409 !mContentType.IsEmpty() && 410 GetTypeOfContent(mContentType) != eType_Document) || 411 // Need to check IsSupportedPlugin() in addition to GetTypeOfContent() 412 // because otherwise the default plug-in's catch-all behavior would 413 // confuse things. 414 (IsSupportedPlugin(mContentType) && 415 GetTypeOfContent(mContentType) == eType_Plugin)) { 416 // Set the type we'll use for dispatch on the channel. Otherwise we could 417 // end up trying to dispatch to a nsFrameLoader, which will complain that 418 // it couldn't find a way to handle application/octet-stream 419 420 chan->SetContentType(mContentType); So as long as mContentType was something supported by a plugin we'd ignore the channel type. Note that we used to set mContentType like so, back in LoadObject: 1124 if ((caps & eOverrideServerType) && 1125 ((!aTypeHint.IsEmpty() && IsSupportedPlugin(aTypeHint)) || 1126 (aURI && IsPluginEnabledByExtension(aURI, overrideType)))) { 1127 ObjectType newType; 1128 if (overrideType.IsEmpty()) { 1129 newType = GetTypeOfContent(aTypeHint); 1130 } else { 1131 mContentType = overrideType; 1132 newType = eType_Plugin; 1133 } But it looks like the eOverrideServerType flag went away in bug 745030? What replaced it? In any case, per spec and for web compat (which is why the spec is the way it is) extension sniffing should occur always for <embed>, and override the server-provided type if the extension is sniffed to a plugin-supported type... So we need to restore the code that did that.

So basically, for an <embed>, type determination should go as follows: 1) "type" attribute, if that type is handled by a plug-in. 2) URI extension, if that maps to a type handled by a plug-in. 3) Channel type, if it's handled by a plug-in or is SVG. 4) Don't render, fall back. See http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#concept-embed-type And in particular, we should never be rendering non-SVG document types in an <embed>. That seems to be broken too. Testcase: <embed src="data:text/html,<script>alert('Hey there')</script>" width="100" height="100"> That should NOT work. And it working is likely a security bug. :( I guess we could split that out from the URI extension bit if we wanted to treat them as two separate bugs...

This was a dropped check in the refactoring - we properly fall back to extension sniffing if we load a channel and end up with a binary type and no hint, but not for tags that can skip channel loading with plugins

Comment on attachment 673425 [details] [diff] [review] Fix type error in plugin code no comment.

Comment on attachment 673425 [details] [diff] [review] Fix type error in plugin code Yikes. Want to just make it a Capabilities? r=me

Comment on attachment 673427 [details] [diff] [review] Guess plugin types from extension prior to falling back to channel loading I don't think this is quite right. Consider something like this: <embed type="something/other" src="something.swf" width="100" height="100"> With this patch, will this instantiate the Flash plugin? It doesn't look like it, and I'm pretty sure it needs to. See bug 431280 and in particular bug 431280 comment 4. The code in this patch looks pretty much like the code before the fix for bug 431280...

Yuck. Okay. This falls back to the extension if we're lacking a valid type, vs any type (GetTypeOfContent considers capabilities). I'll also make sure the test matrix I'm making for bug 783059 includes these, though I believe I does (it would've caught the document case were it in working order)

(In reply to Boris Zbarsky (:bz) from comment #13) > Yikes. Want to just make it a Capabilities? That would make sense wouldn't it. carrying over r+

Comment on attachment 673495 [details] [diff] [review] Guess plugin types from extension prior to falling back to channel loading Hmm. So with this, something like this: <embed type="image/svg+xml" src="something.swf"> will try to load it as SVG, not as Flash, right? What do other browsers do here? I'm fine with doing this, in general, but if we go with this behavior we should make sure the spec gets changed accordingly...

This would treat SVG (or any images actually[1]) with the same priority as a plugin, e.g. > <embed type="application/x-shockwave-flash" src="something.svg"> would similarly load type first (flash). That might be okay as far as the spec goes; It would be (somewhat) invisible to the content whether the browser or a plugin loaded their image-type. [1] http://dxr.allizom.org/mozilla-central/content/html/content/src/nsHTMLSharedObjectElement.cpp#l490

> That might be okay as far as the spec goes Well, it wouldn't match what the spec says right now... > It would be (somewhat) invisible to the content whether the browser or a plugin loaded > their image-type. The problems start when the tag says: <embed type="image/svg+xml" src="something.swf"> and the file really is an swf. Assuming that works in other browsers, of course; if it doesn't work cross-browser them I'm a lot less worried about the behavior change. In any case, I suggest restoring the behavior we used to have for beta and aurora, to minimize risk, no matter what we do on m-c.

Comment on attachment 673495 [details] [diff] [review] Guess plugin types from extension prior to falling back to channel loading Per comment 19.

Same patch, but we prefer type extension if |type != eType_Plugin| rather than |if type == eType_Null|. This means > <embed type="image/svg+xml" src="something.swf"> and > <embed type="application/x-shockwave-flash" src="something.svg"> Will both load flash without opening a channel, which was our pre-bug 745030 behavior as well.

Comment on attachment 673987 [details] [diff] [review] Guess plugin types from extension prior to falling back to channel loading r=me. Thank you! We should add some tests....

(In reply to Boris Zbarsky (:bz) from comment #22) > Comment on attachment 673987 [details] [diff] [review] > Guess plugin types from extension prior to falling back to channel loading > > r=me. Thank you! > > We should add some tests.... I'm finishing bug 783059 to expand tests for these tags as we speak, and made sure these cases are covered

(In reply to John Schoenick [:johns] from comment #23) > I'm finishing bug 783059 to expand tests for these tags as we speak, and > made sure these cases are covered What sg rating do we think this bug has? Or are we specifically concerned about web regressions? We'd like to figure out whether or not this needs to continue tracking. If this is still a critical issue, we should land on m-c asap and Aurora/Beta before Tuesday to make it into b4.

(In reply to Boris Zbarsky (:bz) from comment #9) > <embed src="data:text/html,<script>alert('Hey there')</script>" width="100" > height="100"> > > That should NOT work. And it working is likely a security bug. :( How so? <iframe src="data:...script..."> isn't a security problem. Is this a "install malware" sec-critical, a universal-xss sec-high, or a "we should really fix this" sec-moderate?

Comment on attachment 673496 [details] [diff] [review] Fix type error in plugin code. r=bz I'm not sure what sec-level this would qualify as, sites that are embedding a remote plugin could be fed a document instead, but if they are including untrusted plugins that seems just as risky, and our type-enforcement (e.g. overriding a .wav with another type) would not be defeated any more than with an alternate plugin type. The actual fix is very simple and low-risk. [Security approval request comment] How easily can the security issue be deduced from the patch? Fairly easily, with any familiarity with this code Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No Which older supported branches are affected by this flaw? 17+ If not all supported branches, which bug introduced the flaw? Bug 745030 or one of its followups Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Applies to all affected How likely is this patch to cause regressions; how much testing does it need? Very low risk, fixes an obvious type error

Comment on attachment 673987 [details] [diff] [review] Guess plugin types from extension prior to falling back to channel loading [Security approval request comment] How easily can the security issue be deduced from the patch? This is actually just a ride-along to fix behavior regressions from bug 745030, the security issue is largely in the other patch. Which older supported branches are affected by this flaw? 17+ If not all supported branches, which bug introduced the flaw? Bug 745030 or followups Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Applies to all How likely is this patch to cause regressions; how much testing does it need? Fixes known regression in simple logic, not likely to cause further regression.

> Is this a "install malware" sec-critical, a universal-xss sec-high, or a > "we should really fix this" sec-moderate? It's largely an XSS issue. And smart XSS stuff probably filters out <embed> anyway. So probably sec-moderate, is my guess.

Comment on attachment 673987 [details] [diff] [review] Guess plugin types from extension prior to falling back to channel loading Let's get it in!

Comment on attachment 673496 [details] [diff] [review] Fix type error in plugin code. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd36719e993

Comment on attachment 673987 [details] [diff] [review] Guess plugin types from extension prior to falling back to channel loading https://hg.mozilla.org/integration/mozilla-inbound/rev/0bbc68396695

https://hg.mozilla.org/mozilla-central/rev/dfd36719e993 https://hg.mozilla.org/mozilla-central/rev/0bbc68396695

Comment on attachment 673987 [details] [diff] [review] Guess plugin types from extension prior to falling back to channel loading [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 745030 or one of its followups User impact if declined: Regression in <embed> tag behavior for loading by file extension. (the other patch addresses the sec issue) Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): Low, simple patch to fix known regression String or UUID changes made by this patch: none

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7cd4fa60ceb https://hg.mozilla.org/releases/mozilla-aurora/rev/52a2c63aeeaa https://hg.mozilla.org/releases/mozilla-beta/rev/5048c50f65ff https://hg.mozilla.org/releases/mozilla-beta/rev/e9ea119c6c2c

Verified on Firefox 19.0 beta 2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0 Build ID: 20130116072953 and Firefox 18.0.1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20130116073211 The Flash ad is displayed correctly on the site provided in the STR from comment 0 and also on the reduced testcase.

Bug 783059 takes care of the missing test coverage here

mass remove verifyme requests greater than 4 months old