374578 - Block remote content for messages only - and not just in the message pane

Status: RESOLVED FIXED

(Thunderbird :: Security, defect, P1)

Description

[Scott MacGregor]

The content policy manager for Thunderbird blocks JS, remote content and cookies for all content in the mail 3-pane. We really want it to sandbox just content loaded in the message pane. 

This would allow extension authors the ability to embed web apps in different parts of the Thunderbird application and have them actually work.

Comment 1

[Scott MacGregor]

Attachment: first cut at a fix...

I still have some more testing to do, but this is what I have so far.

Comment 2

[Scott MacGregor]

Comment on attachment 259070 [details] [diff] [review]
first cut at a fix...

Neil, do you mind looking at the remote content part of the patch since that effects seamonkey?

We only care about blocking remote content (and cookies for tbird) if the content is or is a child of the message pane docshell.

Comment 3

[David :Bienvenu]

Comment on attachment 259070 [details] [diff] [review]
first cut at a fix...

in nsresult getMessagePaneDocShell(nsIDocShellTreeItem * aDocShellTreeItem, nsIDocShell ** aMessagePaneDocShell)
+

you could move the decl of rv to where it's first used.

Comment 4

[[e-mail]]

Life would have been so much simpler if the APP_TYPE_MAIL was set on the message pane's doc shell rather than on the root doc shell...

Comment 5

[Wayne Mery (:wsmwk)]

dmose, In the context of were we are at / heading, should this patch proceed?  depends on bug ___?

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> dmose, In the context of were we are at / heading, should this patch proceed? 
> depends on bug ___?

This would make one or two of my tasks a bit easier....

(In reply to comment #4)
> Life would have been so much simpler if the APP_TYPE_MAIL was set on the
> message pane's doc shell rather than on the root doc shell...

Neil: is this something that is still viable to do, and must be done before the patch, or is it just a comment and you didn't get round to marking r+?

Comment 7

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #4)
> > Life would have been so much simpler if the APP_TYPE_MAIL was set on the
> > message pane's doc shell rather than on the root doc shell...
> Neil: is this something that is still viable to do, and must be done before the
> patch, or is it just a comment and you didn't get round to marking r+?
No, but at that point I was still hopeful that someone could have looked into it. (I guess it's still possible for us to set APP_TYPE_MAIL on both the chrome and content document, so that we can start switching consumers...)

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 259070 [details] [diff] [review]
first cut at a fix...

This patch has bitrotted.

$ patch -p0 --dry-run < ~/Desktop/Bug374578.diff 
patching file nsMsgContentPolicy.cpp
Hunk #1 FAILED at 91.
Hunk #2 succeeded at 386 (offset 44 lines).
Hunk #3 succeeded at 657 (offset 101 lines).
Hunk #4 succeeded at 763 (offset 176 lines).
Hunk #5 FAILED at 806.
2 out of 5 hunks FAILED -- saving rejects to file nsMsgContentPolicy.cpp.rej
patching file nsMsgContentPolicy.h
Hunk #1 FAILED at 89.
1 out of 1 hunk FAILED -- saving rejects to file nsMsgContentPolicy.h.rej

Comment 9

[Mark Banner (:standard8)]

Attachment: Updated patch

This is an updated version of Scott's patch.

Although I think setting the docshell on the message pane to APP_TYPE_MAIL is the way we want to go, I think given where we are in the release cycles at the moment, we're unlikely to get the core changes in easily, and its a big change to the policy.

Doing these changes would allow us to do what we need for Thunderbird 3 and then we can revise it for the next version.

Note that I've dropped the cookie handling changes from this version - the cookie policy needs to be re-merged with core (somehow) and the change would blindly allow all cookies on non-messagepane docshells.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

The way I've been thinking about content policy for Tb3 has been under the assumption that we want to restrict content in messages, but generally not restrict content for non-messages, independent of which docshell the thing is loading in.  It sounds like you've been thinking about it differently.  Find me on IRC and we can talk about it more there or by phone.  I also think that this particular overall policy is probably the security review we most need to do before shipping Tb3.

Comment 11

[Mark Banner (:standard8)]

Attachment: Proposed Solution

Here's the new content policy solution based on discussion between myself and Dan last night.

The current issue stems from the fact that we are always using the message pane URI (from GetMessagePaneURI) to do final determination on loading remote content. This means that any non-message pane content docshell (aka iframe in a tab) is also going to use the message pane URI... which is just wrong.

In this patch I have rearranged the functions, created new ones and got rid of some old ones, the intention being to make ShouldLoad much easier to read and understand, the general order is now as follows:

1) If Suite, accept anything that isn't from an app type of APP_TYPE_MAIL.
2) Reject TYPE_OBJECT (plugins) if we're not allowing plugins.
3) Disable JS on TYPE_DOCUMENT.
4) If the requesting location is trusted (chrome, resource, about, file), accept the load.
5) If the content location is exposed or is chrome, resource, data then accept the load.
6) If the content location isn't http, https or file then reject the load.
7) If allowing all remote images/content, accept the load.
8) If we're the compose window, call ComposeShouldLoad to do special processing.
9) Get the originating URI from the docshell that is found by GetSameTypeRootItem (so the root content docshell that we've set up, to account for multiple iframes).
10) If the originating URI is http or https, accept the load.
11) If the originating URI isn't a mailnews message URI, reject the load.
12) Look at the mailnews message uri and determine if we should load it based on the message and whitelists etc.

