1310377 - EditAsNew could contain remote content, should sanitize like forwards

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Kent James (:rkent)]

If we take one of the wack-a-mole approaches (phrase introduced in a disparaging way at https://bugzilla.mozilla.org/show_bug.cgi?id=1151366#c88) then one of the moles to wack is that EditAsNew needs sanitization as it might contain remote content. This bug avoids the long comment stream on 1151366 to just give the standalone solution for that.

Depending on the resolution of bug 1151366 this may or may not be needed, but let's see how many moles there are to wack before we make our user's lives miserable.

Comment 1

[Kent James (:rkent)]

Attachment: Use new CompType for EditAsNew

Comment 2

[Jorg K (CEST = GMT+2)]

Good move.

We could really back out bug 1287268 now. It's only reason for existence was to detect an "own" edit, that is, to detect whether a draft or template is used for composition. We could never distinguish a template edit from a "edit as new" of a foreign message. With Kent's suggestion, we can. For example in MsgComposeCommands.js
  if (params.composeFields.creatorIdentityKey && params.composeFields.from)
would revert to
  if (gComposeType == nsIMsgCompType.Draft && params.composeFields.from)
but be supplemented by
  if ((gComposeType == nsIMsgCompType.Draft || gComposeType == nsIMsgCompType.Template) &&
       params.composeFields.from)
See https://hg.mozilla.org/comm-central/rev/1b6364120f38#l1.38.

Anyway, we can do that in a follow-up bug if we so decide.

Comment 3

[Jorg K (CEST = GMT+2)]

If I understand this correctly, you want to distinguish "edit as new" of a template (which is assumed to be clean) from "edit as new" of a foreign message. Actually, no, looking at the patch. You want to distinguish double-clicking (=using) of a template from "edit as new" of anything, template or foreign message. I actually didn't know that I can double-click a template, I always used "edit as new".

See
https://dxr.mozilla.org/comm-central/search?q=Components.interfaces.nsIMsgCompType.Template&redirect=false
for the current uses of nsIMsgCompType.Template.

Sanitisation would only take place for "edit as new".

This would shift the attack vector. Now the attacker would have to get the victim to move the foreign message to the Templates folder and then double-click it. Sure, absurd, but possible.

What would be wrong with always sending nsIMsgCompType.Template through TagEmbeddedObjects(mailEditor)?

Your patch would then reduce to one hunk:
+ if (isForwarded || mType == nsIMsgCompType::Template)
+   (void)TagEmbeddedObjects(mailEditor);

Would that damage the template? We could prescribe that the user needs to set moz-do-not-send=false on anything they want to send with the template.

You've looked at it for longer than I have, so maybe I'm missing something.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #3)
> What would be wrong with always sending nsIMsgCompType.Template through
> TagEmbeddedObjects(mailEditor)?
> Your patch would then reduce to one hunk:
> + if (isForwarded || mType == nsIMsgCompType::Template)
> +   (void)TagEmbeddedObjects(mailEditor);
> Would that damage the template?
Maybe I missed the point and that would damage the template.

Could we somehow check whether this is a "real" template be seeing whether the creatorIdentityKey is set (see comment #2). That creatorIdentityKey tells us whether the message was saved (not just moved) by us and can therefore be considered clean.

That would eliminate the attack vector completely: To convert the attacker's message into a template it will not be sufficient to get the victim to move it to the Templates folder. A "real" template must be saved, and in order to save it, you need to "edit as new" first (which will sanitise) or paste from it (which will also detect the attack).

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: Sanitise message during "edit as new" if it's not a trusted template.

Untested but compiles ;-)

This is what I had in mind. Advantages:
Foreign message in the Templates folder will be sanitised if double-clicked. Eliminates attack vector.
Also, "edit as new" on "real" template won't sanitise it. That's how I use templates.

Comment 6

[Kent James (:rkent)]

"If I understand this correctly, you want to distinguish "edit as new" of a template (which is assumed to be clean) from "edit as new" of a foreign message." Yes that was my intention. Really this is an exploration of how that might be done. My patch might not accomplish the goal, as I just looked at one path where "EditAsNew" is used.

"That creatorIdentityKey tells us whether the message was saved (not just moved) by us and can therefore be considered clean."

I considered using header values, but the problem is that there is nothing preventing an attacker from also adding those header values, even though it is not typical. A header that I had considered was "Received" which is never present on our messages, but typically (though not necessarily) on foreign messages because it is added usually by our ISP when we receive the message, out of control of the attacker. Even if we do include the suggestions from this bug, we might also want to implement a check for a "Received" header as an additional protection on the (assumed clean) Drafts and Templates folder.

I'm still trying to figure out what my position is going to be on bug 1151366. I'm really uncomfortable introducing major breaking changes at an ESR release, but I need to understand reliable sanitization is even possible if we can identity all of the moles that need wacking. This current bug is one such issue.

There will always be a sequence of operations that an attacker could ask a victim to do that will cause information disclosure ("please decrypt your password file and email it to me").

Comment 7

[Kent James (:rkent)]

Comment on attachment 8801508 [details] [diff] [review]
Sanitise message during "edit as new" if it's not a trusted template.

Review of attachment 8801508 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +753,5 @@
> +      if (mType == nsIMsgCompType::Template) {
> +        // Check where this is our own template or some other message
> +        // edited as new.
> +        char* cik;
> +        m_compFields->GetCreatorIdentityKey(&cik);

As I said, what prevents an attacker from adding this key?

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #6)
> I'm really uncomfortable introducing major breaking changes at an
> ESR release, ...
I'm really uncomfortable breaking users' current editing behaviour where they paste a lot of things into the composition and expect that stuff to be sent. That applies to any release. But let's not carry this discussion over here ;-)

(In reply to Kent James (:rkent) from comment #7)
> As I said, what prevents an attacker from adding this key?
Right. This is about the X-Identity-Key header (HEADER_X_MOZILLA_IDENTITY_KEY). Surely we look at headers upon receipt. One that is really ours we could strip if we ever receive it. Argument: It confuses our internal processing (and it does).

What I don't like about your solution is the compose type that depends on the user action, double-click in special folder vs. "edit as new". IMHO it would be better to base the processing on the data.

Comment 9

[Kent James (:rkent)]

Comment on attachment 8801508 [details] [diff] [review]
Sanitise message during "edit as new" if it's not a trusted template.

Review of attachment 8801508 [details] [diff] [review]:
-----------------------------------------------------------------

Just to keep the feedback? count down, let me - this. But I am still open to considering this approach, there are just more issues here.

Comment 10

[Kent James (:rkent)]

For the record, let me state what I think is the objective of this bug, that may or may not be different from that of bug 1151366.

Reviewing again that bug, it mentions two attack paths: EditAsNew and HTML attach. The sanitizing methods are different for these two cases. This bug is concerned with the EditAsNew and related phenomena (moving a message to Drafts or Templates and double clicking) only. I'll leave bug 1151366 for possible sanitizing of paste operations.

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #9)
> But I am still open to considering this approach,
> there are just more issues here.
Yes, the attacker could supply a malignant template with a X-Identity-Key header, ask the user to extract that message attachment and use it as a template.

(In reply to Kent James (:rkent) from comment #0)
> ... let's see how many moles there are to wack before we make our user's
> lives miserable.
Yes, there seem to be more moles here. And as Magnus suggested, all these can be whacked with those data URIs (or another protocol), but the data URIs alone only solve the paste problem, which is the greater threat, by disabling paste altogether.

Humour, and pardon the pun:
Kent wrote in comment #0: "... before we make our user's lives miserable". I'd like to deny that we have a single user with multiple lives. We should aim for a solution that doesn't require the user to have multiple lives to survive it ;-)

Comment 12

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-moderate because it sounds like this may fix a sec-moderate.

Comment 13

[Jorg K (CEST = GMT+2)]

This is not needed any more with the solution landed in bug 1151366.

Comment 14

[Magnus Melin [:mkmelin]]

I suppose attachment 8801410 [details] [diff] [review] could be of general interest though?

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #14)
> I suppose attachment 8801410 [details] [diff] [review] could be of general
> interest though?
Indeed, reused in bug 731688 and landed as https://hg.mozilla.org/comm-central/rev/ace64c7317e8 but without the sanitise hunk.

Re. that sanitise hunk that as was suggested in the original patch:

-      // when forwarding a message as inline, tag any embedded objects
+      // when forwarding a message as inline, or editing as new (which could
+      // contain unsanitized remote content) tag any embedded objects
       // which refer to local images or files so we know not to include
       // send them
-      if (isForwarded)
+      if (isForwarded || mType == nsIMsgCompType::EditAsNew)
         (void)TagEmbeddedObjects(mailEditor);

Attack vector:

If you get the victim to use a supplied message as new or as template, you can still extract images (or documents, I haven't tried) from the victim's intranet by placing this into the supplied message:
<img src="http://intranet/the-boss/secret/company-secret.png" moz-do-not-send="false">

Kent's hunk turned moz-do-not-send="false" into moz-do-not-send="true", thus changing the original message or template which potentially contained an image that should have been attached.

Maybe we continue the discussion here a little: Magnus and Kent, do you think we should sanitise on "edit as new" or even "new message from template"?

Comment 16

[Jorg K (CEST = GMT+2)]

CC'ing Aceman and Thomas since we came here from bug 731688.

Comment 17

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #15)
> Maybe we continue the discussion here a little: Magnus and Kent, do you
> think we should sanitise on "edit as new" or even "new message from
> template"?

I think we should sanitize for "Edit as New", since that can be any message. For templates I think not, as we can be pretty sure they are our own. Yes, you could trick someone into putting a message into Templates, but it's beginning to be a bit of a stretch to think that would be a successful attack.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1310377-sanitise-for-edit-as-new.patch

Kent's original hunk. Everything else has landed in bug 731688.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 8907224 [details] [diff] [review]
1310377-sanitise-for-edit-as-new.patch

Review of attachment 8907224 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +767,2 @@
>        // which refer to local images or files so we know not to include
>        // send them

I'd change this two lines to

// so what objects to include or not (on send) is not decided behind the user's back.

And capitalize the When

Comment 20

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/309b9671d6a51c6756bc5b9e772cbceba423feb4