Open Bug 1135434 Opened 5 years ago Updated 2 years ago

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

Categories

(MailNews Core :: Backend, defect, P2, critical)

x86
All
defect

Tracking

(Not tracked)

People

(Reporter: otsy, Unassigned)

References

Details

(Keywords: dataloss, Whiteboard: [patchlove])

Attachments

(3 files)

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.
Severity: normal → critical
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).
Group: core-security
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.
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.
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
Group: core-security
Component: Message Reader UI → Backend
Keywords: dataloss
Product: Thunderbird → MailNews Core
Summary: On IMAP, deleting attachment adds following messages to original message but hidden → On IMAP, deleting attachment corrupts message by adding following messages
Whiteboard: [dupeme]
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?
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.
Resulting email message after forwarding corrupted message as attachment.
(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.
(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.
(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.
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?
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.
Assignee: nobody → rkent
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8569745 - Flags: review?(Pidgeot18)
(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.
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?
Blocks: 340886
Flags: needinfo?(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.
Flags: needinfo?(rkent)
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.
Attachment #8569745 - Flags: review?(Pidgeot18) → review-
(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
Flags: needinfo?(rkent)
Flags: in-testsuite?
Flags: in-moztrap?
OS: Mac OS X → All
Flags: needinfo?(rkent)
Priority: -- → P2
Aceman, can you finish off the patch if Kent is unable to do so?
Flags: needinfo?(acelists)
Whiteboard: [dupeme] → [patchlove]
Summary: On IMAP, deleting attachment corrupts message by adding following messages → On IMAP, deleting attachment corrupts message by adding following messages, when using "mark as deleted" delete model
Jorg, has recent attachment work likely impacted this?
Assignee: rkent → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(acelists) → needinfo?(jorgk)
No, that was purely in MIME. This here is IMAP.
Flags: needinfo?(jorgk)
You need to log in before you can comment on or make changes to this bug.