Do not deliver message with whole content unparsable

[giosh94mhz]

There is a bad behaviour with malformed multipart messages. This happens when a message containes the following snippet (for example):

Content-Type: multipart/mixed;
?boundary="LONG_HASH"

Note the spurious question mark. In this situation the message is completely replaced by a configured message and sent to the user, but it can't hardly find something useful there. It is much better to quarantine the message and prevent the sending.

The fix is quite simple, as MailScanner already handle a cantparse situation and I've just joined two very similar situations. In the end, the message is saved in the quarantine, the log message is the same as before, but the useless placeholder message is prevented.

[akissa]

@giosh94mhz Could you please provide sample messages such that we can test this

[giosh94mhz]

Sure, give me some time to retrieve it.

[giosh94mhz]

Ok, I had to anonymize it: example.eml.zip

I run it with something like:

/usr/sbin/sendmail.postfix -i -f [email protected] [email protected] < example.eml
MailScanner --debug

[msapiro]

The main message headers contain

Content-Type: multipart/mixed;
X-Broken-Headers-Here: Stupid headers were here. Probably a broken application... see below :)

boundary="PO7U53KJUZ1C74Q4T8N637"

Is this what you intended. There are two issues here. X-Broken-Headers-Here: is followed by an empty line, thus ending the headers and everything beyond is body, and even if there were no empty line, it separates the boundary= from the Content-Type: so everything is prologue. Prehaps you intended

Content-Type: multipart/mixed;
 boundary="PO7U53KJUZ1C74Q4T8N637"
X-Broken-Headers-Here: Stupid headers were here. Probably a broken application..
. see below :)

[giosh94mhz]

No I meant it. This is an email I have received and is very broken as you
noted.

This is reason for I think the actual behaviour is incorrect.
Il 28 apr 2016 6:20 PM, "Mark Sapiro" [email protected] ha scritto:

The main message headers contain

Content-Type: multipart/mixed;
X-Broken-Headers-Here: Stupid headers were here. Probably a broken application... see below :)

boundary="PO7U53KJUZ1C74Q4T8N637"

Is this what you intended. There are two issues here.
X-Broken-Headers-Here: is followed by an empty line, thus ending the
headers and everything beyond is body, and even if there were no empty
line, it separates the boundary= from the Content-Type: so everything is
prologue. Prehaps you intended

Content-Type: multipart/mixed;
boundary="PO7U53KJUZ1C74Q4T8N637"
X-Broken-Headers-Here: Stupid headers were here. Probably a broken application..
. see below :)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#65 (comment)

[akissa]

Cann't you provide the actual email even if you have to reduct it.

[msapiro]

For the message you have provided, these are the headers:

Message-ID: <[email protected]>
From: "Someone" <[email protected]>
To: [email protected]
Subject: Boring Content
Date: Wed, 28 Apr 2016 10:13:28 -0500
MIME-Version: 1.0
Content-Type: multipart/mixed;
X-Broken-Headers-Here: Stupid headers were here. Probably a broken application..
. see below :)

and the rest is message body. Since it is a MIME multipart message, everything up to the first boundary is prologue. The defect is there is no boundary, but the

--16NCDS459M2CL646164M12
Content-Type: text/html;
?charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

is all just part of the prologue, not headers of a sub-part so the fact that they are not valid headers is irrelevant.

[giosh94mhz]

Well I can't provide the actual email, since it's full of personal
informations (not only mine). Maybe I could send it privately outside of
github... but still I think is enough for unit testing.

the real message is not different from this. A broken mail, with headers
everywhere and a zip file. The error is triggered by the "missing" boundary
tag. In this case is triggered by the misplaced X header, if you remove it
is triggered by the leading "non-space": whatever is the reason, the file
is completely cut out by MailScanner and then sent to the final user.
Il 28 apr 2016 6:28 PM, "Andrew Colin Kissa" [email protected] ha
scritto:

Cann't you provide the actual email even if you have to reduct it.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#65 (comment)

[msapiro]

For what it's worth, Python's email package parses the message with two defects: A message claimed to be a multipart but had no boundary parameter. and A message claimed to be a multipart but no subparts were found. and calls all the text beginning with the boundary="PO7U53KJUZ1C74Q4T8N637" line the message body. I believe this is a correct interpretation according to RFC2045.

So the question is if we have a message which claims to be multipart but has no boundary, what do we do with it. Granted this particular message is unwanted, but I'm not sure all such messages would be. I'd rather err on the side of delivering a notice about an unwanted message rather than silently not delivering a potentially wanted message.

[akissa]

@msapiro I would say quarantine.

[giosh94mhz]

@msapiro I don't agree. In RFC2045 section 5 there is:

For example, the "charset" parameter is applicable to any subtype of
"text", while the "boundary" parameter is required for any subtype of the
"multipart" media type.