I included the existing steps 1 - 8 to provide some context.

The step that has really changed is step 9 - this used to get the message pane URI. Steps 10 and 11 have been moved into ShouldLoad where they were previously in MailShouldLoad.

I have tested some aspects of this locally (e.g. iframe in an email message, loading remote content on in a new tab with its own iframe). I'm hoping that I can do a mozmill test based on Dan's test in bug 374577.

Comment 12

[Mark Banner (:standard8)]

If you want to test the content policy on the what's new page you need the patch on bug 466527 which turns the tab into a content iframe.

Comment 13

[Mark Banner (:standard8)]

Attachment: Content Policy Fix

I'm happy with this now, I'm hoping to come up with some mozmill tests in a few days. For testing on Thunderbird it needs the patch on bug 466527 applied - this means the what's new page is correctly loaded into content.

Neil, I realise you can't test this on SeaMonkey but thought you might want to take a look at it anyway - feel free to push sr to bienvenu if you want to.

Comment 14

[Mark Banner (:standard8)]

The patch here blocks bug 466527 which is blocking TB 3. Therefore bringing this into the blocking list.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> The patch here blocks bug 466527 which is blocking TB 3. Therefore bringing
> this into the blocking list.

Note: the cookies part may be shipped out to another bug.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 371227 [details] [diff] [review]
Content Policy Fix


