336903 - New feed view run under chrome context

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect, P1)

Description

[Martijn Wargers (dead)]

New feed view run under chrome context. This is very dangerous, as you can see with the url testcase.
When clicking on the link, it opens a new tab with google.com inside. That should not be possible.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase

Comment 2

[Daniel Veditz [:dveditz]]

This should block 2.0a2

Comment 3

[Daniel Veditz [:dveditz]]

The main problem is that the feed viewer page has chrome privilege rather than using the feed URL as the origin (though that's the page's document.location, so it's partly correct). It is probably easier and safer to make sure the entire page has the correct privilege than to deny javascript: on each link.

Not sure what kind of rich-content support the feed-viewer has, but relying on perfect sanitization to protect against <script></script> in content worries me. Fixing the page's privilege would provide back-stop protection against that kind of failure too.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So does adding:

  chromeChannel.owner = null;

at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/feeds/src/FeedConverter.js&rev=1.5#210 fix this?  It ought to...

Ideally, we'd set this to the owner of the original channel; Ben might know how we can get that here.

Comment 5

[Daniel Veditz [:dveditz]]

The problem with that is that the page is really subscribe.xhtml and needs chrome privs to do its thing. I don't think a trivially easy fix like that will work, the subscription controls must be segregated from the massaged feed content.

Comment 6

[Daniel Veditz [:dveditz]]

yup, adding channel.owner=null fubars the page.

It does fix the security hole however ;-)

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  So let's try this the other way.  Right now what we do is http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/feeds/content/subscribe.js&rev=1.6&mark=176-178#170

Perhaps instead of that we should use something more like:

  var a = document.createElementNS(HTML_NS, "a");
  a.appendChild(document.createTextNode(entry.title));
  a.setAttribute("href", "javascript:doLoad(" + entry.link.spec + ");");

and have:

function doLoad(uri) {
   checkLoadURIStr(document.documentURI, uri, DISALLOW_SCRIPT_OR_DATA);
   // Then do the load by setting document.location or whatever
}

Would that work?

Comment 8

[Martijn Wargers (dead)]

I guess that could work.
But wasn't there concern that unprivileged code could open windows with privileged code (like about:config, etc). That has been fixed, not? But with this feed view thing, it would be possible again, just do window.open on any feed url.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Looking, will have a patch right quick. 

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: another test

uses the title image link

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: image src test (crashes 1.8 branch builds on load)

uses the image src for the title image

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 221259 [details]
image src test (crashes 1.8 branch builds on load)

This crashes branch builds on load because bug 328697 wasn't landed there yet.

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch, check every uri attribute set.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 221261 [details] [diff] [review]
patch, check every uri attribute set. 

ignore the microsummaries aprt, it was so my tree would build

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221261 [details] [diff] [review]
patch, check every uri attribute set. 

r=bzbarsky, except the makefile change.  Do run this whole thing by Jesse, though...

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

OK, I have landed this fix on the trunk and the 1.8 branch... I'm going to ask Jesse to take a look at this and see if he can see anything else that might be problematic.

Comment 17

[Daniel Veditz [:dveditz]]

Martijn is correct in comment 8 -- leaving this page privileged means any same-origin violation XSS bug is trivially elevated to an arbitrary code execution bug. It also means the feed sanitization has to be perfect.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Retargetted at beta1

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: pass one

