196749 - Incoming mail contains "X-Mozilla-Status" and cause Mozilla to handle it as read, flagged, prioritised

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Matthias Bockelkamp]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030307
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030307

If I send myself an email (using the Indy-Library) containing the extra-headers
"x-mozilla-status" and "x-mozilla-status2" the values set by me are used for
displaying the new mail. In addition Mozilla generates new "x-mozilla-status"
and "x-mozilla-status2" entries which contain the value "0". This leads to the
following questions: 

1. Is it intentional to use those values, if they come in with the mail?

2. Why does Mozilla generate additional entries, which could cause confusion if
the order of processing header fields is changed in future?

3. Could this be a way for spammers to bypass the junk mail controls? Then the
priority should be increased!


Reproducible: Always

Steps to Reproduce:
1. Send yourself a mail with x-mozilla-status and x-mozilla-status2 header fields
2. Get the mail
3. View the message source

Actual Results:  
Looks like this:

X-Mozilla-Status: 0000
X-Mozilla-Status2: 00000000
..
X-Mozilla-Status: 0003
X-Mozilla-Status2: 06000000


Expected Results:  
There should be no double entries.

Comment 1

[Eric]

Over the last two days I've started to see spam coming in with the X-mozilla*
fields set, to avoid the spamfilters.  Most have been marked Read and Not Spam.
 Spam filters don't get these messages, unless I "Run Junk Mail Controls On Folders"

Perhaps a spammer has read this bug.  In any case, I'd think that this should be
bumped up on the priority list.  

I'm using Thunderbird 0.6 on Windows.

Comment 2

[Matthias Bockelkamp]

Confirming, reassigning to mscott's new address, changing Hardware and OS to
All, changing Severity to Major, setting "blocking1.7?".

Comment 3

[Scott MacGregor]

way too late for the 1.7 train.

Comment 4

[Matthias Bockelkamp]

