Bug 6845 - Message parsing incorrectly detects MIME headers in message body
Summary: Message parsing incorrectly detects MIME headers in message body
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: PC Linux
: P2 normal
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-28 21:34 UTC by John Hardin
Modified: 2012-10-11 15:55 UTC (History)
2 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Malformed message, MIME headers in the body application/octet-stream None John Hardin [HasCLA]
insert blank line in body lines array if needed to fix multipart parsing patch None John Hardin [HasCLA]
Neatened up initial patch patch None John Hardin [HasCLA]
patch with test patch None John Hardin [HasCLA]
Show Obsolete (2) Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description John Hardin 2012-09-28 21:34:04 UTC 
Created attachment 5093 [details]
Malformed message, MIME headers in the body

Running a spample (attached) through my test bed I found a rule that should hit this message (MIME_NO_TEXT) does not. Review of the debug log shows that the MIMEHeader plugin is detecting the "Content-Type: text/plain" line in the message body.

While these appear to be MIME headers, there is a blank line before them so they are neither part of the message headers nor part of a MIME body part header. They should not be detected by mimeheader rules.
Comment 1 John Hardin 2012-09-28 22:11:42 UTC 
Followup: if there are *two* blank lines between the message header and the message body, the headers are properly ignored.
Comment 2 John Hardin 2012-09-28 22:17:51 UTC 
The MIMEHeader plugin is a lightweight wrapper around the parsed message structure. The problem is apparently somewhere in Message.pm
Comment 3 John Hardin 2012-09-29 01:07:01 UTC 
Question about one possible approach to corrrecting this: would it be reasonable to insert a blank line as the first line of the body lines array if the MIME type is multipart, and the first line of the body lines array is not blank, and the first line of the body lines array does not start with the MIME boundary string? This seems to me to be a simple way to avoid the bug without destabilizing the parser or making any substantive changes to the message being scanned...
Comment 4 John Hardin 2012-09-29 01:27:22 UTC 
Created attachment 5094 [details]
insert blank line in body lines array if needed to fix multipart parsing

Proposed patch per above suggestion, comments & votes solicited.
Comment 5 John Hardin 2012-09-29 20:02:50 UTC 
Created attachment 5095 [details]
Neatened up initial patch
Comment 6 Kevin A. McGrail 2012-10-01 03:31:16 UTC 
The patch seems straightforward but can you think of any way to add t/ tests to check this code to make sure it doesn't have a larger impact than intended?
Comment 7 John Hardin 2012-10-02 14:52:50 UTC 
(In reply to comment #6)
> The patch seems straightforward but can you think of any way to add t/ tests
> to check this code to make sure it doesn't have a larger impact than
> intended?

I'm not experienced in writing Perl test cases, but I'm sure it can be done. :)

I did run it through a make test and fixed one minor issue (I initially assumed @message would never be empty); all existing tests pass.

A test case that expresses the problem (i.e. fails when the patch is not present) is easy to design; figuring out what it might screw up is a little more complicated. I went through my corpus looking for properly-formatted multipart messages, and didn't see any possibilities where inserting a blank line at the head this way would be an obvious problem.

I'll take a look at the existing tests and see if any of them can be easily leveraged to add some cases that would indicate if this caused problems. Suggestions solicited.
Comment 8 Kevin A. McGrail 2012-10-02 16:58:40 UTC 
I would say any test to start with is good.  we can build on it from there.  But I'm always afraid of these tiny core changes rippling things badly.
Comment 9 John Hardin 2012-10-11 03:43:36 UTC 
Created attachment 5099 [details]
patch with test


(In reply to comment #8)
> I would say any test to start with is good.  we can build on it from there. 
> But I'm always afraid of these tiny core changes rippling things badly.

Okay, I took a look at the existing MIME parser tests. They appear to give fairly good coverage of valid forms and they all pass so I don't _think_ any new tests are needed there.

I did add one malformed MIME test that fails without the patch.
Comment 10 Kevin A. McGrail 2012-10-11 13:48:04 UTC 
(In reply to comment #9)
> Created attachment 5099 [details]
> patch with test
> 
> 
> (In reply to comment #8)
> > I would say any test to start with is good.  we can build on it from there. 
> > But I'm always afraid of these tiny core changes rippling things badly.
> 
> Okay, I took a look at the existing MIME parser tests. They appear to give
> fairly good coverage of valid forms and they all pass so I don't _think_ any
> new tests are needed there.
> 
> I did add one malformed MIME test that fails without the patch.

I like the test.  Applies clean to trunk, looks like it passes make test (have a full running now but had other patches intermixed) and looks sound and I think it should move forward.  Haven't tested my fears that it breaks something internal though so some production esque testing would be nice from others.

I say commit it.
Comment 11 John Hardin 2012-10-11 14:29:23 UTC 
Commit 1397075
Comment 12 John Hardin 2012-10-11 14:44:14 UTC 
spamassassin2.zones.apache.org svn updated so that this change will affect nightly masschecks
Comment 13 Kevin A. McGrail 2012-10-11 15:08:38 UTC 
(In reply to comment #12)
> spamassassin2.zones.apache.org svn updated so that this change will affect
> nightly masschecks

Doesn't that happen automatically?
Comment 14 John Hardin 2012-10-11 15:13:22 UTC 
(In reply to comment #13)
> (In reply to comment #12)
> > spamassassin2.zones.apache.org svn updated so that this change will affect
> > nightly masschecks
> 
> Doesn't that happen automatically?

Apparently not. When I ran "svn up" a *lot* of stuff was updated. Including the recent masscheck scripting changes.
Comment 15 Kevin A. McGrail 2012-10-11 15:55:55 UTC 
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > spamassassin2.zones.apache.org svn updated so that this change will affect
> > > nightly masschecks
> > 
> > Doesn't that happen automatically?
> 
> Apparently not. When I ran "svn up" a *lot* of stuff was updated. Including
> the recent masscheck scripting changes.

Email me outside of bugzilla with what dir was it in because masscheck should checkout the latest version from SVN as it runs?

FYI, my make test completed without issue as well.