Closed Bug 803159 Opened 7 years ago Closed 7 years ago

Type determination does not take URI extension into account for embed, non-SVG documents instantiated for embed

Categories

(Core :: Plug-ins, defect, critical)

17 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 + verified
firefox19 + verified
firefox-esr10 --- unaffected

People

(Reporter: epinal99-bugzilla2, Assigned: johns)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [adv-track-main17-])

Attachments

(4 files, 3 obsolete files)

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
Attached file Reduced testcase
Attachment #672828 - Attachment description: Screenshot FF15 vs FF17+ on Win 7 → Screenshot FF16 vs FF17+ on Win 7
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 798026
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?
Severity: normal → major
Status: RESOLVED → REOPENED
OS: Windows 7 → All
Hardware: x86_64 → All
Resolution: DUPLICATE → ---
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.
Assignee: nobody → jschoenick
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...
Group: core-security
Severity: major → critical
Summary: Flash ad bad decoded since Firefox 17 → Type determination does not take URI extension into account for embed, non-SVG documents instantiated for embed
Attached patch Fix type error in plugin code (obsolete) — Splinter Review
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.
Attachment #673425 - Flags: review?(bzbarsky)
Attachment #673427 - Flags: review?(joshmoz)
Comment on attachment 673425 [details] [diff] [review]
Fix type error in plugin code

Yikes.  Want to just make it a Capabilities?

r=me
Attachment #673425 - Flags: review?(bzbarsky) → review+
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)
Attachment #673427 - Attachment is obsolete: true
Attachment #673427 - Flags: review?(joshmoz)
Attachment #673495 - Flags: review?(bzbarsky)
(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+
Attachment #673425 - Attachment is obsolete: true
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.
Attachment #673495 - Flags: review?(bzbarsky) → review-
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.
Attachment #673495 - Attachment is obsolete: true
Attachment #673987 - Flags: review?(bzbarsky)
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....
Attachment #673987 - Flags: review?(bzbarsky) → review+
(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?
Flags: needinfo?(bzbarsky)
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
Attachment #673496 - Flags: sec-approval?
Attachment #673496 - Flags: approval-mozilla-beta?
Attachment #673496 - Flags: approval-mozilla-aurora?
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.
Attachment #673987 - Flags: sec-approval?
> 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.
Flags: needinfo?(bzbarsky)
Attachment #673496 - Flags: sec-approval? → sec-approval+
Comment on attachment 673987 [details] [diff] [review]
Guess plugin types from extension prior to falling back to channel loading

Let's get it in!
Attachment #673987 - Flags: sec-approval? → sec-approval+
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
Attachment #673987 - Flags: checkin+
Attachment #673496 - Flags: approval-mozilla-beta?
Attachment #673496 - Flags: approval-mozilla-beta+
Attachment #673496 - Flags: approval-mozilla-aurora?
Attachment #673496 - Flags: approval-mozilla-aurora+
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
Attachment #673987 - Flags: approval-mozilla-beta?
Attachment #673987 - Flags: approval-mozilla-aurora?
Attachment #673987 - Flags: approval-mozilla-beta?
Attachment #673987 - Flags: approval-mozilla-beta+
Attachment #673987 - Flags: approval-mozilla-aurora?
Attachment #673987 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-track-main17-]
Group: core-security
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.
Blocks: 783059
Bug 783059 takes care of the missing test coverage here
Flags: in-testsuite? → in-testsuite+
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.