330443 - privacy: loading remote xbl when replying or forwarding

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[georgi - hopefully not receiving bugspam]

thunderbird/seamonkey load remote xbl when forwarding as inline or replying.

this have privacy implications of tracking users.

attached is local folder.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: local folder for remote xbl

Comment 2

[georgi - hopefully not receiving bugspam]

Attachment: cgi

Comment 3

[Daniel Veditz [:dveditz]]

Scott: is this fixed by the patch for bug 328917? I'm updating my tbird tree now to test.

Georgi: which build were you testing? Trunk? 1.5 release?

At least scripts in the XBL aren't run.

Comment 4

[georgi - hopefully not receiving bugspam]

(In reply to comment #3)
> Scott: is this fixed by the patch for bug 328917? I'm updating my tbird tree
> now to test.
> 
> Georgi: which build were you testing? Trunk? 1.5 release?
> 

tested cvs build of seamonkey trunk and tunderbird latest 1.5.0.2 and 2.0a

Comment 5

[Scott MacGregor]

No it wouldn't be fixed by that other bug Dan. The compose window currently allows remote images to load in it. there are several already public bugs that talk about that. Making it so we don't allow remote images in the compose window when replying or forwarding a message should have the effect of fixing this bug to. 

Comment 6

[Daniel Veditz [:dveditz]]

Please ask for approval if there's a safe fix, but otherwise it doesn't look like there's traction here and we wouldn't hold the release for it.

Does this affect SeaMonkey too, and if so would it need a separate patch?

Comment 7

[georgi - hopefully not receiving bugspam]

(In reply to comment #6)
> Does this affect SeaMonkey too, and if so would it need a separate patch?

yes, it affects seamonkey trunk from today. 

Comment 8

[Scott MacGregor]

Attachment: framework for a fix

This patch fixes the test case but still has lots of problems (you can't add a remote image to a new compose window) and is not intended for checkin. I'm also not convinced that this approach is the best way to fix this. I'm hoping there's a better way.

When loading a url from an editor window, aRequestingLocation in the content policy manager for mail is about:blank because that's the src we set on the editor attribute. What we really want aRequestingLocation to be is the mail url for the message we are quoting or forwarding. Then we can do our normal nsIMsgDBHdr lookup to see if remote images are allowed for the message or not.

So this patch looks to see if we are inside a mail compose window, if we are, we try to get an attibute I'm now setting on the compose window object,  "originalMsgURI". Once we get this string from the window, the content policy manager can do a bunch of work to turn that into a nsIMsgDBHdr which lets us proceed like we do for mail windows.

Comment 9

[Scott MacGregor]

Maybe I could make the nsIMsgCompose object a property on the window, then get at it through nsIPDOMWindow::GetObjectProperty in the content policy manager. Then add a method to nsIMsgCompose for determing whether images should be allowed to load in it. 

Comment 10

[Scott MacGregor]

although replacing gMsgCompose with something like window.MsgCompose would probably break a lot of extensions (like enigmail).

Comment 11

[Scott MacGregor]

Attachment: improved framework

ShouldLoad was getting so big I couldn't read through it anymore so I broke it up into a couple pieces. 

Included with the fact that I still don't like this approach, this patch has the following problems:

1) The compose window property for originalMsgURI needs to be cleared after we've finished quoting the reply or forward text so the user can start adding additional remote images to the message even if the fwd/reply parts are blocked. Right now those get blocked to. 

2) This doesn't work at all if you use Options/Quote message. The remote images will load because the property never gets set. You can also quote multiple messages at the same time....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Is the fact that the compose window is not APP_TYPE_MAIL relevant here?  It certainly is for Seamonkey, but not sure about tbird...

Comment 13

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 14

[Scott MacGregor]

GetObjectProperty is no longer a DOM API on a window so my idea in comment 9 should be ignored.

Another alternative to the design outlined in the current patch could be:

Creation of every compose window, currently goes through the nsIMsgComposeService.

The compose service could keep a mapping between dom windows and nsIMsgCompose objects, allowing you to get the compose object associated with a particular dom window. 

We implement a new content policy manager class, add it to the chain of existing content policy managers. ::ShouldLoad extracts the dom window from the aContext, goes through the compose service to get the corresponding nsIMsgCompose object. From there, we can figure out if we are in the act of quoting a message (in which case we want to block remoate loads).

Comment 15

[Scott MacGregor]

Hmm, I've been able to implement this later solution such that during the quoting process,  remote requests get blocked if the original message had its remote content blocked. 

However, editor seems to do something that frequently causes it to reload the remote image requests (i.e. just opening the insert image dialog caused the quoted remote content to get reloaded, and since we aren't quoting anymore, the remote content loads).

We might have to take an even more restrictive stance. If you reply or forward a message which has blocked remote content, then that compose window stays in a 'restricted' mode, blocking all remote content, even remote images that you add yourself (Insert / Image).

Comment 16

[Scott MacGregor]

Attachment: a better solution

This patch implements the following idea:

* When replying or forwarding a message which has blocked remote content, block the content from loading in the compose window. On send, this same content should be blocked as well. 

* A compose window opened in this situation will block remote content the user may add as well. However on send, in the case of user added remote images, they will be fetched and sent out as parts of the message like we usually do. I'm still trying to understand the ramifications of that.

Design Details:

1) Every compose window registers its DOM Windowsand nsIMsgCompose object with the compose service. The compose service stores this information in a hash table. When a compose window closes (or is recyled), the entry gets removed from the hash table. This allows the compose content policy code to get the nsIMsgCompose object for a given dom window.

2) Our content policy implementation now has a policy for mail windows and another one for compose windows. For compose windows, we get the compose object for the passed in dom window. From there we get the original msg URI which has a value when replying or forwarding. The URI maps to a msg hdr which we then use to determine if we should block remote content or not.

