Closed Bug 1409458 Opened 7 years ago Closed 7 years ago

Privacy Issue: Replying to or forwarding an HTML e-mail with external content (e.g. images), may load this content without user notification.

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(thunderbird_esr5257+ fixed, thunderbird58 fixed, thunderbird59 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 59.0
Tracking Flags:
Tracking Status
thunderbird_esr52 57+ fixed
thunderbird58 --- fixed
thunderbird59 --- fixed

People

(Reporter: jlnwntr, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

(Keywords: privacy, sec-low)

Attachments

(1 file, 1 obsolete file)

1409458-kw-check.patch (v1b)
1.56 KB, patch
aceman
: review+
jorgk-bmo
: approval-comm-beta+
jorgk-bmo
: approval-comm-esr52+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0 Build ID: 20171003101344 Steps to reproduce: In the scope of academic research in cooperation with FH Münster and Ruhr-University Bochum, we discovered a bypass for active content blocking. An example for active content in emails are images in HTML emails that can either be embedded into the email or loaded from the displaying application. This active content can be used to track users as the sender can detect if and when images are loaded. For this reason, Thunderbird blocks active content per default and asks the user if the content should be displayed. A victim may receive an HTML e-mail with following content: <html> <body> <p>Please forward this email to Max Müller!</p> <img src=“http://attacker.domain/image.png” /> </body> </html> The user replies to or forwards this email. If the user clicks on the image-pane of the img-tag, an HTTP-request is send immediately to the foreign server. Actual results: An HTTP-request is send immediately to the foreign server. This happens although the user never agreed to load any active content. Expected results: The expected behavior of Thunderbird is to ask the user with a yellow bar, if external contents shall be loaded. Always notify the user before the application tries to request any external sources.
Yes the image is loaded if you go into the properties of the blocked image you're forwarding/replying to. But I'm not sure that is wrong. The user actively choose to reply/forward this message, AND then go into the properties to do something special with it. It would appear to me any kind of warning then is just a click-through dialog as you've already gone through many steps to actually USE that image. The privacy implications are minor at that point.
Keywords: privacy
Minor disclosure that is better known (public) than hidden since we're probably not going to change anything.
Group: mail-core-security
Version 5?
If the email was malicious, the user probably wouldn't reply to it. He might forward malicious emails, but then probably not click on it. Even if he does by accident, the entire damage done is a HTTP ping. Given that this needs multiple actions from the user - a reply, and then a click on the image - I think this is very minor, if a bug at all.
Keywords: sec-low
Severity: normal → minor
I'm seeing a much more serious version of this in Thunderbird 52.4.0/56.0b4. All it takes for an originally blocked remote image to be loaded for me is to start composing a reply to the message. This didn't happen in 45.8.0. Reported in bug 406825, comment 3.
I don't understand the report. If you have a message with blocked remote images, they do *not* get loaded if you reply to the message or forward the message inline. As Magnus said, if you "go into the properties" of the image, or double-click on it, then the image gets loaded. That's has changed in bug 609406. We discussed about it there. So behaviour has changed between TB 45 and TB 52. I'm duping the ancient bug 406825 over here, so we can look into the issue.
> If you have a message with blocked remote images, they do *not* get loaded if you reply to the message or forward the message inline. They get loaded for me in 52. They don't in 45. I have an incoming HTML message with an img tag with a remote Avast tracker in it. It isn't displayed, and TB shows "To protect your privacy, Thunderbird has blocked remote content in this message." I press reply or forward, and I get a notification from Little Snitch for a load attempt to avast.com. Going to the image properties is not required. I can reliably reproduce this in 52 and 56, and not in 45. If no one else is seeing this, I can dig into it with a debugger and try to figure out why it's happening for me. (I recognize this is a different report from this bug, which is why I posted it to bug 406825 (which predated the later fix, but describes the behavior I'm seeing again in >=52).)
OK, so we all agree that images are *not* *displayed* when replying or forwarding? You're saying that "Little Snitch" (which is a Mac firewall, right?) is alerting you about the download? Hmm, I'm on Windows, I'd have to install some network analysis tool and see whether I can confirm this ... when I find the time. Setting NI so I won't forget. If confirmed, that would be a regression from bug 609406 introduced in TB 52.
Flags: needinfo?(jorgk)
OK, I was able to look a little more closely. I had just been denying the connection, but you're right -- it is indeed not displaying, or even loading, the image in the reply window. What it is doing is briefly opening a TCP connection to the server and then closing it a couple seconds later. So somewhat less of privacy issue, since no HTTP requests are made, but the remote server could in theory still log incoming TCP connections, and people running local firewalls will still get notified of connection attempts. I didn't want to derail this bug, which is for a different issue, so I can open a new bug for this if that'd be helpful.
(In reply to Dan Stillman from comment #12) > I didn't want to derail this bug, which is for a different issue, so I can > open a new bug for this if that'd be helpful. I don't think you're derailing this bug at all, quite the opposite. As I said, I see placeholders when replying/forwarding, but we need to check what's happening under the covers with the network traffic. I'm not a networking expert so I don't quite understand how/why a connection would be opened without sending some sort of request. Maybe a Wireshark protocol or similar could be useful. Maybe you can even pursue this issue further, since I won't find time for in for a while, other fires are burning much hotter right now.
Correction: This is absolutely making an HTTP request for the image (even though it's not displayed in the reply window), so it is a significant privacy issue. Once the image has been loaded once, it's added to the cache, and then pressing Reply only triggers a TCP connection that stays open for exactly 5 seconds (at least with the server I'm testing). If the cache is cleared, the HTTP request will be made again. Steps to reproduce: 1) Send yourself an email with an img tag pointing to a web server you control. 2) View the received message with remote images blocked. 3) Clear the Thunderbird cache. 4) Press Reply on the message. TB makes a request for the image, which appears in the web server log. If the reply window is closed and reopened without clearing the cache, only a TCP connection is made and nothing appears in the web server log.
Boris, can I trouble you with this. As you know, we have mailnews/base/src/nsMsgContentPolicy.cpp where we reject images when preparing a reply or forwarded message: https://dxr.mozilla.org/comm-central/rev/952762964f408e33d060dd1110cac0626ae5fa51/mailnews/base/src/nsMsgContentPolicy.cpp#669 As a TB user maybe of TB 52.x you know the effect. You reply to a message with blocked images and in the reply you see don't see the image but instead the alt text or nothing. Somehow we don't appear to see placeholders. When you double-click such a placeholder, the image is loaded since we no longer reject it. We also don't reject images the user is placing into such a reply. That's what bug 609406 was about: https://hg.mozilla.org/comm-central/rev/06666873a81d1b12cd0f82cf8406015499db1609 As Dan described in comment #14, the image is fetched from the server when preparing such a reply although it's not shown. Do you understand this effect and can you shed some light onto it. We even have a test that tests that blocked images are not shown when replying: https://dxr.mozilla.org/comm-central/rev/952762964f408e33d060dd1110cac0626ae5fa51/mail/test/mozmill/content-policy/test-general-content-policy.js#392 However, the test checks .imageBlockingStatus https://dxr.mozilla.org/comm-central/rev/952762964f408e33d060dd1110cac0626ae5fa51/mail/test/mozmill/content-policy/test-general-content-policy.js#65 which may or may not be related to having fetched the image. It would be nice if an image that is blocked is also not fetched from the server.
Flags: needinfo?(jorgk) → needinfo?(bzbarsky)
.imageBlockingStatus is definitely related to whether the image was fetched. When we say we blocked the image load, that means we actually blocked it. Note that this only gives you information about the _last_ load that was performed for that <img>, of course. If we're doing two loads and the first is succeeding while the latter gets blocked, this test will not catch that. Someone needs to actually sit down and debug this in Thunderbird. I'd probably start with adding a breakpoint in nsHttpChannel::AsyncOpen (assuming e10s is not involved here) and seeing what the stack is when it gets hit. Or a breakpoint in nsHttpChannel::AsyncOpen2 and then stepping through what nsMsgContentPolicy ends up doing with the load.
Flags: needinfo?(bzbarsky)
Boris, thanks for the reply so far. I'm happy to debug it (when/if I find the time). Just one more question. It is observed behaviour that intentionally blocked http-based images are not displayed in a reply/forwarded message. Nonetheless the claim was made that the images are fetched from the server. That doesn't make sense to me. M-C code controls the display of the document, so why would an image be fetched if not to display it? Or are you suggesting that two loads are done, the first time the content policy answers "yes" and the image is fetched, then it answers "no" and the fetched image is not displayed? Looking at the code, while the document is prepared for reply, quoted images are blocked via GetInsertingQuotedContent(). Maybe a second load happens without this gate, so the answer would be "don't block", but why wouldn't the image be displayed then? Too hard to answer, needs debugging?
Flags: needinfo?(bzbarsky)
> Or are you suggesting that two loads are done, the first time the content policy > answers "yes" and the image is fetched, then it answers "no" and the fetched > image is not displayed? That is the most plausible hypothesis I have so far, yes. Now _whether_ that happens, and _why_ it does if it does are things that would need debugging.
Flags: needinfo?(bzbarsky)
OK, I was able to track this down with a debugger. It looks like there are indeed two calls to AsyncOpen2(). The first goes through InsertAsCitedQuotation() and doesn't trigger the load. The second comes from JS and involves a nsINode::cloneNode() call, and that one triggers the load. I noticed a cloneNode() call in MsgComposeCommands.js::checkForAttachmentKeywords(), and sure enough, disabling "Check for missing attachments" prevents the load from happening.
Hey Dan, great job, saves me a lot of time. I'm glad to hear that we didn't mess up TB's content policy but that it some other undesired side effect. So Boris, should cloning the node (body of the document) go and fetch images, and then not display them? We're calling let mailBodyNode = mailBody.cloneNode(true); so maybe changing the deep clone parameter to false could already fix the problem? What does the parameter do anyway? (I can research it, but you can possibly answer it in 35sec.)
Flags: needinfo?(bzbarsky)
> should cloning the node (body of the document) go and fetch images It depends on what document the node is in, and what content policies have to say about the result. In general, the clone of an image node with an src will _try_ to load that same src, obviously. > and then not display them Depends on what you do with the node. > so maybe changing the deep clone parameter to false could already fix the problem? Well, do you want a deep clone or not? > What does the parameter do anyway? If false, it will clone just the node you're calling cloneNode on, but not any of its kids, and give you a node with no kids. If true, it will clone the entire tree and give you the cloned tree. Chances are, you do in fact want a deep clone.
Flags: needinfo?(bzbarsky)
> It depends on what document the node is in, and what content policies have to say about the result. Oh, and the point is that this clone is happening at some random point in time while composing, and hence insertingQuotedContent is false in nsMsgContentPolicy::ComposeShouldLoad, because we're no longer inserting quoted content. What I would suggest you do is that in _checkForAttachmentKeywords instead of cloning the body you create a new data document (via getBrowser().contentDocument.implementation.createDocument(), say) and then importNode into that document. Since it's a data doc, nsDataDocumentContentPolicy::ShouldLoad will block the loads. And since the data doc is created from the DOMImplementation of the untrusted document, it shouldn't have a system principal, so should actually reach nsDataDocumentContentPolicy::ShouldLoad.
Assignee: nobody → jorgk
Severity: minor → major
Status: UNCONFIRMED → ASSIGNED
Component: Message Compose Window → Composition
Ever confirmed: true
Product: Thunderbird → MailNews Core
Version: 5.0 → 52
Attached patch 1409458-kw-check.patch (v1) (obsolete) — — Splinter Review
Boris, thanks for the hints and sorry for the delay here. This does what you described. I checked with a TCP monitoring tool that simply replying to a message with embedded images doesn't send a TCP request. Aceman, could you please check whether this affect the attachment reminder. I typed in a few keywords and the reminder came up just the same.
Attachment #8931949 - Flags: review?(bzbarsky)
Attachment #8931949 - Flags: feedback?(acelists)
Comment on attachment 8931949 [details] [diff] [review] 1409458-kw-check.patch (v1) Review of attachment 8931949 [details] [diff] [review]: ----------------------------------------------------------------- Somehow I couldn't reproduce the original bug. I didn't get a hit at the server and the message didn't show the "external content" warning and only the "alt" text of the image was shown. On reply, nothing of the image was visible (not even the alt text). But the change to the attachment keyword checker seems to work fine. Boris, will the document be destroyed when newDoc var holding it goes away at the end of the function? Or do we need to cleanup manually? Also, is the creating of a new document expensive memory wise? Is it anything to be concerned about? ::: mail/components/compose/content/MsgComposeCommands.js @@ +6010,5 @@ > "mail.compose.attachment_reminder_keywords", > Components.interfaces.nsIPrefLocalizedString).data; > let mailBody = getBrowser().contentDocument.querySelector("body"); > + let newDoc = getBrowser().contentDocument.implementation.createDocument("", "", null); > + let mailBodyNode = newDoc.importNode(mailBody, true); Please add the comment why we are doing it like this. Somebody could get the itch to optimize this back to .cloneNode(true) in the future :)
Attachment #8931949 - Flags: feedback?(acelists) → feedback+
With my TCP monitoring tool I noticed network activity without the patch, but none with the patch. Test case: Reply to a message the has blocked http-based images. I'm sure Boris will take a look soon and also answer the question about the document being destroyed. I'll add a comment before checking it in.
(In reply to :aceman from comment #24) > But the change to the attachment keyword checker seems to work fine. Yep, |mozmake SOLO_TEST=composition/test-attachment-reminder.js mozmill-one| still passes.
Comment on attachment 8931949 [details] [diff] [review] 1409458-kw-check.patch (v1) This definitely needs documentation explaining why we're using a separate document. r=me with that. > Boris, will the document be destroyed when newDoc var holding it goes away at the end of the function? Well, after some GC and CC that happens after that point... But there's no additional cleanup needed, for sure.
Attachment #8931949 - Flags: review?(bzbarsky) → review+
Aceman, please let me know whether you like the comment.
Attachment #8931949 - Attachment is obsolete: true
Attachment #8933084 - Flags: review?(acelists)
Comment on attachment 8933084 [details] [diff] [review] 1409458-kw-check.patch (v1b) Review of attachment 8933084 [details] [diff] [review]: ----------------------------------------------------------------- Yes, thanks. ::: mail/components/compose/content/MsgComposeCommands.js @@ +6012,5 @@ > let mailBody = getBrowser().contentDocument.querySelector("body"); > + > + // We use a new document and import the body into it. We do that to avoid > + // loading images that were previously blocked. Content policy of the newly > + // created data document will block the loads. Details: Bug 1409458 comment #22. Kill the trailing space.
Attachment #8933084 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/5389c3dc10db use new document and importNode() to avoid loading of blocked images in attachment keyword check. r=bz,aceman
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
status-thunderbird58: --- → affected
status-thunderbird59: --- → fixed
status-thunderbird_esr52: --- → affected
Target Milestone: --- → Thunderbird 59.0
Comment on attachment 8933084 [details] [diff] [review] 1409458-kw-check.patch (v1b) Let's get this uplifted so the regression gets fixed. Boris, the fix will work on mozilla52 ESR, right?
Flags: needinfo?(bzbarsky)
Attachment #8933084 - Flags: approval-comm-esr52+
Attachment #8933084 - Flags: approval-comm-beta+
It should, yes.
Flags: needinfo?(bzbarsky)
Dan, have you tried the fix in a local compile or a Daily version?
Flags: needinfo?(dan-bugzilla)
Yes, the behavior I reported appears to be fixed in Daily -- thanks. The behavior reported by the OP, where single-clicking on an image in the reply window triggers a load, is still there (and note that going into Properties, as one comment above said, is not required). That's definitely a bug, since there's no reason for simply clicking once on a blocked image (which could easily be done accidentally) to trigger a load, but it's less serious.
Flags: needinfo?(dan-bugzilla)
Blocks: 1422860
I filed bug 1422860 to check the remaining single-click issue.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: