1135434 - On IMAP, deleting attachment corrupts message by adding following messages, when using "mark as deleted" delete model

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[Jack Otto]

Attachment: On IMAP, deleting attachment and moved.eml

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

While viewing a message in an IMAP folder (with "mark it as deleted" selected, in account settings), delete an attachment, using bottom-right pull-down item.
Move the original message to another (local) folder, or even just "View - Message Source".


Actual results:

The original message now contains the following messages from the original folder, as seen with "View - Message Source". After moving the original message to another folder, its message size now reflects the inclusion of those (many) following messages, with those messages now marked ">From" at their start,. But still, those extra messages don't appear when viewed normally. The IMAP system retains the correct original message; so a "repair folder" reloads the original message intact. 


Expected results:

The original message should have remained unchanged locally.

Comment 1

[Jack Otto]

Moved up to critical. Maybe even higher, as any message now included in the original message remains, though hidden. If the original message is moved, un-deleted, forwarded, etc. it remains a risk to privacy, now containing (hidden) message(s).

Comment 2

[Jack Otto]

Changed to restricted, as just realised now anyone could hack into (hidden) messages and reveal them. As I did to clean up the resulting, sometimes very large, original messages, which I had moved to a "large messages" holding area, just keeping the smaller (attachment deleted) message; by removing the angle bracket in front of the ">From", and then deleting the extra messages. A fix to clean up potentially harmful private (though hidden) messages may be in order as well. And then from backups, etc. Hopefully not too many people have deleted attachments form an IMAP folder, and then kept the originals, having "mark it as deleted" selected, in account settings.

Comment 3

[Jack Otto]

Just a note on fixing messages with hidden following messages, tagged with ">From". It's sufficient just to tackle just the first of the following hidden messages. Once detached from the original message, it now contains all of any following hidden messages, if any; so deleting it, removes all of the following hidden  messages.

Comment 4

[Wayne Mery (:wsmwk)]

Such a hacker would need access to your machine - which if it happened, you have bigger issues than Thunderbird message corruption :)   What you are describing message corruption

Comment 5

[Kent James (:rkent)]

If the message contains hidden material that is also included in a message forwarded to a third party, that would leak information in a bad way.

Jack, can you confirm that this extraneous material is included in a forward message?

Is this repeatable? And I assume that your imap is in the default configuration of storing messages offline, right?

Comment 6

[Jack Otto]

Both possibilities result when forwarding.

As inline: only the (first) message is forwarded; the same as what is displayed when a message is viewed.

As attachment: the hidden messages are also forwarded intact in the attachment; the same as what is moved or seen in "View - Message Source".

I forwarded the message after it was moved from IMAP to a local folder (so the angle bracket had been placed before the leading "From" lines).

I'll upload the resulting message.

Comment 7

[Jack Otto]

Attachment: Fwd as Attachment. On IMAP, deleting attachment

Resulting email message after forwarding corrupted message as attachment.

Comment 8

[Jack Otto]

(In reply to Kent James (:rkent) from comment #5)
> If the message contains hidden material that is also included in a message
> forwarded to a third party, that would leak information in a bad way.

Yes, when forwarded as an attachment.

> Jack, can you confirm that this extraneous material is included in a forward
> message?

Yes, see earlier message.

> Is this repeatable?

Yes, the bug is repeatable and consistently so. Tried it on IMAP on both TPG.com.au and GMail.com. But only from Mac OSX.

Comment 9

[Jack Otto]

(In reply to Kent James (:rkent) from comment #5)
> And I assume that your imap is in the default
> configuration of storing messages offline, right?

Not quite sure what you mean here. I have two TPG and one GMail IMAP accounts set up. Basically straight-forward; SSL/TLS port 993; "just mark it as deleted" is set.

I keep mail on IMAP for a time, until read and/or replied to. Eventually mail is deleted or mostly moved to local folders. Some messages with attachments are moved to a (local) large storage structure, with the message stripped of its attachment archived or filed locally.

Basically, the default "Local Folders" is unchanged and used for "offline storage". Most IMAP folders have the messages stored (synchronised) locally for offline use.

I haven't tried to repeat the bug in an IMAP folder which is NOT stored (synchronised) locally.

Comment 10

[Jack Otto]

(In reply to Wayne Mery (:wsmwk) from comment #4)
> Such a hacker would need access to your machine - which if it happened, you
> have bigger issues than Thunderbird message corruption :)

I wasn't so much thinking of my case: a personal setup and quite secure. But rather others, home and office, especially on shared computers or mail, who may think that deleted messages have gone. Now, if another family member or work mate has deleted attachments on IMAP and then keep the original message, those original messages could contain sensitive information. This report now gives a way for prying eyes to look easily for hidden messages, thought deleted.

I've already cleaned mine up, though backups (usually secure) haven't been; but I rarely have sensitive or embarrassing emails. My concern was that I was finding messages which were originally 50k, now 15Meg, because of a hidden message or more, with big attachments. I was wondering why my storage folders were quickly rising to 100's of Meg in size.

The issue of forwarding such messages as attachments is a risk; which I've rarely done, luckily, as I had hundreds of hidden messages, in just a few messages. One corrupted message could contain many hidden messages (all those received later than the (eventually corrupted) message, up to when the attachment was deleted) and message kept (moved or marked "undeleted").

But I'd guess that there are VERY few people who use IMAP and also regularly delete attachments and keep the original message. If it's COPIED first, and then deleted after the attachment is deleted, there's no problem, as the corrupted message is deleted (possibly automatically, if "just mark it as deleted" is not set.

Comment 11

[Kent James (:rkent)]

I can observe this behavior on the deleted message in the original folder (which can be viewed because of the delete model) but not on the moved message. Is that the behavior that see?

Comment 12

[Kent James (:rkent)]

During the delete, we hit this code in nsImapMailFolder::CopyFileMessage:

          // Perhaps we have the message offline, but even if we do it is
          // not valid, since the only time we do a file copy for an
          // existing message is when we are changing the message.
          // So set the offline size to 0 to force SetPendingAttributes to
          // clear the offline message flag.
          msgToReplace->SetOfflineMessageSize(0);

And we do soon hit nsImapMailFolder::SetPendingAttributes as promised, which sees the zero message offset, but does not do the promised clear of the offline message flag.

Later, in nsMsgDBFolder::GetOfflineFileStream, we get the zero size:

hdr->GetOfflineMessageSize(size);

but then we adjust that zero size here:

          // Check that the first line is a header line, i.e., with a ':' in it.
          // Or, the line starts with "From " - I've seen IMAP servers return
          // a bogus "From " line without a ':'.
          if (findPos != -1 && (startOfMsg[findPos] == ':' ||
                                !(strncmp(startOfMsg, "From ", 5))))
          {
            *offset += msgOffset;
            *size -= msgOffset;
          }

Obviously subtracting from zero, and later casting that size to int64_t, is a Bad Thing.

One fix is to intelligently use the zero size in nsMsgDBFolder::GetOfflineFileStream to reject the stream, another might to do what it wants in nsImapMailFolder::SetPendingAttributes and set the message as not available offline.

Comment 13

[Kent James (:rkent)]

Attachment: Don't zero size of offline store for message

This may be overkill, but it removes the original cause (which I added years ago :( ) and then adds various checks that should also have caught the problem.

Comment 14

[Jack Otto]

(In reply to Kent James (:rkent) from comment #11)
> I can observe this behavior on the deleted message in the original folder
> (which can be viewed because of the delete model) but not on the moved
> message. Is that the behavior that see?

Clarifying: if you move the original (marked as deleted) message from IMAP after the attachment is deleted, it retains the extra messages (in my tests).

But the new message without the attachment is fine; whether in IMAP or moved.

(All works fine if working from a Local Folder.)

Or did you mean something else?

You'll notice that the new message is also included in the extra messages contained in the corrupted message. I don't know if further messages received are also included before moving it local.

Jack.

Comment 15

[Wayne Mery (:wsmwk)]

Interesting. Is bug 340886 the only change since 2009 that helps this happen?

Seems worthy of adding a test. Or do we already have a test that is not 100% solid?

Comment 16

[Kent James (:rkent)]

I doubt if we have any tests that examine deleted messages. A quick look also did not find any tests targeting the IMAP delete model, which is what this bug is about. There are many comments in the code though about how the IMAP delete model is a special case, so tests would be good. But the issue is bigger than just this bug, and I am not enthusiastic about writing that test today.

Comment 17

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8569745 [details] [diff] [review]
Don't zero size of offline store for message

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

If I apply this patch, I get a failure in mailnews/local/test/unit/test_streamHeaders.js (some method is throwing NS_ERROR_FAILURE)

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +836,5 @@
>            // Or, the line starts with "From " - I've seen IMAP servers return
>            // a bogus "From " line without a ':'.
>            if (findPos != -1 && (startOfMsg[findPos] == ':' ||
> +                                !(strncmp(startOfMsg, "From ", 5))) &&
> +                               *size > msgOffset)

Judging from comments you've made, you might want an assertion that size is sane somewhere in this method.

Comment 18

[Wayne Mery (:wsmwk)]

(In reply to Kent James (:rkent) from comment #16)
> I doubt if we have any tests that examine deleted messages. A quick look
> also did not find any tests targeting the IMAP delete model, which is what
> this bug is about. There are many comments in the code though about how the
> IMAP delete model is a special case, so tests would be good. But the issue
> is bigger than just this bug, and I am not enthusiastic about writing that
> test today.

marking in-moztrap?

(In reply to Joshua Cranmer [:jcranmer] from comment #17)
> Comment on attachment 8569745 [details] [diff] [review]
> Don't zero size of offline store for message
> 
> Review of attachment 8569745 [details] [diff] [review]:
> ...
> Judging from comments you've made, you might want an assertion that size is
> sane somewhere in this method.

sounds like you are close

Comment 19

[Wayne Mery (:wsmwk)]

Aceman, can you finish off the patch if Kent is unable to do so?

Comment 20

[Wayne Mery (:wsmwk)]

Jorg, has recent attachment work likely impacted this?

Comment 21

[Jorg K (CEST = GMT+2)]

No, that was purely in MIME. This here is IMAP.

Comment 22

[gene smith]

I can duplicate something similar with an mbox backed folder. It wasn't clear to me how to duplicate as described so here's what I did:

• Choose a fairly small folder and make sure it has mbox offline store and sort by "order received".
• Repair or Compact the folder to make sure it is initially OK.
• Copy into that folder a message with at least 2 attachments (msg A)
• Copy into that folder a fairly short message (msg B)
• Configure delete mode to "just mark as deleted" and restart tb for good measure
• Open msg A and delete (not detach) an attachment
• Msg A gets crossed out and modified message Msg A- is added to the message list at highest "order received" number
• Look at Msg A- and its source. It should be OK with the attachment removed.
• Look at mbox file for the folder with Msg A- last in the file. All looks OK at this point:
  -- any leading messages in folder
  -- Msg A
  -- Msg B
  -- Msg A-
• Now undelete Msg A so the cross-out is removed (undelete may not be necessary, I always did undelete)
• In Msg A delete a different attachment
• Again Msg A gets crossed out and modified Msg A-- is added to the message list at highest "order received"
• Look at mbox file again and you will now see this:
  -- any leading messages in folder
  -- Msg A
  -- Msg B
  -- Msg A-
  -- Msg A--
  -- Msg B <-- wrong
  -- Msg A- <-- wrong

Now if you forward Msg A-- as an attachment, the message will look correct at the destination but the message source will also contain Msg B and Msg A- content appended at the bottom.
Anyhow, this is what I see now when I tried this. It seems a bit different than described in comment 0 in that I have to do the attachment delete at least twice to trigger the bug.
Yes, if you delete even more attachments at Msg A you end up with even more baggage appended.
I haven't tried this with maildir so don't know if it has the problem too.

Comment 23

[gene smith]

I was not able to consistently duplicate the problem with the step2 shown in comment 22. However, the comment 0 STR actually demonstrates the problem, restated like so:

• Choose a folder and make sure it has mbox offline store and sort by "order received".
• Repair and Compact the folder to make sure it is initially OK.
• Copy into that folder a message with attachments (msg A)
• Copy into that folder a couple fairly short messages (msg B1 msg B2)
• Look at folder's mbox file and make sure the order in the file is msgs A, B1, B2
• Configure delete mode to "just mark as deleted"
• Open msg A and delete an attachment
• Msg A gets crossed out and modified message Msg A- is added to the message list at highest "order received" number after B2
• Look at message source for Msg A, it will have msg B1 and B2 tacked on at the bottom.
• If you copy or move Msg A to another folder, the extra baggage (B2 and B2) will come along when you look at the destination source.
• Msg A always displays correctly even with the extra baggage.

The patch provided in comment 13 by Kent James does fix the problem. As he describes it, the most important part is to remove these lines:
https://searchfox.org/comm-central/rev/f8c1a9e5db5ea9efeb0ef6c1a996c25d039ae3d3/mailnews/imap/src/nsImapMailFolder.cpp#7487-7489
Setting the offline size to 0 when the messages is still present in offline store, as it would be with "just mark as deleted", causes usually the mbox file to be read from the start of Msg A to the end of file when only Msg A is wanted, so you get the extra "baggage".

As Kent James describes it, the other changes in the patch are error checking (some "overkill") that would have maybe caused the problem to be noticed without producing a possible corrupted message containing a lot of unintended content.

I've tested the changes with "delete to trash" and not setting the offline size to 0 cause no problems.
Also, my tests indicate this doesn't affect maildir, just mbox (although maildir seems to have other unrelated problems when "just mark deleted" is used).
The bug doesn't happen and the fix doesn't do anything bad when offline store is not used for the folder.

From comment 17:

If I apply this patch, I get a failure in mailnews/local/test/unit/test_streamHeaders.js (some method is throwing NS_ERROR_FAILURE)

I'm not seeing this failure.

Comment 24

[gene smith]

Attachment: Bug 1135434 - With imap delete model, deleting attachment corrupts message by adding following messages. r=mkmelin

This patch is the work of Kent James from 9 year ago. I rebased it and tested it extensively.

Comment 25

[gene smith]

Wanted to run a try build on this to make sure it produces no test failure. However, I've been unsuccessful in getting a build to work, e.g., https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6291ebdad6708bb0149a66c7e4a88d3072da45a4&selectedTaskRun=DEMx2OaDRpiVmz1nTmKjxw.0
So will go ahead and request checkin.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e55600ee3b94
With imap delete model, deleting attachment corrupts message by adding following messages. r=mkmelin

Comment 27

[gene smith]

This fails when I try to "detach" an attachment. The attachment gets saved but it remains present in the message. Not sure it is working exactly right when deleting attachments either. Sometimes the attachment deleted completely disappears from mbox rather than leaving the "altered" and placeholder mime part with the message.
It also fails mailnews/imap/test/unit/test_imapAttachmentSaves.js when "AttachmentDetached" never gets written to mbox at line 149.
Re: comment 25 -- I should have manually run at least the imap unit tests.

So this needs to be backed out.

Comment 28

[gene smith]

Attachment: t.diff

This fixes the problems described in the previous comment. I'll do some more checking tomorrow before submitted the changes formally.

Comment 29

[Magnus Melin [:mkmelin]]

Ok, we'll back it out.

Comment 30

[Martin Giger [:freaktechnik]]

Backed out: https://hg.mozilla.org/comm-central/rev/f2c8dafde6572cdcd57904d7026c26a65f42103b

Comment 31

[gene smith]

Attachment: Bug 1135434 - With imap delete model, deleting attachment corrupts message by adding following messages. r=benc,mkmelin

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b1f3d40c9cef
With imap delete model, deleting attachment corrupts message by adding following messages. r=BenC

Comment 33

[Wayne Mery (:wsmwk)]

Lets land it and create a follow-up bug to think about how to tidy up the way attachments are removed from messages.

Is the follow-up bug filed?

Comment 34

[gene smith]

(In reply to Wayne Mery (:wsmwk) from comment #33)

Lets land it and create a follow-up bug to think about how to tidy up the way attachments are removed from messages.

Is the follow-up bug filed?

That was Ben's recommendation here: https://phabricator.services.mozilla.com/D185617
I'm don't know what he has in mind for this new bug and he doesn't appear to be on the CC list for this bug at the BMO level so I'll CC/NI him.

Comment 35

[Wayne Mery (:wsmwk)]

Comment on attachment 9347780 [details]
Bug 1135434 - With imap delete model, deleting attachment corrupts message by adding following messages. r=benc,mkmelin

[Triage Comment]
Approved for esr115

Comment 36

[Daniel Darnell [:dandarnell]]

Thunderbird 115.2.3:
https://hg.mozilla.org/releases/comm-esr115/rev/3a1f9fba9487

Comment 37

[Ben Campbell]

(In reply to gene smith from comment #34)

That was Ben's recommendation here: https://phabricator.services.mozilla.com/D185617
I'm don't know what he has in mind for this new bug and he doesn't appear to be on the CC list for this bug at the BMO level so I'll CC/NI him.

I don't know I had anything specific in mind (other a general feeling of horror at the sheer obfuscated complexity in there).
So I've rolled it into Bug 1788159, which deals with general tidying-up of all the attachment-saving and detaching code.