3) Some of the code in nsMsgContentPolicy has been refactored into helper methods so they could be shared by the mail and the compose content policy routines.

4) The compose window sets the docshell app type as editor when it is created. I found several call sites that were re-setting the app type to APP_TYPE_EDITOR before we did quoting. Those calls are unneeded.

Comment 17

[Scott MacGregor]

Comment on attachment 242800 [details] [diff] [review]
a better solution

Guys, see comment 16 for details. Maybe we can get Georgi to help us test the patch as well.

Comment 18

[georgi - hopefully not receiving bugspam]

sure i will test the patch.

very wild suspicions, probably false:

1. does it block remote content in <iframe> in the compose window?
2. very unlikely but it is possible to break the restriction of attaching '<img src=file:///' ?
3. is it possible the strangeness of 'insert image' to cause troubles now (without the patch)?

Comment 19

[georgi - hopefully not receiving bugspam]

can't apply the patch. get a lot of 'hunk # FAILED' on both thunderbird trunk and suite trunk from today cvs.

on what it should be applied?

Comment 20

[Scott MacGregor]

Hmm, this patch was made against a trunk build from a couple days ago. I'll update my tree again to see if there are any conflicts.

Comment 21

[Scott MacGregor]

Attachment: same patch as before, updated against the trunk

I updated my trunk tree again and made a new patch. There aren't any code changes between this one and the previous one, but hopefully Georgi can get this one to apply. Georgi, also note that I don't think this patch works for seamonkey yet so please test Thunderbird with it. Seamonkey has an #ifdef at the top of ::ShouldLoad which returns early for any window that's not of type APP_TYPE_MAIL, we'll need to work with the seamonkey folks to fix that.

Comment 22

[georgi - hopefully not receiving bugspam]

the problem with the patch is resolved. linux patch(1) is confused by windows style newlines. after stripping '\r' the patch applies and seems to work, trying to abuse it.

is there patch(1) options to ignore '\r\n' - "-l" doesn't seem to work?
 

Comment 23

[[e-mail]]

(In reply to comment #22)
>linux patch(1) is confused by windows style newlines.
RedHat's patch automatically detects and strips CRs from newlines.

Comment 24

[georgi - hopefully not receiving bugspam]

the patch seems to fix the issue with the following exception (so far):

"edit message as new" still loads the remote content.

the patch also fixes loading images without xbl via 'style="background: url()' and <iframe src="data:text/html;,<img src='....'>"> (affects all thunderbirds) - "edit message as new" still loads them.

Comment 25

[georgi - hopefully not receiving bugspam]

(In reply to comment #23)
> RedHat's patch automatically detects and strips CRs from newlines.
> 

hm, at least some versions of ubuntu's patch(1) also do this, but some distros don't.

Comment 26

[Scott MacGregor]

Attachment: updated fix to block edit as new

The content policy changes are identical to the previous patch. 

To understand the change, it helps to know that the Edit as New operation is really opening up a message as a template...

I had to make a change to libmime and the compose code that fetches the body for templates to make sure the original msg URI attribute gets set set on the compose object like we do for reply and forward actions. There could be side effects from doing this for a template, but I haven't been able to figure any out.

While fixing the compose code to pass around the original msg uri for templates, I had to make changes to an interface called nsIMsgDraft with an implementation defined in nsMsgCreate.cpp.h. This whacky XPCOM interface and implementation was used so the compose service could kick off a part fetch for templates and inline message forwards. All of which could be done in a simple method in the compose service. There's no need for this xpcom object. So I made a new private method on the compose service called LoadDraftOrTemplate which replaces nsIMsgDraft/nsMsgCreate. A lot of that code was written very poorly, using incorrect coding patterns, I *hopefully* improved on that when I re-implemented it as a private method.

Comment 27

[Scott MacGregor]

Attachment: updated fix to block drafts as well

Drafts weren't getting the originalMsgURI attribute set for their compose objects so I had to extend the libmime tweak for drafts and templates. 

Additionally, I also made the content policy block remote images for mailto urls in all cases.

Comment 28

[[e-mail]]

(In reply to comment #27)
>Created an attachment (id=243448) [edit]
>updated fix to block drafts as well
So what happens if a) you create a message, save a draft, and edit that
b) you reply to a message with blocked content, save a draft, and edit that?

Comment 29

[georgi - hopefully not receiving bugspam]

the patch 2006-10-24 23:26 seems to work.

notice the following:

1. it fails to build on fresh trunk because of missing nsIMsgDraft.h. building trunk and then applying the patch and rebuilding works.

2. the patch does not work for messages for which "show images" has been applied in the past - this seems unrelated to this bug.

Comment 30

[Scott MacGregor]

Attachment: updated fix

see comment 16 and comment 26 for an overview of the design.

This patch blocks on drafts, replies, templates, forwards and remote image requests that are attached to mailto urls.

Dan, feel free to just review the content policy change itself. I won't be offended if you don't want to get dragged into the compose changes :).

