Bug #113464 for MailTools: Mail::Header incorrect decoding

* Andrew Colin Kissa via RT (bug-MailTools@rt.cpan.org) [160330 22:04]: Show quoted text
Why would it by-pass the filters? An application which scans for spam and malware must first prepare the content of the message to make it possible to scan. Well, when that code uses MailTools, it needs to do the base64 decoing itself. If it does not, that application has a bug. It's not MailTools fault if it is used incorrectly. It's a very limited module. So: no, lacking of handling of Content-Transfer-Encoding does not not have any effect on filtering software. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net

* Andrew Colin Kissa via RT (bug-MailTools@rt.cpan.org) [160330 22:31]: Show quoted text
I have no knowledge about MIME-Tools, but a quick scan shows http://search.cpan.org/~dskoll/MIME-tools-5.507/lib/MIME/Body.pm ### Dump the ENCODED body data to a filehandle: $body->print(\*STDOUT); ### Slurp all the UNENCODED data in, and put it in a scalar: $string = $body->as_string; ### Slurp all the UNENCODED data in, and put it in an array of lines: @lines = $body->as_lines; So, the latted two options are documented to decode base64, and the first is not. It is documented, hence not a bug of the module. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net

* Dianne Skoll via RT (bug-MailTools@rt.cpan.org) [160330 23:02]: Show quoted text
Ah, that's a clearer report. Usually, you stop when you cannot handle mistakes automatically. Do the other lines end-up in the body? I have no idea about the logic you use around it: there are a many ways. As far as I can see in the code, you get croak "Bad RFC822 field name '$tag'\n"; So, the program needs to eval the header reading, and decide what to do. Probably best to trash the whole message. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net

Show quoted text
That does not appear to be the case.. Since example code is always clearer: #!/usr/bin/perl use strict; use warnings; use Mail::Header; use File::Temp qw# tempfile #; my $headers = <<EOF; Content-Disposition: inline; filename="testing.txt" Content-Type: text/ascii; x-unix-mode=0600; name="testing.txt" Content-Transfer-Encoding: base64 EOF my ($fh_out, $filename) = tempfile(); print $fh_out $headers; close $fh_out; open my $fh_in, "<", $filename or die "Can't open $filename: $!"; my $obj = Mail::Header->new([ split m/\n/, $headers ]); print "Extract method:\n"; print "---------------\n"; print $obj->as_string, "\n"; print "\n"; print "Read method:\n"; print "------------\n"; my $obj2 = Mail::Header->new($fh_in); print $obj2->as_string, "\n"; __END__ Running this will show: Extract method: --------------- Content-Disposition: inline; filename="testing.txt" Content-Type: text/ascii; x-unix-mode=0600; Read method: ------------ Content-Disposition: inline; filename="testing.txt" Content-Type: text/ascii; x-unix-mode=0600; => No fatal error is given; When this module is used via MIME::Parser: it has extra code to detect that Mail::Header did not consume all input (and thus all headers); it does flag this as an error but in the default configuration it continues processing the message. The body that followed after the above header was base64 encoded, MIME::Parser however will not base64 decode it since the parsed headers do *not* include 'Content-Transfer-Encoding: base64'. Checking with some mail clients: (i.e. mime part with the above headers + base64 encoded content): both Outlook and Thunderbird are forgiving and base64 decodes the attachment anyway.. There is a case to be made to fixing this but I guess there is also a case to be made for not fixing it (since it are not valid headers).. Looking at this from the point of view of a mail filter such as amavisd: it uses MIME::Parser (which then uses Mail::Header) amavisd does check the result of MIME::Parser (which did record the error) and in a default setup it would reject the mail because of the bad header; The bad header check however can be disabled (or address can be white-listed) and if that happens then other features of amavisd will no longer work correctly.. For example: if the attachment is a ZIP archive then it will attempt to extract it but if the headers are mangled then this will not work since the body is still base64 encoded which will make it impossible to extract/check the archive.. (i.e. virus scanning) Possible patch: --- Mail/Header.pm.orig 2016-04-04 12:36:23.000000000 +0200 +++ Mail/Header.pm 2016-04-04 15:20:36.000000000 +0200 @@ -282,9 +282,11 @@ { my ($self, $lines) = @_; $self->empty; - while(@$lines && $lines->[0] =~ /^($FIELD_NAME|From )/o) - { my $tag = $1; - my $line = shift @$lines; + while(@$lines) + { my $line = shift @$lines; + $line =~ /^($FIELD_NAME|From )/o or next; + my $tag = $1; + $line .= shift @$lines while @$lines && $lines->[0] =~ /^[ \t]+/o; @@ -320,10 +322,12 @@ { ($tag, $line) = _fmt_line $self, $tag, $line; _insert $self, $tag, $line, -1 if defined $line; + $line = undef; + $tag = undef; } - defined $ln && $ln =~ /^($FIELD_NAME|From )/o - or last; + defined $ln or last; + $ln =~ /^($FIELD_NAME|From )/o or next; ($tag, $line) = ($1, $ln); } Running with the above script: Extract method: --------------- Content-Disposition: inline; filename="testing.txt" Content-Type: text/ascii; x-unix-mode=0600; Content-Transfer-Encoding: base64 Read method: ------------ Content-Disposition: inline; filename="testing.txt" Content-Type: text/ascii; x-unix-mode=0600; Content-Transfer-Encoding: base64