moves content generation and utilities into a service, and makes this an about safe page without chrome privs. (this was dveditz' suggestion). The MOZ_PHOENIX in  netwerk/caps is highly unfortunate. We should make AboutRedirector and the about:safe stuff parameterizable.

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: pass two

don't add MOZ_PHOENIX to netwerk if we don't have to

Comment 21

[Boris Zbarsky [:bzbarsky]]

> We should make AboutRedirector and the about:safe stuff parameterizable.

I've had a patch that does that waiting on dveditz's review for about a month in bug 337746... Not that his helps you on branch, of course.

I'm not sure what the purpose of the docshell GetInterface change is.  Why is that needed?  Pretty much any use of GetInterface is an undiscoverable hack, imo... Is the problem that nsIDocument is not scriptable?

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

Yeah, I need to get at the channel so I can do a rudimentary check on the "actual" URI of the page so as to prevent this from being called from pages other than the preview page. 

Is there a better way to do this branch and trunk? I think access to the channel could be useful - I can't remember the exact case but I remember wishing for this in the past.

Comment 23

[Boris Zbarsky [:bzbarsky]]

On trunk, we could use the same setup as we'll probably end up using for nsIURI and nsIPrincipal, I guess, but that doesn't really work for branch.

The problem I have is that it doesn't really make sense to get the channel off the docshell and get the channel of the last-loaded document (as opposed to, say, the currently-loading channel).  It's prone to nasty side effects, and impossible to remove once we add it, since we have no way to tell who's depending on it.

Darin, biesi, any idea how we could expose the channel here in a sane way?

I also wonder what you mean by "actual" URI.  Does this differ from the nsIWebNavigation's current URI somehow?

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

Yes, the "actual" URI is a jar: URI of the preview page. At some point, code in docshell and friends begin using the channel's originalURI for updating UI, etc, and originalURI is the web feed URI.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I see.  Perhaps we just need to expose that URI somehow, then.

Comment 26

[Darin Fisher]

> Darin, biesi, any idea how we could expose the channel here in a sane way?

Put it on nsIDocShell (or the Mozilla 1.8 branch version that we can edit).  You could have getters for the previous channel as well as the one that is currently loading.  It might be useful to have the full channel instead of just a URI.

Comment 27

[Ben Goodger (use ben at mozilla dot org for email)]

Boris, what do you think of this? If you like the idea I'll revise my patch.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I'm happier adding an nsIChannel prop on nsIDocShell, yes.  At least that way we can find the consumers if we find a better way to expose it (e.g. on the document, on trunk).  Ben, if you would do that, that would be great. 

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch for browser code

Comment 30

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: docshell portion (trunk only)

Adds currentDocumentChannel attribute to nsIDocShell

Once this patch is reviewed I will create a branch patch that adds this to the nsIDocShell_MOZILLA_1_8_BRANCH interface.

Comment 31

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: caps portion

adds about:feeds to SSM to show about safe feed preview page

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 226399 [details] [diff] [review]
docshell portion (trunk only)

>Index: docshell/base/nsDocShell.cpp
>+nsDocShell::GetCurrentDocumentChannel(nsIChannel** aResult)

>+    nsresult rv = EnsureContentViewer();

Why not just return null if there's no viewer?  That would make a lot more sense to me.

You seem to never set *aResult when there's no document; that's wrong.

>Index: docshell/base/nsIDocShell.idl

>+   * Gets the channel for the currently loaded document.

Add ", if any" at the end.

r=bzbarsky with those nits picked.

Comment 33

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: revised trunk docshell patch

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: revised trunk docshell patch

Comment 35

[Darin Fisher]

Comment on attachment 226423 [details] [diff] [review]
revised trunk docshell patch

It might be good to add some comments about when currentDocumentChannel changes.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Er... that's still calling EnsureContentViewer().  Just take that line out.  ;)

Comment 37

[Ben Goodger (use ben at mozilla dot org for email)]

OK!

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 226400 [details] [diff] [review]
caps portion

r=dveditz

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 226400 [details] [diff] [review]
caps portion

Do you really want "about:feeedsaroo" to execute scripts?

Comment 40

[Ben Goodger (use ben at mozilla dot org for email)]

good call, revised patch coming.

Comment 41

[Mark Mentovai]

Comment on attachment 226398 [details] [diff] [review]
patch for browser code

Some of your functions begin with FH_ and others with SH_ - consolidate.

nsAboutFeeds on the trunk needs a new GetURIFlags method which should set its result to 0, following bug 337746.

Comment 42

[Ben Goodger (use ben at mozilla dot org for email)]

Will have the final trunk patches in place tomorrow morning. Will land then get branch patches ready. 

Comment 43

[Boris Zbarsky [:bzbarsky]]

Actually, GetURIFlags should return ALLOW_SCRIPT, I'd guess.

Comment 44

[Ben Goodger (use ben at mozilla dot org for email)]

yeah, URI_SAFE_FOR_UNTRUSTED_CONTENT | ALLOW_SCRIPT

rebuilding final trunk patch now...

Comment 45

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: final trunk pieces for checkin, browser

Comment 46

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: final trunk pieces for checkin, docshell

Comment 47

[Ben Goodger (use ben at mozilla dot org for email)]

Landed on the trunk. Will now make a branch patch. 

Comment 48

[Boris Zbarsky [:bzbarsky]]

So does untrusted content really need acess to the about:feeds pages?   That is, why was the GetBaseURIScheme change needed in the original patch?

Comment 49

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: branch browser patch

Comment 50

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: branch caps patch

Comment 51

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: branch docshell patch

Same as the other patch, but the attribute is on nsIDocShell_MOZILLA_1_8_BRANCH instead. Requesting quick re-review for this minor change for the branch.

Comment 52

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: ook. the right docshell patch

fix merge error

Comment 53

[Darin Fisher]

Comment on attachment 227460 [details] [diff] [review]
ook. the right docshell patch

Might be good to bump UUID of nsIDocShell_MOZILLA_1_8_BRANCH since we've shipped the old version of the interface in an alpha.  If someone needed to use it, they would get hosed by an alpha.

r=darin

Comment 54

[Darin Fisher]

Comment on attachment 227453 [details] [diff] [review]
branch browser patch

micro-nit: destructor in nsAboutFeeds can be private and non-virtual.

Comment 55

[Ben Goodger (use ben at mozilla dot org for email)]

fixed-1.8-branch, fixed-on-trunk