1572864 - Inline image is lost on reply; URL that refers to the sender's IMAP service is exposed to recipients

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Mabry Tyson]

Attachment: inline_img_msgs.tgz

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

Setup:
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0)
Gecko/20100101 Thunderbird/60.8.0
We are using a UW Imap server (on Solaris) and Gmail.com IMAP server.

My Thunderbird is configured for replies to quote the original message inline and to put my text in front of it.
Forward messages inline.
"Send messages as plain text if possible." is checked.
"Send the message in both plain text and html." is selected.

The script below has worked the handful of times I've tried it with our UW IMAP server. Another user (same TB & server) has the problem appear at step 3 rather than waiting until step 5.

It did not produce the problem when I initially tried it with the gmail.com server. I did 3 recursive replies without the problem.

However, when I turned on IMAP debugging and used gmail.com, the first reply (step 3) shows the problem as is documented in step 5 (no image in the composition window, IMAP: URL in the reply when it is received. I am attaching an IMAP log from TB showing the problem.

1. I send a message to myself with an inline image. (The problem is identical if I am sending mail to someone else. I send it to myself to make it easily reproduced.)

2. I receive that as Message 1 in my INBOX: The original inline image appears as expected in the received mail. Message 1 is a multipart/related with one part text/html and one part image/png (disposition inline). The text/html has a reference to the image/png as in
   <img src="cid:[e-mail]" alt="" class="">

3. I compose and send a reply to Message 1 (with the original message quoted inline, as is my default). The original image appears in the composition window.

4. I receive the reply as Message 2. The inline image appears as expected in the received mail. Message 2 is a multipart/related with one part text/html and one part image/png (disposition inline). The text/html has a reference to the image/png as in
   <img src="cid:[e-mail]" alt="" class="">

5. I compose and send a reply to Message 2. The inline image does NOT appear in the compose window.

6. I receive that 2nd reply as Message 3. When I read it, the inline image does NOT appear. Message 3 is content type text/html with a reference where the image should be as
   <img
   src="imap://[e-mail]:143/fetch%3EUID%3E/MY_MAIL_FOLDER%3E9275?header=quotebody/;section=2?part=1.2&filename=eacofmnadelkdcjp.png"
   alt="" class="" moz-do-not-send="true">

The above illustrates the problem. I did the following as well, but the results were not what I expected, and may inform what the issue is. I believe that these next steps demonstrate that this problem is not due to the structure or content of the message.

7. Within TB, I copied Message 2 to a different IMAP folder (same account). Obviously the message content is still identical to Message 2, but call it Message 2b. It is the only message that I'm reporting on that was in a different mail folder. When I read Message 2b, the inline image appeared as it should (and as it did in step 4).

8. I compose and send a reply to Message 2b. I expected the same issue as step 5 above, but that's not what happened. The compose window for the reply DOES show the inline image as it should (but did not in step 5)

9. I received that reply as Message 4 in my inbox. When I read Message 4, the inline image appears. The source contains the CID image.

10. I compose and send a reply to Message 4. The compose window does NOT show the inline image.

11. I received that reply as Message 5 in my inbox. When I read Message 5, the inline image does not appear. The source of Message 5 does not contain the CID image but instead has the URL (as in step 6 above)

Actual results:

(UW Server) At steps 5, 6, 10, and 11, the quoted inline image does not appear. A URL referencing my mail file is in the message sent to the recipient (in this case me, but the same thing happens if I am sending mail to someone else).

We have a user (Mac TB 60.8.0) for whom the FIRST reply to a message with an inline image causes the URL to be delivered rather than the image.

(Gmail.com server with no logging) The problem did not arise through three iterative replies. The inline image appeared in compose window each time and in the received message.

(Gmail.com server with IMAP:5 logging) At step 3, the quoted inline image does not appear. A URL referencing my mail file is in the message as received byP the recipient.

Expected results:

1. The inline image should have always been visible.

2. In no case should the URL referencing my mail file ever get into an outgoing mail message.

3. The behavior should be the same for all users, and for the same user in different states.

4. The behavior should be the same whether IMAP logging is enabled or not.

Anonymized versions of the messages (for the UW IMAP case) are attached. These are all from my INBOX. I examined the versions of messages 1, 2, 3 in my SENT mail folder to confirm they had the same content (ignoring headers) as the received messages. [The mail goes through Microsoft and so the body is potentially mangled. But I received what I sent.]

The (shortened & annotated) IMAP log when I am connected to the Gmail.com server is attached. You can see the problematic message (generated by Reply) when it stores it in the SENT folder. This should be the most easily reproduced case and is the best documented.

Comment 1

[Jorg K (CEST = GMT+2)]

I cannot reproduce step 5. I can reply as many times as I want, the image is always included in the compose window. I tried with my own IMAP server and Gmail.

What happens behind the scenes is that in the message you're viewing, the reference to the image is something like like the imap URL you're showing in step 6. When you're replying, the picture is fetched and inserted into the composition as a data URL. You can inspect the HTML of the message, either by selecting it and using "Insert > HTML" or using the add-on ThunderHTMLedit.

Irrespective of how many times you reply, that replacement should always work. I'm sure you have errors in the error console, Tools > Developer Tools > Error Console. What are these errors?

If changing the imap URL to a data URL fails, the message will go out with the original URL, that's unfortunate.

BTW, the mechanism to convert imap (or mailbox) URLs to data URLs has been in place since TB 52 and we've never heard this complaint.

Just for reference: Can you try out TB 69 beta? https://www.thunderbird.net/en-US/thunderbird/beta/all/

Gene, in the attachment there's an IMAP log (imap.log.gmail.201908091047-short.txt). Can you see anything in there?

Comment 2

[Jorg K (CEST = GMT+2)]

One more question to the reporter: The received messages are stored in your IMAP inbox, right? Is that synchronised for offline use? Check the properties: Right-click on folder, Properties.

Comment 3

[gene smith]

Attachment: test-with-this-email-please.eml

Reporter,I tried to duplicate the problem with the attached email since I couldn't really tell much using your "msg1" file since the image was intentionally corrupted. Like Jorg, I don't see the problem. You can save and drag this file to a folder (using a file manager) and "edit as new" and send this message to yourself as your steps indicate. Does this email cause a problem? (You may have to click on the green area to see the fully rendered image due to another ongoing issue.)

Also, if possible, can you attach a complete displayable email with an image that you see cause the problem? If the email I attached doesn't cause a the problem this would help.

I looked at the gmail log but haven't studied it in detail. But you say you only see the problem with gmail server when IMAP logging is active? I will try that now.

Comment 4

[gene smith]

I've sort of been able to duplicate the problem using the reporter's msg1 email fixed up by appending a larger image to it that exceeds the mime "fetch on demand" threshold which is 30,000 bytes. However, I only see a problem if I have the setting "View Attachments Inline" turned off. The fixed up msg1 email setting in inbox still displays the image but when I do a reply, I only see the text in the message and no image or attachment links. Also, it happens on the first reply. If I send the message (to myself) I see only imap urls in the source and not the actual image data, and the received message is only text.

If I increase the fetch on demand threshold to 300,000, the reply shows the complete message including image and, when sent, the received message shows the complete message and source shows the image data and not just imap urls.

Typically when the setting for "view attachments inline" is set and all the mime parts of the message are viewable inline, the whole message is fetched, all in one piece. So I'm curious to know if the reporter has "Display attachments inline" set or not. I can see the message being fetched in parts in the gmail imap:5 log so it appears that it is not set.

Reporter, the message I attached will probably always work because the image is less than 30000 bytes. According the imap log, your image was about 90K bytes so it exceeds the threshold for fetch on demand.

P/S: While working on caching issues I think I have seen this problem before.

Comment 5

[Mabry Tyson]

Attachment: inline_image.png

This is the inline image from the original message in my Google account to which I replied.

Comment 6

[Mabry Tyson]

[Sorry, I was called out of town for an emergency and was not able to respond until now.]

(In reply to Jorg K (GMT+2) from comment #1)

If changing the imap URL to a data URL fails, the message will go out with the original URL, that's unfortunate.

In my opinion, that's not just unfortunate. It is exporting information that should not be revealed to the recipients of the email. As I said in my original, this should not be included (even if the image can not be inserted). This reveals potentially sensitive information about the sender's mail (imap server & port, account name, mail folder name, message number). In the original mail that led me to this bug, person 1 sent mail to person 2. When person 2 replied, the imap url was included. Hence person 1 can trigger person 2 into revealing this info.

If nothing else, I encourage the developers to plug this information leak.
While it is unfortunate that the image is not included in some cases, the important issue to me is to avoid this leakage.

Just for reference: Can you try out TB 69 beta? https://www.thunderbird.net/en-US/thunderbird/beta/all/

Using 69.0b2 (64-bit) and my gmail account, I replied to the message with the inline image. (I did NOT enable html logging). The image did not appear in the compose window. The imap URL was in the received message. The error log showed this error (when the Compose window is visible, before sending):

Security Error: Content at moz-nullprincipal:{df110209-ed38-4ad9-9adb-7efeee84d483} may not load or link to imap://[e-mail]:993/fetch%3EUID%3E/INBOX%3E9349?header=quotebody/;section=2.2?part=1.2.2&filename=mkmiobjofinbejdp.png.

(In reply to Jorg K (GMT+2) from comment #2)

One more question to the reporter: The received messages are stored in your IMAP inbox, right? Is that synchronised for offline use? Check the properties: Right-click on folder, Properties.

Yes, these messages are in INBOX. I do NOT synchronize this folder.

(In reply to gene smith from comment #4)

Typically when the setting for "view attachments inline" is set and all the mime parts of the message are viewable inline, the whole message is fetched, all in one piece. So I'm curious to know if the reporter has "Display attachments inline" set or not. I can see the message being fetched in parts in the gmail imap:5 log so it appears that it is not set.

I don't see that setting in Preferences (at least in Beta).
"Allow remote content in messages" is unchecked.
In about:config, when I search for "inline", the only one that isn't default is
mail.inline_attachments: false

Reporter, the message I attached will probably always work because the image is less than 30000 bytes. According the imap log, your image was about 90K bytes so it exceeds the threshold for fetch on demand.

When I load your message in TB 69.0 (click right on file and specify open with TB) and look at the source, I see it is a multipart/alternative. One part is text/plain and has no reference to any image. The other part is a multipart/related, and contains a text/html part and 4 image/gif parts, all content-disposition:inline (id:a gwava6.gif, id:b gwavabars.gif, id:c release.gif, id:d myqms.gif). The HTML references cid:a and cid:b, but cid:c and cid:d are not referenced. The two referenced images (a&b) are shown inline while the other two are not. TB shows two attachments (release.gif and myqms.gif).
NOTE: This structure is different than the structure in the reported problem.

When I do a REPLY, in the compose window, the two referenced images are not (initially) shown nor are the two unreferenced images. At the bottom of the compose window is an alert "Thunderbird has blocked a file from loading into this message. Unblocking the file will include it in your sent message" with a Preferences button. The Preferences button allows me to unblock either/both of mailbox://<the local file with your .eml file>&part=1.2.{2,3}&filename={gwava6,gwavabars}.gif. If I do unblock them, the images are shown inline.
NOTE: This alert does not appear when I do the Reply in the reported problem.

When I receive that reply, the structure is the same, except the two unreferenced images are not included, and the IDs of the two referenced images are different. If I then reply to that, the inline images are included in the compose window.

---

I appreciate that Gene Smith recognized that size of the image was important, and was able to replicate the problem.

To clarify how the original message (per account) was created, I created a new message addressed to myself and pasted (Command-V) an image that I had copied within another app directly into the compose window. I generally typed a single line to describe the image, and emailed it. All the failures were on replies to such an original message. I've attached to the bug report the image that was in the original message on the Google account.

Comment 7

[Jorg K (CEST = GMT+2)]

A few answers here:

• "Unfortunate" was a clear understatement, I'm aware of the leak.
• The security error is expected. We catch it and load the image from the location, unless is fails, like in your case. We could detect that failure better and the null out the reference so it doesn't leat. However, the aim is to make it work all the time.
• Not synchronising the inbox is a problem, since then the image may need to be fetched for a reply.
• View attachments inline is on the menu: "View > Display Attachments Inline". Also driven by the mail.inline_attachments pref.
• When I reply to Gene's message, both from a local folder or IMAP folder, I don't get any "unblock this image" notification. And I shouldn't since the embedded images should just be transferred to the reply, the unreferenced "attachments" are not part of a reply.

I think we first need to work out what's going on here, then we can improve the error handling and null out any unresolved reference. BTW, that's happening here:
https://searchfox.org/comm-central/rev/b0bfd7596c6d6f0ef580ce2c9cd680b2e6ca0bf1/mail/components/compose/content/MsgComposeCommands.js#6731
BTW, in cases where the image is not included in the reply, do you see another error in the error console, other than the security error which is expected?

Comment 8

[Kai Engert [:KaiE:]]

Let me try to summarize the issue.

In some scenarios, outgoing email sent by Thunderbird will leak information about the username, server name, folder name, message number, for the sender's mail server.

I think in many cases the server name is probably easy to identify. For example, Ari, if I lookup "imap.your-email-domain", I get a result, and I can connect to port 993. Also, the MX record of your domain points to a large provider that is used by many companies. I guess it's quite likely that your username is identical with your full email address? Listing all records of your domain using "dig +nocmd ai.sri.com any +multiline +answer" reveals a rather large set of potential sender IP addresses for the SPF record. It would be easy to run a port scan to find servers that listen on IMAP ports.

My point is, the username and the hostname / IP of the imap server are probably not very confidential information. In order to access that mail account, an attacker would still be required to guess passwords, or run a brute force attack. Users should have strong passwords to make that harder, and servers should rate limit the amount of login attempts, I think that's the usual protection measures.

The leak of the folder name seems more inconvenient. If the sender had stored your email in a folder named "annoying-people", that would be a very unconvenient leak.

I think we should indeed avoid sending out such URLs in messages.

Comment 9

[Kai Engert [:KaiE:]]

Besides learning information about the user's server configuration, are there any other issues?

What happens if an attacker sends an HTML email with a carefully crafted URL, that points to other message numbers?

If the recipient replies, might Thunderbird fetch that other message, and include the contents in the reply?

In other words:

• attacker learns about the correct IMAP URLs used by the victim's configuration
• attacker sends an email to victim, which contains an <img> tag, that point to another message number
• victim replies.

If the victim replies, will Thunderbird set the contents of the image to the contents of that referenced message number?

We should test that doesn't happen.

Comment 10

[Kai Engert [:KaiE:]]

Jörg, thanks for looking into this issue and identifying the source location that's related. Who should work on a fix? Who could test that the scenario from comment 9 doesn't work?

Comment 11

[Jorg K (CEST = GMT+2)]

Kai, the entire compose pipeline was reworked for the "stolen file bug", bug 1151366.

The way it works is that when the document is prepared for reply, any content is blocked. We carefully load content from the same origin as the original message, nothing else. If you send other "probes", they will not be satisfied. The code is here:
https://searchfox.org/comm-central/rev/b0bfd7596c6d6f0ef580ce2c9cd680b2e6ca0bf1/mail/components/compose/content/MsgComposeCommands.js#6724
So there is absolutely NO problem.

If loading the image fails here:
https://searchfox.org/comm-central/rev/b0bfd7596c6d6f0ef580ce2c9cd680b2e6ca0bf1/mail/components/compose/content/MsgComposeCommands.js#6731
you can make the src attribute of the error target, the blocked image, null with one additional line:
event.target.src = null;
I'll attach a patch in a moment.

The interesting question remains: Why can IMAP not load the image when called through loadBlockedImage() here:
https://searchfox.org/comm-central/rev/b0bfd7596c6d6f0ef580ce2c9cd680b2e6ca0bf1/mail/components/compose/content/MsgComposeCommands.js#6986

Since I can't reproduce the bug (and I have other commitments), I can't work on it.

Comment 12

[Jorg K (CEST = GMT+2)]

And besides: The new compose pipeline has been working since TB 52, I've never heard about any problems of lost images. And as you said, the information leaked is already 95% guessable from the e-mail received.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-null-img-src.patch

Gene, if you can reproduce it, you can test this one-liner and see whether it does the trick.

Comment 14

[gene smith]

Reporter Mabry: The setting I was referring to is under the View menu item and is a checkbox called "Display Attachments Inline". It applies to all accounts and folders in TB when set and it defaults to checked I think. If not checked, attachments don't display inline. However, the image in your email will appear inline regardless since it is marked as "inline" and not "attachment" in the mime header. Anyhow, how do you have this, checked or un-checked?

Comment 15

[gene smith]

(In reply to Jorg K (GMT+2) from comment #13)

Created attachment 9085884 [details] [diff] [review]
1572864-null-img-src.patch

Gene, if you can reproduce it, you can test this one-liner and see whether it does the trick.

Will do ASAP.

Comment 16

[Mabry Tyson]

(In reply to gene smith from comment #14)

Reporter Mabry: The setting I was referring to is under the View menu item and is a checkbox called "Display Attachments Inline". It applies to all accounts and folders in TB when set and it defaults to checked I think. If not checked, attachments don't display inline. However, the image in your email will appear inline regardless since it is marked as "inline" and not "attachment" in the mime header. Anyhow, how do you have this, checked or un-checked?

Sorry, I was looking in preferences. It is unchecked.

Comment 17

[gene smith]

Mabry, Good, unchecked (attchments only show as links at the bottom and not inline) is consistent with what I observe. So a temporary workaround until this is fixed would be to set it to checked.
Edit: Another possible temporary work-around is to set Inbox folder so it has offline store and repair the folder. The repair will force a download of each message in one piece and store it into the database. Now, even when attachments are not displayed inline, tb will just fetch the stored message from the database in one piece and display it entirely when you do a reply. However, you may have reasons for not wanting to store all messages for offline use.
Edit2: Sorry, I really didn't need to ask you about the check/unchecked thing since, on a 2nd read, I see you said "mail.inline_attachments: false" which tells me it is unchecked.

Comment 18

[gene smith]

(In reply to gene smith from comment #15)

(In reply to Jorg K (GMT+2) from comment #13)

Created attachment 9085884 [details] [diff] [review]
1572864-null-img-src.patch

Gene, if you can reproduce it, you can test this one-liner and see whether it does the trick.

Will do ASAP.

The change doesn't seem to do anything. The url's are still showing up in the received message like before with no visible image. Do see the "broken picture" icon when received with trunk build but not with 60.8 release. I then added a "dump" call after the one-liner and I don't see it occurring either on re-test.
Edit: The above may sound like I expected this to cause the image to display, but I realize that it doesn't. I think the idea is that the imap:// url in the received message source should just be removed.

Comment 19

[Jorg K (CEST = GMT+2)]

Thanks for checking, Gene. I wonder whether loadBlockedImage() actually throws and event.target.src = null; is executed. If the catch block were entered, we also see some additional error in the console, see comment #7, last line. Please check that.

If it doesn't throw, then we need some debugging in the function itself:
https://searchfox.org/comm-central/rev/b0bfd7596c6d6f0ef580ce2c9cd680b2e6ca0bf1/mail/components/compose/content/MsgComposeCommands.js#6986

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9085884 [details] [diff] [review]
1572864-null-img-src.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6735,2 @@
>            Cu.reportError(e);
> +          event.target.src = null;

I think you should null it out only if it's not http/s.
Probably best to remove the "src" attribute?

Comment 21

[Jorg K (CEST = GMT+2)]

That's for the suggestion. But apparently that code isn't even run, see comment #19. I must see whether I can set it up with a picture larger than mail.imap.mime_parts_on_demand_threshold on a non-sync folder.

Comment 22

[Jorg K (CEST = GMT+2)]

After stopping the crash from bug 1573639, I can now reproduce the issue with the threshold set to 300 (bytes?) from 30000 (30 KB). As Gene pointed out, if I set attachments to display inline, the problem doesn't occur. So there's certainly an IMAP bug lurking there.

I see how to improve nulling out the undesired reference.

Comment 23

[Jorg K (CEST = GMT+2)]

I did a bit of debugging, the result is surprising. In case of the low threshold and on a non-sync folder, composition returns here:
https://searchfox.org/comm-central/rev/b0bfd7596c6d6f0ef580ce2c9cd680b2e6ca0bf1/mail/components/compose/content/MsgComposeCommands.js#6697
Yes, on checking the "do not send" attribute. No idea what's happening, but it's not an IMAP issue. Removing the return makes the thing work. Strange.

Comment 24

[Jorg K (CEST = GMT+2)]

The damn URL has a semicolon in it:
=== imap://[e-mail]:993/fetch%3EUID%3E.INBOX.marc
h%202019%3E94?header=quotebody/;section=2.2?part=1.2.2&filename=FR%20%E2%80%94%2
0Se%CC%80te%20%E2%80%A2%2011.3.16%20#0234%20100cm%20copy.jpg
and we don't expect that here, grrrr:
https://searchfox.org/comm-central/rev/cb71ec93ae822bce37fb00e321ef201cce24ff1e/mailnews/compose/src/nsMsgCompose.cpp#268

One-liner to fix.

Comment 25

[Jorg K (CEST = GMT+2)]

Not quite, but close, the issue is that there are two ? in the query, grrrr. If we switch inline display on, we get:
=== imap://[e-mail]:993/fetch%3EUID%3E.INBOX.marc
h%202019%3E94?header=quotebody&part=1.2.2&filename=FR%20%E2%80%94%20Se%CC%80te%2
0%E2%80%A2%2011.3.16%20#0234%20100cm%20copy.jpg

Sigh.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-fix-missing-image.patch

This fixes the issue. It replaces stone-age string manipulation with the code from here which has always worked:
https://searchfox.org/comm-central/rev/cb71ec93ae822bce37fb00e321ef201cce24ff1e/mailnews/imap/src/nsImapProtocol.cpp#8883

Magnus, do you still want to null out the src attribute when the image can't be loaded, which wasn't the issue here at all.

BTW, that bug has always existed since TB 52.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-fix-missing-image.patch

Fixed commit message, sorry about the noise.

Comment 28

[gene smith]

Jorg, I tested your patch and it fixes the problem so the reply messages now contains all the inline image data and the imap:// links are not sent. However, what I don't understand is how the patched function would now ever return false. In other words, how would orginalPath ever differ from the the trimmed path? Of course, if there is a way for the function to return false, the imap:// link will be put into the message which is the security issue (if I understand right).

Also, your trimming of path seems to assume the /;,if present, will occur before the ?'s which I suppose is correct. Would it be possible to just compare originalPath with path for strlen(originalPath) chars and not bother with finding ?'s or /; ?

Comment 29

[Jorg K (CEST = GMT+2)]

You gave the answer yourself. Someone evil might send a prepared message. In normal operation, this should not return false.

Your other suggestion is almost right, but it's safer to compare the entire path. Otherwise the first strlen(originalPath) could be the same and the evil person gets to extract something from a longer and unequal path.

Comment 30

[gene smith]

From your example in comment 25 slightly modified:

orig = /fetch%3EUID%3E.INBOX.march%202019%3E94
path = /fetch%3EUID%3E.INBOX.march%202019%3E944?header=quotebody&part=1.2.2&filename=FR%20%E2%80%94%20Se%CC%80te%20%E2%80%A2%2011.3.16%20#0234%20100cm%20copy.jpg

I see, in the above case we would want the function to return false. (The orginal path was for uid 94 and the evil path is for uid 944. With my suggestion it would return true and send the image if uid 944 exists and contains the mime part.)

Comment 31

[gene smith]

Attachment: Re: Inline image.eml

With the patch in place I drop the attached .eml into a folder of a non-gmail account. As you can see the .eml has two links to message UID 6118 in a gmail account. I then attempt to reply to the message from the non-gmail account. As expected, the patched function returns false and the inline images don't appear in the reply since the replied to message is not UID 6118. I don't attempt to send the reply.

However, under the Options menu of the reply screen there is a "Quote Message" selection. If I click that I then see a duplicate of the message and in yellow at the bottom there is a message about "Daily has blocked some files from loading, unblocking will include it in your message". I then click "Preferences" and can click to unblock the imap:// url's for the missing files. The links are, of course, to the files in the gmail account folder at UID 6118 which still exists. Clicking them bring in the inline images into the reply email and in the appended "quoted" email (actually one email ready to send). I can then delete the quoted emails so I only have one copy of each image in the email which could be sent.

So this seems to let the user override the missing inline images due to the patched function returning false. However, it seems that the yellow "unblock" message should appear immediately upon doing the reply and not only after the original message is quoted. (I think the reporter Mabry noticed something similar in comment 6 but didn't have to "quote the message" in order to see the unblock prompt.)

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086263 [details]
Re: Inline image.eml

Please attach messages as text/plain for easier viewing in BMO.

Comment 33

[Jorg K (CEST = GMT+2)]

If I reply to "Re: Inline image.eml" and inspect the message with ThunderHTMLedit, I see

<img src="imap://[e-mail]:143/fetch%3EUID%3E/INBOX%3E6118?header=quotebody/;section=2.2?part=1.2.2&amp;filename=gwava6.gif" alt="" class="" moz-do-not-send="true">

so as expected, moz-do-not-send was added.

I've never seen the "Quote Message" menu item in the Options menu, and that after about five years on the project, but hey, we can still discover new functions every day. That inserts

<img src="imap://[e-mail]:143/fetch%3EUID%3E/INBOX%3E6118?header=quotebody/;section=2.2?part=1.2.2&amp;filename=gwava6.gif" alt="" class="">

without the moz-do-not-send since most likely the "Quote Message" didn't run through the sanitiser code. Whether it should, is a different issue for another bug. When unsafe content is added to the compose window, we trigger that blocked content notification with the option to include the blocked content.

I think that works as designed. Whether we want to change that design is a different question. This bug here is about fixing an obvious bug where some URLs are not handled properly and hence some username/server information is leaked.

Comment 34

[gene smith]

I was trying to find and test a situation where the patched IsEmbeddedObjectSafe() function returns false. I had intentionally edited out the moz-do-not-send=true before dropping the file into the non-gmail folder but not sure that mattered. I didn't intend to get into this "quote"/"unblock" thing.

I also had never before clicked on even noticed the "quote message" function When I clicked it, I was looking for a way to see the .eml source of the reply message. I have never seen this "unblock" dialog before either. It seem rather strange since it is not clear what it means when it occurs (calling the unblock button "Preferences" is really not a good name).

I think this is a another bug since it allows an evil dropped-in eml file to link to and send content from another account.

So, for this bug, something is still needed to stop the imap:// url's from being included in the message, right?

Comment 35

[Jorg K (CEST = GMT+2)]

The unblock notification is mostly there to unblock file: URLs which were copied if someone copies HTML into a message containing such references. It was implemented in the "stolen file bug", bug 1151366. That you will be asked to unblock some imap: URLs is a side-effect.

I repeat the end of comment #33: This bug here is about fixing an obvious bug where some URLs are not handled properly and hence some username/server information is leaked.

I think this is a another bug since it allows an evil dropped-in eml file to link to and send content from another account.

What is the bug? The attacker can send you some "probes" and you will be notified. Of course we could suppress/remove any mailbox: or imap: URLs which would cause this. We can have the discussion on whether to be even more defensive in another bug. Options include:

• remove all unsafe references to other messages without even prompting
• Make "Quote Message" more defensive.

Comment 36

[gene smith]

(In reply to Jorg K (GMT+2) from comment #35)

The unblock notification is mostly there to unblock file: URLs which were copied if someone copies HTML into a message containing such references. It was implemented in the "stolen file bug", bug 1151366. That you will be asked to unblock some imap: URLs is a side-effect.

I repeat the end of comment #33: This bug here is about fixing an obvious bug where some URLs are not handled properly and hence some username/server information is leaked.

So your patch fixes this bug? That's what I'm asking. At one point above you were also wanting to null out the imap:// url in case the patched function returned false. With the patch, do you think this is no longer necessary?

I think this is a another bug since it allows an evil dropped-in eml file to link to and send content from another account.

What is the bug? The attacker can send you some "probes" and you will be notified. Of course we could suppress/remove any mailbox: or imap: URLs which would cause this. We can have the discussion on whether to be even more defensive in another bug. Options include:

• remove all unsafe references to other messages without even prompting
• Make "Quote Message" more defensive.

Not sure this is a bug: I drop the "crafted" message file into a folder and reply to it. It initially comes up with the images missing because the patched function returns false. There is no option presented to "unblock" the images. I send the email and the imap:// links are sent with the message. But if before sending I "quote" the message then I get the "unblock" prompts and can send the literal images and not just imap:// links. If I don't unblock, just the links are sent. (Anyhow, I know I am repeating myself, but when I know the resolution of this bug, I will fork this issue to a new bug.)

P/S: Thanks for cc'ing me on the other bug, I can see it now.

Comment 37

[Jorg K (CEST = GMT+2)]

Yes, the patch fixes the bug. I've considered nulling the reference if loadBlockedImage() fails, but we have never seen this and it cannot be tested, other than be faking a failure. Let's see what Magnus says.

The "stolen file" bug isn't a good read with 400+ comments :-(

Comment 38

[Magnus Melin [:mkmelin]]

I think if we find more than one question mark we should just assume that's a bad url and not do any further comparisons.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086032 [details] [diff] [review]
1572864-fix-missing-image.patch

> I think if we find more than one question mark we should just assume that's a bad url and not do any further comparisons.

Huh? The URLs cited in the comment occur routinely in MailNews. Sure, we can clean this up one day. This bug is about detecting the path correctly. And we do this already in IMAP, hence I copied those four lines. Do I need to find a different reviewer?

Comment 40

[Magnus Melin [:mkmelin]]

If we're creating bad url like that I think it's preferable fixing those cases. Allowing "anything" invalid is rather error prone.

Comment 41

[Jorg K (CEST = GMT+2)]

Agreed, all the fiddles with URLs in MailNews aren't nice at all. However, that is the observed behaviour and instead of redoing URL handling and causing 101 regressions, why don't we just copy the IMAP code and make the existing (unfortunate) cases work as suggested?

We don't allow "anything". All we do is to strip the query reliably in all cases, even the ugly. Looking for a ? from the right was never a good idea since the double-? URLs, as invalid as they are, always existed. I guess before TB 52, the "do-not-send" was also whacked on, the post-processing in MsgComposeCommands.js didn't exist, so the user saw the image, but it wasn't shipped out and no one noticed. I haven't tried to confirm.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-fix-missing-image.patch

OK, we can maximise the churn by putting the code which is in the system four times to far into a common function to avoid a fifth copy. Also, if it's used so frequently, it must be good ;-)

Comment 43

[:aceman]

Comment on attachment 9086484 [details] [diff] [review]
1572864-fix-missing-image.patch

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

I like this. Whether the URL is valid or not, at least we use the same code to parse it now.
Once somebody fixes the URL it will be easy to remove the helper.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +1966,5 @@
>  }
> +
> +// Helper function to remove query part from URL spec or path.
> +void MsgRemoveQueryPart(nsCString &aSpec) {
> +  // Sadly the query part can different forms, these were seen "in the wild",

Some missing word after 'can'.

@@ +1967,5 @@
> +
> +// Helper function to remove query part from URL spec or path.
> +void MsgRemoveQueryPart(nsCString &aSpec) {
> +  // Sadly the query part can different forms, these were seen "in the wild",
> +  // even with two ''?':

Surplus ' ?

::: mailnews/base/util/nsMsgUtils.h
@@ +395,5 @@
>   */
>  nsCString MsgExtractQueryPart(const nsACString &spec,
>                                const char *queryToExtract);
> +/**
> + * Helper function to remove query part from URL spec or path.

Does this specific URL have some name inside mailnews? It seems to be an URL of an attachment.
We could name that here.

@@ +397,5 @@
>                                const char *queryToExtract);
> +/**
> + * Helper function to remove query part from URL spec or path.
> + */
> +void MsgRemoveQueryPart(nsCString &aSpec);

Is this clear that the result will be in the argument, it is incoming and outgoing too? Is that common pattern or do we need to say it in the description?

Comment 44

[Jorg K (CEST = GMT+2)]

Thanks for the review. I fixed the comment. As for your questions:

As you know, message have parts, so the URLs shown in the comment block are all URLs addressing parts. However, also URLs addressing messages have a query part, for mailbox: URLs it's ?number=NN. All the calls from the "normalise" functions calling our new helper also want to remove that part.

Yes, nsCString &aSpec is in/out, otherwise there would be a const. BTW, it's not nsACString &aSpec since the find functions don't work on those and I wanted to avoid having to add PromiseFlatCString.

Comment 45

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4006ae274bcc86d0d4130ca0e489197490007fad
Use common function to remove query part of URL in nsMsgCompose::IsEmbeddedObjectSafe(). r=aceman

Comment 46

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086484 [details] [diff] [review]
1572864-fix-missing-image.patch

We could fix it in TB 60 as well.

Comment 47

[Jorg K (CEST = GMT+2)]

Hmm, seems to have broken mozmill/composition/test-image-display.js :-(

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-follow-up.patch

Turns out that the originalPath is this for mailbox:
=== org |/C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/0l426a83.Testing/Mail/Local%20Folders/Testordner_3?number=8|

Before, with the PL_strncasecmp we only compared up to the ? we found. And we tested on imap URLs, which don't have the ?number=.

Anyway, just needs another cut.

Comment 49

[Magnus Melin [:mkmelin]]

For it to be safe to embed you do need to make sure not only the folder is correct, but the actual message. Does that check happen elsewhere?

Comment 50

[Jorg K (CEST = GMT+2)]

Strangely enough, the original code didn't check that since it compared up to the ?. You could argue that we should extract a "number=NN" and add it to the comparison, an extraction that we do here:
https://searchfox.org/comm-central/search?q=extract.*number%3D&case=false&regexp=true&path=
Material for another bug if you wish.

Yes, the check happens elsewhere since you implemented it:
https://searchfox.org/comm-central/rev/d55fb7f2405fd4e07cb267851340626a63a8c270/mail/components/compose/content/MsgComposeCommands.js#6724

Dumping out src and originalMsgNeckoURI.value.spec we see:
=== mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/0l426a83.Testing/Mail/Local%20Folders/Testordner_3?number=8&header=quotebody&part=1.2.2&filename=FR%20%E2%80%94%20Se%CC%80te%20%E2%80%A2%2011.3.16%20#0234%20100cm%20copy.jpg
=== mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/0l426a83.Testing/Mail/Local%20Folders/Testordner_3?number=8

or

=== imap://[e-mail]:143/fetch%3EUID%3E.INBOX.march%202019%3E94?header=quotebody&part=1.2.2&filename=FR%20%E2%80%94%20Se%CC%80te%20%E2%80%A2%2011.3.16%20#0234%20100cm%20copy.jpg
=== imap://[e-mail]:143/fetch%3EUID%3E.INBOX.march%202019%3E94

or

=== imap://[e-mail]:143/fetch%3EUID%3E.INBOX.march%202019%3E94?header=quotebody/;section=2.2?part=1.2.2&filename=FR%20%E2%80%94%20Se%CC%80te%20%E2%80%A2%2011.3.16%20#0234%20100cm%20copy.jpg
=== imap://[e-mail]:143/fetch%3EUID%3E.INBOX.march%202019%3E94

So for mailbox, we compare the "number=8" and for imap the "94". We're lucky this is well-behaved, so the mailbox: URL has "number=8" first in the query and not some other junk, like we see here:
https://hg.mozilla.org/comm-central/rev/4006ae274bcc86d0d4130ca0e489197490007fad#l1.15
I guess, that all happens for IMAP, mailbox URLs are better behaved.

Comment 51

[Jorg K (CEST = GMT+2)]

As we've seen, being too restrictive at
https://searchfox.org/comm-central/rev/d55fb7f2405fd4e07cb267851340626a63a8c270/mailnews/compose/src/nsMsgCompose.cpp#267
leads to adding "moz-do-not-send", ignoring the image and shipping out the MailNews URL. But you can argue that for IMAP you already check the number, so OK, I'll add it for mailbox:.

Comment 52

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-follow-up2.patch

As requested by Magnus, let's do the comparison with the number added. That changes the original behaviour, but it aligns it with IMAP where the message number was never a query part.

Comment 53

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/1c5dad422769d908b8552bb44533bd2538269200
Follow-up: Transfer message number before comparing. rs=bustage-fix

Comment 54

[Jorg K (CEST = GMT+2)]

I checked NNTP as well. Here we see:
=== |/mailman.259.1566306089.1596.test-multimedia%40lists.mozilla.org?group=mozilla.test.multimedia&key=3478|
=== |/mailman.259.1566306089.1596.test-multimedia%40lists.mozilla.org?group=mozilla.test.multimedia&key=3478&header=quotebody&part=1.2.2&filename=hlgkhobceicekcee.png|

So the old code only compared to the ? and didn't add "moz-do-not-send", the new code compares the first URL with the second with the entire query stripped, and since they are unequal, it adds "moz-do-not-send". You don't see the missing image in the compose windows, since there are different content policy rules for NNTP, you can basically load anything, but the message is shipped with a broken image.

So I guess we need to what the JS code does: A "starts with". In effect, we could backout both patches, but we might as well leave the clean-up from the first one.

Comment 55

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-follow-up3.patch

So this applies after a backout of follow-up 2.

Bug sigh: NNTP still doesn't work. OK, we no longer put on "moz-do-not-send", but since no error is triggered in MsgComposeCommands.js, the image is also not replaced with a data URL.

Strangely with this patch here sending crashes, I'll have to see why.

Comment 56

[Jorg K (CEST = GMT+2)]

OK, if you back out the two landed parts, go to m.t.multimedia on news.mozilla.org, find the a message with an embedded image in the thread "test attachment", produce a HTML reply and send it, you'll crash.

If you go with the two landed patches, it doesn't crash, since the "faulty" code makes the image "moz-do-not-send" for NNTP.

If you back out the second patch and used the third, you'll crash again.

The crash is at nsMsgSend.cpp#2793 where we call HackAttachments(attachments, preloaded_attachments); with 2x nullprt.

I've looked at that crash before in bug 1571813.

Comment 57

[Jorg K (CEST = GMT+2)]

Sigh, the follow-up 3 doesn't work for images in messages opened from a file. Motivated by bug 1412639 which added this:
https://searchfox.org/comm-central/diff/132e925a996bc1f52c80e1d840350ae28b22624b/mail/components/compose/content/MsgComposeCommands.js#5493

There we have:

=== |/D:/Desktop/Rocket.eml?fetchCompleteMessage=true&number=0&realtype=message/rfc822|
=== |/D:/Desktop/Rocket.eml?fetchCompleteMessage=true&number=0&part=1.2.2&filename=FR%20%E2%80%94%20Se%CC%80te%20%E2%80%A2%2011.3.16%20#0234%20100cm%20copy.jpg|

So the image path doesn't start with the original path since that has the funny &realtype=message/rfc822 in it.

So let's summarise the three follow-up options explored so far:

1. Magnus didn't like. Always compare without the queries. That's the original code that only compared to the ?
2. Currently landed: Transfer number=2 across before comparing. Won't work for NNTP since we'd need to transfer group and key across. But that might be too restrictive since for news we should be able to access anything.
3. Used the "begins with" code which is used in JS. Doesn't work for forwarded saved messages.
4. There's also the concept of a "normalised" URL: https://searchfox.org/comm-central/search?q=GetNormalizedSpec&case=false&regexp=false&path=mailnews. But that's not implemented for NNTP :-(

Looks like a no-win situation unless we go back to option 1.

Opinions?

Comment 58

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086656 [details] [diff] [review]
1572864-follow-up3.patch

I thought about this afk. The best solution that will fix everything is what is landed plus a refinement. Coming up.

Comment 59

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086586 [details] [diff] [review]
1572864-follow-up2.patch

So this was landed and we should keep a modified version of it. Coming up.

Comment 60

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-follow-up2b.patch

OK, for clarity I decided to back out the already landed follow-up and use this enhancement of it.

Comment 61

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086714 [details] [diff] [review]
1572864-follow-up2b.patch

Well, you can't win this. Looking here:
https://searchfox.org/comm-central/search?q=reference.*random.*message&case=false&regexp=true&path=mail%2F
I get the impression that references to random message parts should not be blocked by the backend, but notified by the frontend.

So that would imply just using option 1. In reality, it doesn't matter since it won't happen in "normal" production. It can only go wrong, if - say - a draft references an attachment in a previous draft, we've seen those bugs, but they are all gone now.

Comment 62

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086574 [details] [diff] [review]
1572864-follow-up.patch

OK, back to option 1.

Comment 63

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-follow-up.patch (1b)

OK, changed the variable names around.

Comment 64

[Jorg K (CEST = GMT+2)]

OK, back to option 1.

That's also better for the "Quote Message" function. There we insert some other message with its references, so unless we do option 1, we won't get prompted. EDIT: Scrap that. Of course we insert the same message.

Comment 65

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/54f7c7ba6885a28271328355e8d1063f879d64e2
Backed out changeset 1c5dad422769 to make way for a different solution. a=backout
https://hg.mozilla.org/comm-central/rev/f5221f1bc7487c5b166c71ee2bab18bd43b1f574
Follow-up: Remove query from base URL as well. rs=bustage-fix

Are we're done here?

Comment 66

[Jorg K (CEST = GMT+2)]

Attachment: 1572864-comment.patch

OK, Aceman requested that the insights we gained today be condensed into a comment. Here goes.

Comment 67

[gene smith]

So with your latest patch, imap original UID has to match UID in the path var since original has no '?'. But for mailbox, the "number" is chopped off and not compared. But maybe comparison of the message UID or number is not really important for what the function does. (Comment 66 came in while typing this so I think it expresses the same observation.)

Comment 68

[Jorg K (CEST = GMT+2)]

Right, Gene. IMAP compares the number, mailbox doesn't and never did in the past since it always compared to the ?

I had another solution worked out in attachment 9086714 [details] [diff] [review], but decided against it in the end since we have frontend code to deal with cross-message references. We need to fix this bug here and in TB 68 ESR, so I don't want to backport a change that changes the behaviour.

As I said before, we can certainly revisit the area in another bug.

Comment 69

[:aceman]

Comment on attachment 9086755 [details] [diff] [review]
1572864-comment.patch

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

Thanks, let's not lose the knowledge you found.

Comment 70

[gene smith]

Sounds good...
The new comment mentions "JS Account". Don't know what that is but maybe its a thing I've just not seen.

Comment 71

[Jorg K (CEST = GMT+2)]

Filed bug 1575330. JS Account is a component that lets you define new "account types". It's used by the add-ons ExQuilla and Owl to implement support for the MS Exchange protocol. https://searchfox.org/comm-central/source/mailnews/jsaccount, there's a readme.

Comment 72

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/8cf257622392290756fdcfd36c62ba233b5f8558
Follow-up: Add comment. r=aceman

Finally done?

Comment 73

[Jorg K (CEST = GMT+2)]

TB 69 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/343f09115e69ba55ea5fded11339f8c79dbbdfdd (merged three patches).

Comment 74

[Jorg K (CEST = GMT+2)]

TB 68.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/0e1c15a440f08ff8b4d77ea7ff7fb53c2fea4cd3

Comment 75

[Jorg K (CEST = GMT+2)]

TB 60.9 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/0349f4cf9c24a9110cae02ea7fba6ad633f5ae53