Bug 6823 - [review] Malformed messages allow evasion of URIBL checks / Last body line skipped if multipart end boundary missing
Summary: [review] Malformed messages allow evasion of URIBL checks / Last body line sk...
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: 3.3.2
Hardware: PC Linux
: P2 normal
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-12 03:27 UTC by Marco d'Itri
Modified: 2012-09-27 07:05 UTC (History)
4 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
A malformed message which allows evading URI checks. text/plain None Marco d'Itri [NoCLA]
Proposed fix patch None Henrik Krohns [HasCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Marco d'Itri 2012-08-12 03:27:19 UTC

    
Comment 1 Marco d'Itri 2012-08-12 03:30:16 UTC
A malformed MIME message with an URI in the last line of the full body allows perfectly evading URIBL checks, because the URI is totally ignored.
This has been used by a spamming operation for months with massive spam-runs.

See the attached sample for details.
Comment 2 Marco d'Itri 2012-08-12 03:32:02 UTC
Created attachment 5083 [details]
A malformed message which allows evading URI checks.
Comment 3 Marco d'Itri 2012-08-12 03:33:59 UTC
Just to be clear: URIBL checks fail because the last line of these messages is not considered part of the body, so uri and rawbody rules do fail as well. A full rule is needed to be able to match the line content.
Comment 4 Henrik Krohns 2012-08-12 10:28:32 UTC
It seems Message.pm does not handle missing boundary end well. From what I see, it has been like this since 2004..

    # if we're on the last body line, or we find any boundary marker,
    # deal with the mime part
    if ( --$line_count == 0 || (defined $boundary && /^--\Q$boundary\E(?:--)?\s*$/) ) {
      my $line = $_; # remember the last line

After that, there's nothing adding the "remembered $line" to body array, so indeed it goes missing. I could come up with a quick patch, not sure if there are any caveeats..
Comment 5 Henrik Krohns 2012-08-12 12:21:00 UTC
Created attachment 5085 [details]
Proposed fix


Here's a minimum change patch proposal.

I've run it against 20000s/10000h multipart messages and reviewed the diffs of internal data (Data::Dumper(find_parts)) by hand. Zero differences in ham and only few dozen cases of missing end boundary in spam. But that's my very old corpus.

So the change looks safe to me.. waiting for another opinion to commit. ;-)
Comment 6 Kevin A. McGrail 2012-08-12 15:40:15 UTC
+1 from me.  I would say this is urgent and needs testing but from your code, you are just pushing the last part of the message into the buffer and I can't predict many issues.

Great catch.

And definitely open another ticket for the idea " Could "missing end boundary" be a useful rule?"

I don't think the concern that the message was modified/corrupted along the way should be consider valid.  It will fail many other checks such as DKIM as well.

regards,
KAM
Comment 7 Henrik Krohns 2012-08-13 08:07:29 UTC
Ok patch as is in trunk..

Sending        Message.pm
Transmitting file data .
Committed revision 1372304.
Comment 8 Mark Martinec 2012-08-14 14:30:46 UTC
> Ok patch as is in trunk..
> Committed revision 1372304.

+1  looks alright to me (code inspection)
Comment 9 Henrik Krohns 2012-09-27 07:05:25 UTC
I guess it works so resolved.