I also ran through the additional test case ideas Neil suggested and those passed too.

Comment 31

[David :Bienvenu]

Comment on attachment 243529 [details] [diff] [review]
updated fix

Cool - the two areas to check are opening a .eml file from disk, and opening a forwarded as attachment message (also .eml), and make sure reply/forward do the right thing w.r.t. remote content.

Comment 32

[Scott MacGregor]

Attachment: another update

I slept on this a little more and decided to make two changes to nsMsgContentPolicy. I'll go over them here so you don't need to re-review the patch David.

I made the compose logic for blocking remote content tighter. The previous patch did this:

// drafts, replies, forwards and templates will always have a value for
// originalMsgURI
if (!originalMsgURI.IsEmpty())
{
  nsCOMPtr<nsIMsgDBHdr> msgHdr;
  GetMsgDBHdrFromURI(originalMsgURI.get(), getter_AddRefs(msgHdr));
  AllowRemoteContentForMsgHdr(msgHdr, nsnull, aContentLocation, aDecision);
}
else 
  // Always block remote images for mailto urls
  *aDecision = composeType == nsIMsgCompType::MailToUrl ? nsIContentPolicy::REJECT_TYPE : nsIContentPolicy::ACCEPT;
 
Now it does:

// Only allow remote content for new mail compositions.
// Block remote content for all other types (drafts, templates, forwards, replies, etc)
// unless there is an associated msgHdr which allows the load...
if (composeType == nsIMsgCompType::New)
  *aDecision = nsIContentPolicy::ACCEPT;
else if (!originalMsgURI.IsEmpty())
{
  nsCOMPtr<nsIMsgDBHdr> msgHdr;
  rv = GetMsgDBHdrFromURI(originalMsgURI.get(), getter_AddRefs(msgHdr));
  NS_ENSURE_SUCCESS(rv, NS_OK);
  AllowRemoteContentForMsgHdr(msgHdr, nsnull, aContentLocation, aDecision);
}

This blocks remote content for *any* compose type with the exception of a new composition or the case where we successfully get a msg hdr which instructs us to allow the load. 

This more restrictive code fixes your suggested test cases for opening a .eml file and for replying to a .eml attachment.

I also changed how we propagate error codes in ShouldLoad back to the content policy manager. nsContentPolicy ignores our decision if we ever return an error code. In the case of blocking remote content, if we are given something we believe we should be able to make a decision on, but one of our method calls triggers an error, I think we want to block the load instead of ignoring our decision because we generated an error. 

I modified ShouldLoad to return NS_OK instead of rv in the various NS_ENSURE_SUCCESS macros once we've gotten far enough along to realize the content request originates from a window we care about and should be making a decision on. This corresponds to the point in ShouldLoad when we default aDecision to reject the load.

Comment 33

[Scott MacGregor]

This patch caused a small regression for RSS feeds which are loading the web page in an iframe.

<iframe src="http url for the article">

remote images are getting blocked for this use case now, where before we always loaded rss images. 

I filed Bug 359402 to track this.

Comment 34

[Scott MacGregor]

Now that I've fixed Bug 359402, it's time to move this onto the 1.8.1 branch for Thunderbird 2.

Comment 35

[Scott MacGregor]

Attachment: ported to the 1.8.1 branch

Comment 36

[Daniel Veditz [:dveditz]]

Can we get this landed ASAP? In danger of not making 1.8.0.9, won't hold 1.8.1.1 for this.

Comment 37

[Scott MacGregor]

this landed on the 1.8.1.1 branch on November 4th. Shame on me for forgetting to come back and mark it as such.

I don't believe we're going to want to take this on the 1.5.0.x branch. The changes are pretty significant and we haven't had a release of any sort with them yet.

Comment 38

[Daniel Veditz [:dveditz]]

based on scott's comment I'm moving the 1.8.0 blocking flag to the wanted? state to look at in a later release.

Comment 39

[Daniel Veditz [:dveditz]]

Has this baked enough? people want this in 1.5.0.x

Comment 40

[Daniel Veditz [:dveditz]]

Any answer to comment 39? or does comment 37 still hold?

Comment 41

[Scott MacGregor]

I'd still be pretty scared Dan, but I'm happy to talk about it with you. 

Comment 42

[Daniel Veditz [:dveditz]]

OK, users who are concerned can upgrade to Thunderbird 2.