> {
>   nsresult rv = NS_OK;
>+  // The default decision at the start of the function is to accept the load.
>+  // Once we have checked the content type and the requesting location, then
>+  // we switch it to reject. 
>   *aDecision = nsIContentPolicy::ACCEPT;

Probably worth also explicitly calling out that if one of the NS_ENSURE_* macros causes method this to return an NS_ERROR, any decision made here will be ignored and the document will be rejected.

Other comments passed along in IRC, more to come...

Comment 17

[Mark Banner (:standard8)]

Attachment: Content Policy Fix v2

Updated version based on Dan's comments so far.

Comment 18

[Mark Banner (:standard8)]

Attachment: WIP content policy test

Basic test case, covers remote images in emails. The actual checks aren't ideal at the moment, and it needs expanding to cover tabs, and preferably have a local http server doing the tests but it gives the basic idea.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 372873 [details] [diff] [review]
Content Policy Fix v2

In addition to fixing this bug, your patch is a huge step in the right direction of making this code easier to grok; nice work!  Most of the changes I'm suggesting are just simple ways to make the code easier to deal with.

Can I talk you into pushing each of the clauses starting with:

  // if aRequestingLocation is chrome, resource about or file, allow
  // aContentLocation to load

and

  // if aContentLocation is a protocol we handle (imap, pop3, mailbox, etc)
  // or is a chrome url, then allow the load

down into its own method?  I think doing that will actually leave ShouldLoad at a size that's not unnecessarily difficult to reason about.

>@@ -224,12 +232,13 @@ nsMsgContentPolicy::ShouldLoad(PRUint32 
> #ifndef MOZ_THUNDERBIRD
>   // Go find out if we are dealing with mailnews. Anything else
>   // isn't our concern and we accept content.
>-  nsCOMPtr<nsIDocShell> docshell;
>-  rv = GetRootDocShellForContext(aRequestingContext, getter_AddRefs(docshell));
>+  nsCOMPtr<nsIDocShell> rootDocShell;
>+  rv = GetRootDocShellForContext(aRequestingContext,
>+                                getter_AddRefs(rootDocShell));
>   NS_ENSURE_SUCCESS(rv, rv);

How about just hoisting the declaration and getting of the rootDocShell out of the ifdef clauses, since it's identical in both cases?

>+  // Work out if we're in a compose window or not.
>+  if (isComposeWindow)
>+    ComposeShouldLoad(rootDocShell, aRequestingContext, aContentLocation,
>+                      aDecision);

How about adding a "return NS_OK" to the then clause?  This would allow the else block to go away, and the content of the else block to be hoisted up to the top-level; which would be clearer to the reader, I think.  Bonus points if you feel like making ComposeShouldLoad return void in order to better reflect reality.

> /**
>- * For a given msg hdr, iterate through the list of remote content blocking criteria.
>- * returns nsIContentPolicy::REJECT if the msg hdr fails any of these tests.
>+ * For a given msg hdr, go through the list of remote content blocking criteria:
>  *
>- * @param aRequestingLocation cannot be null
>+ * #1 Allow if there is a db header for a manual override.
>+ * #2 Allow if the message is in an RSS folder.
>+ * #3 Allow if the domain for the remote image in our white list.
>+ * #4 Allow if the author has been specifically white listed.
>+ *
>+ * If any of these fail, the request will be rejected.

That last sentence is a bit ambiguous about whether it's referring to the "if" statements returning false or referring to any sort of error while attempting to compute that, I assume it's the latter.  I'd suggest being a bit more explicit.

>@@ -432,76 +461,69 @@ nsresult nsMsgContentPolicy::AllowRemote
>   AllowRemoteContentForSender(aMsgHdr, &allowForSender);
> 
>   *aDecision = allowForSender ?
>-               nsIContentPolicy::ACCEPT : nsIContentPolicy::REJECT_REQUEST;
>+               (PRInt16)nsIContentPolicy::ACCEPT :
>+               (PRInt16)nsIContentPolicy::REJECT_REQUEST;

In general, prefer C++-style casts: they express intent more precisely, which allows the compiler to catch more unintentional errors.

>+nsresult
>+nsMsgContentPolicy::AllowContentForPotentialMessage(nsIURI *aOriginatorLocation,
>+                                                    nsIURI *aContentLocation,
>+                                                    PRInt16 *aDecision)

While I understand that this is named to parallel the other Allow methods here, that naming scheme has the disadvantage that it's easy to misread calling code as Allow! rather than the intended Allow?  If you can think of a clearer naming scheme for all three methods, that'd be great, but don't let that block you on landing this.

Is there some reason this method needs to return an nsresult instead of void?

Finally, can you spend a bit of time thinking about the call to NS_CP_GetDocShellFromContext that's been added in order to convince yourself that (given the bizarre semantics documented in nsContentPolicyUtils.h), it's actually going to do the right thing when called on the various different types of content?

Thanks!

Comment 20

[Mark Banner (:standard8)]

I've spun the cookies issue out into bug 492279 - this bug can just focus on the content policy.

Comment 21

[Mark Banner (:standard8)]

Attachment: Content Policy Fix v3

Changes since the last patch:

1) Renamed the Allow* functions to ShouldAccept*
2) Changed various functions not to return nsresult - either the value or void depending on the situation.
3) GetRootDocShellForContext - this function was warning in various cases where it couldn't get a doc shell. SeaMonkey has been seeing this for a while (bug376460), TB now sees it because the function is now called much earlier in ShouldLoad. I've done some analysis and I think its safe to not have the doc shell in various instances - I've documented those in the function.
4) Several new functions: IsSafeRequestingLocation, IsExposedProtocol, IsExposedChromeProtocol, ShouldBlockUnexposedProtocol. These are sections split out from ShouldLoad in the previous patch.
5) If when we get to the nsIMsgMessageUrl check (at the end of ShouldLoad, now in ShouldAcceptContentForPotentialMsg) the originator location is NOT an nsIMsgMessageUrl we now ACCEPT the load. This can happen in the nested iframe case. Reason for change: If the originator location is not an nsIMsgMessageUrl then we're most likely loading a web page or some other protocol, but not a message - therefore we don't need to worry about remote content, and we should be treating it like a standard browser/iframe and letting the core content policies decide what to do.

From my various tests I believe the NS_CP_GetDocShellFromContext is safe to be calling - I couldn't find a case where I ended up with something I didn't expect, we certainly never seemed to go above the top iframe for a content type.

Re-requesting sr even though sr+ already given because of point 5.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

This looks good; a few comments:

As we discussed in IRC, we need some unit and functional tests here to get full confidence; as long as those block RC1, having the main code land now seems fine to me.

> 3) GetRootDocShellForContext - this function was warning in various cases where
> it couldn't get a doc shell. SeaMonkey has been seeing this for a while
> (bug376460), TB now sees it because the function is now called much earlier in
> ShouldLoad. I've done some analysis and I think its safe to not have the doc
> shell in various instances - I've documented those in the functio

Reading the comments describes cases where those things wouldn't be dangerous, but it's not clear to me from reading the comments that we should believe that no other code paths could trigger dangerous-but-indistinguishable case.  Please add some commentary about why we think that's true (if we do).  If we don't, I suspect it'll be helpful to talk to bz about the calling code to get a better feel for what risk we're accepting, if any.

> 4) Several new functions: IsSafeRequestingLocation, IsExposedProtocol,
> IsExposedChromeProtocol, ShouldBlockUnexposedProtocol. These are sections split
> out from ShouldLoad in the previous patch.

Great; definitely helps the understandability of the code.

> 5) If when we get to the nsIMsgMessageUrl check (at the end of ShouldLoad, now
> in ShouldAcceptContentForPotentialMsg) the originator location is NOT an
> nsIMsgMessageUrl we now ACCEPT the load. This can happen in the nested iframe
> case. Reason for change: If the originator location is not an nsIMsgMessageUrl
> then we're most likely loading a web page or some other protocol, but not a
> message - therefore we don't need to worry about remote content, and we should
> be treating it like a standard browser/iframe and letting the core content
> policies decide what to do.

Sounds reasonable.  When you get to the test-writing phase, please make sure that feed summaries and content all do the right thing here.

Comment 23

[Mark Banner (:standard8)]

(In reply to comment #22)
> As we discussed in IRC, we need some unit and functional tests here to get full
> confidence; as long as those block RC1, having the main code land now seems
> fine to me.

These are covered in bug 491921.

> > 3) GetRootDocShellForContext - this function was warning in various cases where
> > it couldn't get a doc shell. SeaMonkey has been seeing this for a while
> > (bug376460), TB now sees it because the function is now called much earlier in
> > ShouldLoad. I've done some analysis and I think its safe to not have the doc
> > shell in various instances - I've documented those in the functio
> 
> Reading the comments describes cases where those things wouldn't be dangerous,
> but it's not clear to me from reading the comments that we should believe that
> no other code paths could trigger dangerous-but-indistinguishable case.  Please
> add some commentary about why we think that's true (if we do).  If we don't, I
> suspect it'll be helpful to talk to bz about the calling code to get a better
> feel for what risk we're accepting, if any.

bz: Dan is talking about two cases we're hitting in ShouldLoad/GetRootDocShellForContext:

http://mxr.mozilla.org/comm-central/source/mozilla/content/base/public/nsIContentPolicy.idl#232

1) aContext the nsISupports param is sometimes null. This looks to be the case where we first create a hidden window (on Mac at least) and so there isn't any relevant context to be supplied.

Is there another case where aContext could be null?

2) When calling NS_CP_GetDocShellFromContext with aContext we don't always get a shell returned.

I believe that:

+  // This appears to occur in cases where there is no display or window
+  // associated with a load yet, e.g. loading a skin xml bindings file which
+  // loads some locale files, or loading textbox.css which wants to load
+  // autocomplete.css.

Is that a reasonable assumption, or could there be other cases that we need to handle?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Is there another case where aContext could be null?

Sure.  Any caller is allowed to pass in a null aContext if that's what makes sense for the load in question.  The interface says so clearly...

> Is that a reasonable assumption, or could there be other cases that we need to
> handle?

All you know if that returns null is that either we couldn't get a window from the context (it's not a window itself, and isn't a node or document living in a window) or that window's docshell is null (which means it's partway through being destroyed).  Certainly resource loads for random chrome XBL might fall in here, as might various other stuff.

In general, please remember that extensions can (and should!) make content policy calls on loads they do, so you really can't rely on much more than what the API promises directly.

Comment 25

[Mark Banner (:standard8)]

(In reply to comment #24)
> In general, please remember that extensions can (and should!) make content
> policy calls on loads they do, so you really can't rely on much more than what
> the API promises directly.

Ok, so what I think we need to think about here is two things:

- SeaMonkey uses the context to get the rootDocShell to work out if we're APP_TYPE_MAIL or not. If it is not then it will accept the load, leaving it to other parts of the "core" content policy to work out.

- Thunderbird will get the root docShell but won't use it until it tries to determine if the window is a compose window or not. At that stage if it hasn't got one, it will reject the load (with and without the patch).

What I am therefore tempted to do is to restore the status quo with this function: use NS_ENSURE_SUCCESS as we do now, and although it complicates the function slightly with ifdefs, only get it when we need to.

Then if anyone hits it unexpectedly, we'll be alerted and can look at the case. This does mean SeaMonkey will retain the existing warnings, but that is covered by bug 376460.

Comment 26

[Mark Banner (:standard8)]

Attachment: Content Policy Fix v4

This leaves the GetRootDocShellForContext behavior the same as it was before the patch. I'll check it in tomorrow.

Comment 27

[Mark Banner (:standard8)]

Updating summary to better reflect what we've actually done here.

Comment 28

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/8ca8d974b709

Setting in-testsuite? as we want tests for this, though they will be written in bug 491921.

Comment 30

[Mark Banner (:standard8)]

The tests for this bug were done in bug 491921.