(In reply to comment #3)
> way too late for the 1.7 train.

Won't be nice for the 1.7 users. Perhaps make this a Security Bug to at least
prevent Spammers from reading it?

Comment 5

[Matthias Bockelkamp]

Oh, sorry for changing your flags, but I'm sure I didn't touch them.

Comment 6

[Brodie]

No point in security on the bug. The information is on a public news server and
appears to be already in use in the wild.

Comment 7

[Howard Chu]

Attachment: Proposed fix

This patch silently removes any "X-Mozilla*" headers from incoming POP3 email.
It also cleans up some other places that were using the bare literals instead
of the X_MOZILLA macros, and fixes a crash I ran into while sending myself
various test messages with X-Mozilla headers set.

Comment 8

[Howard Chu]

Attachment: Fix strncasecmp in previous patch

Same as before, but uses PL_strncasecmp instead of strncasecmp

Comment 9

[Howard Chu]

(In reply to comment #8)
> Created an attachment (id=151256)
> Fix strncasecmp in previous patch
> 
> Same as before, but uses PL_strncasecmp instead of strncasecmp

But this would probably have done the trick all by itself:

Index: nsParseMailbox.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/local/src/nsParseMailbox.cpp,v
retrieving revision 1.235
diff -u -r1.235 nsParseMailbox.cpp
--- nsParseMailbox.cpp  18 Jun 2004 18:37:36 -0000      1.235
+++ nsParseMailbox.cpp  20 Jun 2004 01:27:35 -0000
@@ -940,7 +940,12 @@

     value = buf;
     if (header)
+    {
+      // Ignore repeated headers.
+      if (header->value)
+        return 0;
       header->value = value;
+    }

 SEARCH_NEWLINE:
     while (*buf != 0 && *buf != nsCRT::CR && *buf != nsCRT::LF)

Comment 10

[Brodie]

Those headers are also added to IMAP mail right? Any solution needs to address 
all sources of email, not just POP. I don't think that there is any point to 
cleaning duplicate headers up, just go with your solution in comment 9 and 
ignore headers that have already been parsed. It's cleaner and simpler with 
less change required. 

Comment 11

[Howard Chu]

(In reply to comment #10)
> Those headers are also added to IMAP mail right? Any solution needs to address 
> all sources of email, not just POP. I don't think that there is any point to 
> cleaning duplicate headers up, just go with your solution in comment 9 and 
> ignore headers that have already been parsed. It's cleaner and simpler with 
> less change required. 

Yes, you're right. The solution in comment #9 solves the problem for both POP
and IMAP, and it's the simplest.

From a "correctness/maintainability" standpoint I think some of my other changes
should be made anyway, but that should be a separate issue so I'll drop it from
the current discussion.

Comment 12

[Christian Eyrich]

The code from comment #9 only prevents parsing all (thus using the last)
occurences of a header line.
But Mozilla still adds a header, e.g. X-Mozilla-Status, a second time instead of
overwriting the one that came in - that's at least confusing.

I'm very much in favour of removing all internal used header lines upon receive.

Comment 13

[Howard Chu]

(In reply to comment #12)
> The code from comment #9 only prevents parsing all (thus using the last)
> occurences of a header line.
> But Mozilla still adds a header, e.g. X-Mozilla-Status, a second time instead of
> overwriting the one that came in - that's at least confusing.

Actually the code in Comment #9 makes it so that only the *first* occurrence is
parsed, and the first occurrence is always the header that Mozilla added,
because Mozilla writes the X-Mozilla-Status immediately after creating the
"From" envelope header. So this patch by itself is enough to prevent spammer
tricks from working.

> I'm very much in favour of removing all internal used header lines upon receive.

I agree, it's confusing to have the bogus headers remain. I've spent the past
couple weeks reading thru all the POP3 code but I'm not familiar enough with the
IMAP code to figure out how to do the equivalent fix. I.e., you have to squash
any illegal headers in the incoming protocol stream, without disturbing headers
that Mozilla itself generates.

Comment 14

[Brodie]

Adding bienvenu for IMAP concerns.

I see now that the actual change for removing the duplicate lines is quite 
simple. I agree that it is the best solution. It was all of the other changes 
in the patch which threw me.

Why have you added the length and changed from signed to unsigned? Was there a 
problem that you are fixing? If not then these changes aren't really needed. 
This code isn't going to be a performance problem - the network delays with the 
pop protocol will far outweigh any savings in getting rid of a single strlen.

> nsCAutoString uidlCString("X-UIDL: ");
X_UIDL?

> char *statusLine = PR_smprintf(X_MOZILLA_STATUS_FORMAT MSG_LINEBREAK, flags);
If you are going to clean up this code a bit, why don't you remove the malloc 
at this line and use PR_snprintf into a stack var instead. 

> // Check for illegal headers in incoming mail
Since you are using the defines for all of the other X_MOZILLA stuff, why don't 
you create new defines for X_MOZILLA_PREFIX and X_MOZILLA_PREFIX_LEN and use 
them. Ditto for FCC.

Could we ever get a dummy "From - " envelope header line in the email? Do you 
need to check for that?

Comment 15

[David :Bienvenu]

I don't think you can eliminate all duplicate headers since I think it's legal
to duplicate some headers, in particular, to: and cc:.

Personally, I believe the simplest, safest, fix is to just ignore the
x-mozilla-status duplicate headers when parsing the mail headers. I don't think
we need to strip them out. If we just ignore them when parsing, we fix the
problem for pop3, imap, and for reparsing mail folders. I'll attach a simple
patch showing what I mean...

Comment 16

[David :Bienvenu]

Attachment: proposed fix

untested, but this is what I have in mind.

Technically, we're never supposed to remove stuff from a message - but it's
perfectly alright to ignore stuff.

I don't see any reason to strip out X-UIDL headers, since we only ever pay
attention to the first one, which will be the one we put in...

Comment 17

[David :Bienvenu]

*** Bug 246638 has been marked as a duplicate of this bug. ***

Comment 18

[Mike Cowperthwaite]

Perhaps related, another duplicate-header (Subject:) issue is bug 166068.

Comment 19

[Stefan Borggraefe]

(In reply to comment #18)
> Perhaps related, another duplicate-header (Subject:) issue is bug 166068.

Hm, wrong bug number?

Comment 20

[Mike Cowperthwaite]

Oops, yes, copied from the wrong page.  I meant bug 172104.

Comment 21

[Howard Chu]

(In reply to comment #16)
> Created an attachment (id=151292)
> proposed fix
> 
> untested, but this is what I have in mind.

Am I blind, or is there really No code that sets m_IgnoreXMozillaStatus? (I did
a find in the mailnews source tree, only found tests, not sets.)

> Technically, we're never supposed to remove stuff from a message - but it's
> perfectly alright to ignore stuff.

Right. And the existing code was effectively "ignoring" things anyway, it's just
that it was ignoring the first instances of a header, not the last.

Comment 22

[Howard Chu]

(In reply to comment #14)

> Why have you added the length and changed from signed to unsigned? Was there a 
> problem that you are fixing? If not then these changes aren't really needed. 
> This code isn't going to be a performance problem - the network delays with the 
> pop protocol will far outweigh any savings in getting rid of a single strlen.

This is an interesting question, but not for this particular bug. If there's a
better forum to discuss this, let's take it there. But to answer the point:
whether there are significant network delays or not is no excuse to waste CPU
time; we're not running in a single-tasking environment. There are other things
out there that may have a use for the CPU time even if this particular program
doesn't. When's the last time anybody profiled this codebase? (It's not like
there aren't already enough things to worry about, of course...) In general
though, why write code that executes in linear time (strlen) when you can do it
in constant time (sizeof)? Why recalculate information that was already
available, but got thrown away? Speaking from personal experience, profiling and
paying attention to details like this has allowed us to speed up OpenLDAP by
over a factor of 200 (yes, that's right, 200) from the 1999 code base to present
day. Every algorithmic choice you make matters.

Comment 23

[David :Bienvenu]

m_IgnoreXMozillaStatus is currently unused - in 4.x, it was used in a couple
situations, parsing outgoing messages, and parsing imap headers. We should be
setting it when parsing imap headers, and we might someday need to parse
outgoing messages.

Fix checked in.

Comment 24

[David :Bienvenu]

Comment on attachment 151292 [details] [diff] [review]
proposed fix

nice to have for 1.7.1

Comment 25

[Mike Kaply [:mkaply]]

Comment on attachment 151292 [details] [diff] [review]
proposed fix

a=mkaply for 1.7.1.

Comment 26

[Greg Miller]

*** Bug 249501 has been marked as a duplicate of this bug. ***

Comment 27

[Martin Sch]

Isn't it possible to move those mozilla flags into the .msf file? It looks like
mozilla/thunderbird stores that information in the .msf file for imap mail.

But if mozilla flags are stored together with the mail, there should be 2
separate parts: control information and the original mail itself. Hence the mail
box file looks like:

 From - ....
 X-Mozilla-Status: ...
 X-Mozilla-Status2: ...
 X-Mozilla-Border: ---------------------------
 Received: ...
 Date: ...
 From: ...

If mozilla/thunderbird checks for flags, this should not be done below the
border, i.e. in the original mail.

Comment 28

[Peter Kirk]

(In reply to comment #27)
> Isn't it possible to move those mozilla flags into the .msf file? It looks like
> mozilla/thunderbird stores that information in the .msf file for imap mail.
> 
One problem with this is that .msf files are rebuilt from scratch rather too
readily, for example on a time zone change (a real problem for me when I change
time zones, see bug 136049 and especially my comment 45 to it). If these flags
are stored only in the .msf files, the information is completely lost when the
.msf file are rebuilt.

Comment 29

[Mike Cowperthwaite]

Bug 257532 claims this bug is still present in TB 0.7.2/0.7.3.

Comment 30

[David :Bienvenu]

I believe this is fixed in nightly aviary builds, and nightly trunk builds, but
I don't know that it's fixed in .7.2 or .7.3...

Comment 31

[Mike Cowperthwaite]

*** Bug 258120 has been marked as a duplicate of this bug. ***

Comment 32

[Mike Cowperthwaite]

*** Bug 257532 has been marked as a duplicate of this bug. ***

Comment 33

[Mike Cowperthwaite]

See bug 258735.

Comment 34

[Frank Wein [:mcsmurf]]

Someone can checkin this patch into 1.7 branch?
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/local/src/nsParseMailbox.cpp&rev=MOZILLA_1_7_BRANCH&root=/cvsroot
shows it hasn't been checked in yet.

Comment 35

[Mike Cowperthwaite]

*** Bug 258735 has been marked as a duplicate of this bug. ***

Comment 36

[Mike Cowperthwaite]

*** Bug 259539 has been marked as a duplicate of this bug. ***

Comment 37

[David :Bienvenu]

fixed on 1.7 branch

Comment 38

[Frank Wein [:mcsmurf]]

was fixed after the release of 1.7.3

Comment 39

[Frank Wein [:mcsmurf]]

*** Bug 260589 has been marked as a duplicate of this bug. ***

Comment 40

[Peter B. West]

Since I started using a Fedora 2 build of 1.7.2, I have intermittently seen
X-Mozilla-Status and X-Mozilla-Status2 headers in the text of my message.  E.g.
<q>
  Another possible solution would be to create a static final boolean called
DEBUG somewhere in your code and then wrap all of your debug code in "if
(DEBUG)" blocks.  Obfuscation will remove these if-blocks from the final bits
whenever DEBUG=false.  While this solution does not X-Mozilla-Status: 8000
X-Mozilla-Status2: 00000000 make use of IDE functionality, it may be sufficient
for what you're doing.
</q>
This occures quite frequently.  Is it related to this bug, or should I report
another?

Comment 41

[Peter B. West]

Mike Cowperthwaite pointed me to bug #248121, which is related to movemail. 
That's the one I want.

Comment 42

[Stefan Borggraefe]

*** Bug 263283 has been marked as a duplicate of this bug. ***

Comment 43

[Stefan Borggraefe]

*** Bug 263445 has been marked as a duplicate of this bug. ***

Comment 44

[Harold Groteman]

(In reply to comment #43)
> *** Bug 263445 has been marked as a duplicate of this bug. ***

Suggestion for a better description of this bug and reducing number of duplicates:

Incoming mail that is sent with extra headers "x-mozilla-status" and
"x-mozilla-status" cause Mozilla to indicate 'new mail' as 'read mail'.

Comment 45

[Matthias Bockelkamp]

Some more information for the summary.

Comment 46

[Mike Cowperthwaite]

*** Bug 263913 has been marked as a duplicate of this bug. ***

Comment 47

[Ian Nartowicz]

*** Bug 245230 has been marked as a duplicate of this bug. ***

Comment 48

[Mike Cowperthwaite]

*** Bug 200117 has been marked as a duplicate of this bug. ***

Comment 49

[David :Bienvenu]

*** Bug 259698 has been marked as a duplicate of this bug. ***

Comment 50

[Jesse Ruderman]

*** Bug 269678 has been marked as a duplicate of this bug. ***

Comment 51

[Jesse Ruderman]

*** Bug 256112 has been marked as a duplicate of this bug. ***