Show quoted text
I have no real opinion about that; for my use case the fragment is irrelevant so throwing it away is good; adding it to the previous line would also be good.. But thinking about the issue and patch some more: There might be two extra things that need to be considered: a) (ab)using Mail::Header to detect incorrect MIME headers Before the patch Mail::Header would stop processing the input and this made it possible to detect that the headers were not fully correct. Of course this lead to the bug of missing headers. After the patch the valid headers are added (which is good) but there is no indication that some of the content was bogus.. The docs of the module does mention: "Mail::Header does not always follow the RFCs strict enough, ..." So someone using it to check that a message follows the RFCs would already be missing some cases I guess.. I suppose extra code could be added to set a particular flag and/or an extra method to check the value of this flag.. Not sure how good/clean that would be.. b) using Mail::Header on the full mail message When someone uses Mail::Header on the full email message (and not just on the headers) then it would result in a different behaviour.. Consider the code: #!/usr/bin/perl use strict; use warnings; use Mail::Header; use File::Temp qw# tempfile #; my $msg = <<EOF; Subject: foo From: <bar\@bar.bar> Content-Type: text/plain This is the mail body Foo: bar baz EOF my ($fh_out, $filename) = tempfile(); print $fh_out $msg; close $fh_out; open my $fh_in, "<", $filename or die "Can't open $filename: $!"; my @headers = split m/\n/, $msg; my $obj = Mail::Header->new(); $obj->extract(\@headers); print "Extract method (headers):\n"; print "-------------------------\n"; print $obj->as_string, "\n"; print "\n"; print "Extract method (body):\n"; print "----------------------\n"; print join("\n", @headers, ""); print "\n"; print "\n"; print "Read method (headers):\n"; print "----------------------\n"; my $obj2 = Mail::Header->new($fh_in); print $obj2->as_string, "\n"; my $body = do { local $/; <$fh_in> }; $body = "" if not defined $body; print "Read method (body):\n"; print "-------------------\n"; print $body; __END__ Before the patch this would output: Extract method (headers): ------------------------- Subject: foo From: <bar@bar.bar> Content-Type: text/plain Extract method (body): ---------------------- This is the mail body Foo: bar baz Read method (headers): ---------------------- Subject: foo From: <bar@bar.bar> Content-Type: text/plain Read method (body): ------------------- This is the mail body Foo: bar baz After the patch the output would be: Extract method (headers): ------------------------- Subject: foo From: <bar@bar.bar> Content-Type: text/plain Foo: bar Extract method (body): ---------------------- Read method (headers): ---------------------- Subject: foo From: <bar@bar.bar> Content-Type: text/plain Foo: bar Read method (body): ------------------- So for someone (ab)using Mail::Header in that way the patch may break their code... Of course their code is already broken in case there are invalid headers What I do not know: * is there anyone that (ab)uses Mail::Header in such way? * is this behaviour that you want to support/maintain/recommend? * ... A possible 'fix' for it would be to stop at and empty line.. i.e. for the extract method: $line !~ /\A\Z/o or last; $line =~ /^($FIELD_NAME|From )/o or next; my $tag = $1; and for the read method: defined $ln or last; $ln !~ /\A\Z/o or last; $ln =~ /^($FIELD_NAME|From )/o or next; i.e.: * stop on an empty line * skip invalid headers

Show quoted text
I've tested the attached version with my test cases (I'm sure you did the same but can never hurt to double check) and it does produce the expected output.

This change to 2.15 broke many applications...