And the whole RFC2046, which is about multiparts, ephatize this requirement
further, especially in section 5/5.1

Of course, a good MUA, like Thunderbird or Mutt, need to try hard and
interpret the message by guessing the delimiter, but since we are talking
about MailScanner... quarantine all the things! :)
Il 28 apr 2016 8:19 PM, "Andrew Colin Kissa" [email protected] ha
scritto:

@msapiro https://github.com/msapiro I would say quarantine.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#65 (comment)

[msapiro]

@akissa The current behavior is to quarantine. The PR is to not deliver the notice to the recipient about the quarantined message.

If anything, I think this should be a configuration setting with a possible rule set, but the only setting I see is Notify Senders Of Other Blocked Content which controls the notice to the sender, not the recipient.

@giosh94mhz When I run your test message through current MailScanner, it IS quarantined and the recipient receives only the notice that it's been quarantined. I thought your PR is to not deliver the notice to the recipient. If the actual message is being delivered, do you have Dangerous Content Scanning = yes in your config?

[giosh94mhz]

When I said quarantine I meant "go to jail" not just "save the original copy"

I see your point, but I this it doesn't fit the rest of the MailScanner application though. Sending a message with no useful content, only to have "hopefully meaningful" subject and/of from address is wrong to me. If going this way, then all the spam/viruses/garbage will deserve a notification without content. MailScanner already support the generation of report to the postmaster (or whomever) by setting Notices To and it's up to him to release the message.

In my configurations Dangerous Content Scanning is enabled, of course. I also have cron scripts which send daily reports to internal addresses with summary information of the mail quarantined. I think this is way to go.

Consider a scenario like this (a real anonymized story, by the way). EvilSpammer setup an "appointment confirmed for today" campaign and send it to all the email address he collected by crawling the web, but he left a small bug in his goPhish.rb and the boundaries are not leaded by whitespace. Meanwhile in a state far far away, all 100 employee of Acme inc are receiving an email named "appointment confirmed for today", but with content "Hey, something bad happened, please contact Acme postmaster, and tell him to check message 12345678.ABCD". Postmaster will have a very bad day... :)

[msapiro]

@giosh94mhz I disagree. I think never sending the notice to the recipient is not the appropriate way to deal with this. If you're really concerned about the scenario you describe, you can always edit the appropriate stored.content.message.txt template to say something else.

I do think it would be appropriate to have a config setting such as Bad Content Notify Recipient (ideally allowing a rule set) to allow a site to control this, but I don't think it's appropriate to unconditionally suppress the notice.

[msapiro]

@giosh94mhz Regarding your point about all spam, etc. There are extensive configuration controls over this. See Spam Actions, High Scoring Spam Actionsand SpamAssassin Rule Actions, and messages containing viruses are still delivered after 'cleaning'.

[giosh94mhz]

Well, I think we are going off topic here, since we started with bugfix and we are now discussing new features and configurations.

Lets's recap why I think this should be merged:

• MailScanner is supposed to do this already if you look at DisinfectAndDeliver function. The current version should delete messages which cannot be disinfected, parsed, and so on... so it's not a new feature, it's a bug.
• we are talking about a mail which is REALLY broken according to RFC2045 and RFC2046, and more likely than not sent by broken software. No thunderbird, Outlook, or other MUA will ever send a very broken mail like this.
• Last not least, is a real problem. I got many email from real users which complain about this particular issue and I made the fix for them (well... myself actually :).

About the notification concern, I think that this kind of error should notified with a ruleset in Send Notices if any.

That's my opinion, I leave to the maintainer (ping @jcbenton) the final choice.

PS: I'm not a native English speaker, so no rudeness/offence intended.

[msapiro]

MailScanner is supposed to do this already if you look at DisinfectAndDeliver function. The current version should delete messages which cannot be disinfected, parsed, and so on... so it's not a new feature, it's a bug.

I don't see a bug. Mailscanner doesn't deliver the message. It delivers a notice about the message.

In fact, I have tested with and without your patch and with Deliver Disinfected Files = both no and yes and the ONLY difference I see is with your patch, the notice to the user is stored.virus.message.txt and without it is stored.content.message.txt.

If this is not what you see, please explain what your patch actually does in your case compared to what unpatched MailScanner does, and provide enough information about your configuration so I can see what you see.

we are talking about a mail which is REALLY broken according to RFC2045 and RFC2046, and more likely than not sent by broken software. No thunderbird, Outlook, or other MUA will ever send a very broken mail like this.

I agree, and so does MailScanner as far as I can tell. It doesn't deliver the message.

About the notification concern, I think that this kind of error should notified with a ruleset in Send Notices if any.

But your patch doesn't do that.

[jcbenton]

Ok, I have not been following this closely. I have been waiting for everyone else to hash this out. However, apparently I need to make something clear.

Whatever change you want to make, it needs to fit 40,000+ installations. What I want MailScanner to do for me and what you want it to do for you is irrelevant. If you have found a bug, address that specific issue and do not try to change the behavior of the intended actions. Mark has mentioned several configurable actions. From my cursory look at this you are trying to bypass those options and have it behave the way you would like. Please stop doing that. (If that is what is going on.)

I am not trying to discourage you from pointing out issues. We do need people to do that. It is even better if you have a suggested fix. Just keep the fix within the scope of the original logic.

Jerry Benton
www.mailborder.com

On May 1, 2016, at 7:04 PM, Mark Sapiro [email protected] wrote:

MailScanner is supposed to do this already if you look at DisinfectAndDeliver function. The current version should delete messages which cannot be disinfected, parsed, and so on... so it's not a new feature, it's a bug.

I don't see a bug. Mailscanner doesn't deliver the message. It delivers a notice about the message.

In fact, I have tested with and without your patch and with Deliver Disinfected Files = both no and yes and the ONLY difference I see is with your patch, the notice to the user is stored.virus.message.txt and without it is stored.content.message.txt.

If this is not what you see, please explain what your patch actually does in your case compared to what unpatched MailScanner does, and provide enough information about your configuration so I can see what you see.

we are talking about a mail which is REALLY broken according to RFC2045 and RFC2046, and more likely than not sent by broken software. No thunderbird, Outlook, or other MUA will ever send a very broken mail like this.

I agree, and so does MailScanner as far as I can tell. It doesn't deliver the message.

About the notification concern, I think that this kind of error should notified with a ruleset in Send Notices if any.

But your patch doesn't do that.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #65 (comment)

[giosh94mhz]

@jcbenton thank you for the feedback. I'm currently handling ~1000 installations of MailScanner with possibily 10k+ domains, so I'm not talking about "I like that", but "my users complained about". This is a small self-promotion just not to look like a PR troll :)

And I really appreciate @msapiro feedback too, even if I'm not following his reasoning correctly as it seems.

In fact, I have tested with and without your patch and with Deliver Disinfected Files = both no and yes and the ONLY difference I see is with your patch, the notice to the user is stored.virus.message.txt and without it is stored.content.message.txt.

Exactly, this is correct IMO. A fully broken message with absolutely nothing readable, is a disinfected message or a "nuked" message? My patch just do the same thing MailScanner already does with all other unparsable message (too many attachment, too big, no main entity): mark it as "virus > other". You probably have options set, which I don't have, that send notifications of viruses. I have set Silent Viruses = All-Viruses, so I don't receive a notification for the message.

Is my understanding correct?

[jcbenton]

Whatever you guys work out, make sure it gets added to the v5 branch.

Jerry Benton
www.mailborder.com

On May 4, 2016, at 9:38 AM, Giorgio Premi [email protected] wrote:

@jcbenton https://github.com/jcbenton thank you for the feedback. I'm currently handling ~1000 installations of MailScanner with possibily 10k+ domains, so I'm not talking about "I like that", but "my users complained about". This is a small self-promotion just not to look like a PR troll :)

And I really appreciate @msapiro https://github.com/msapiro feedback too, even if I'm not following his reasoning correctly as it seems.

In fact, I have tested with and without your patch and with Deliver Disinfected Files = both no and yes and the ONLY difference I see is with your patch, the notice to the user is stored.virus.message.txt and without it is stored.content.message.txt.

Exactly, this is correct IMO. A fully broken message with absolutely nothing readable, is a disinfected message or a "nuked" message? My patch just do the same thing MailScanner already does with all other unparsable message (too many attachment, too big, no main entity): mark it as "virus > other". You probably have options set, which I don't have, that send notifications of viruses. I have set Silent Viruses = All-Viruses, so I don't receive a notification for the message.

Is my understanding correct?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #65 (comment)

[msapiro]

I finally think I see what you are trying to do with this PR. The title, amongst other things is what confused me. To me, "Do not deliver message with whole content unparsable" means don't deliver the message and what you really mean is don't deliver a notice about the message.

OK, I still think this PR is not the appropriate way to address this. "virus" refers to a specific form of malware. An unparseable message is not a virus and shouldn't be categorized as one. I think that if there are other things that aren't viruses which are classified as such, the fact that they are misclassified is not sufficient justification for adding yet another misclassification.

Also, I think that the fact that controls to suppress notifications about viruses exist and controls for other classes may not is not sufficient reason to classify something as a virus which isn't. A better way to deal with this is to add more controls for other classes of unwanted mail.

Clearly, this patch works for you, and that's fine, but I don't think it is the correct approach for the base code.