Skip Menu |
 

This queue is for tickets about the MailTools CPAN distribution.

Report information
The Basics
Id: 79993
Status: resolved
Priority: 0/
Queue: MailTools

People
Owner: Nobody in particular
Requestors: tsibley [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

Attachments
0001-Reject-and-warn-about-bad-header-continuations.patch
0002-Tests-to-ensure-properly-continued-header-values-are.patch



Subject: [PATCH] Reject and warn about bad header continuations
Date: Wed, 03 Oct 2012 15:15:58 -0700
To: bug-MailTools [...] rt.cpan.org
From: Thomas Sibley <tsibley [...] cpan.org>
Download (untitled) / with headers
text/plain 260b
Newlines without a space after them are invalid continuations and may lead to hidden or invalid headers once the object is stringified. Refuse to insert headers with invalid continuations, but allow folding (when Modify is true) to fix them up automatically.

Message body is not shown because sender requested not to inline it.

Subject: Re: [rt.cpan.org #79993] [PATCH] Reject and warn about bad header continuations
Date: Wed, 03 Oct 2012 15:25:26 -0700
To: bug-MailTools [...] rt.cpan.org
From: Thomas Sibley <tsibley [...] cpan.org>
Another patch which adds two tests for an additional sanity check.

Message body is not shown because sender requested not to inline it.

Download (untitled) / with headers
text/plain 270b
Updated patch, more tests, supersedes the previous two patches. I've combined the two patches above into one as well as updated the regex to be more RFC 2822 compliant with regards to whitespace ([ \t] are valid not \s) and continuations containing _only_ whitespace.
Subject: 0001-Reject-and-warn-about-bad-header-continuations.patch
From 9dbc62b96c3ed4cd260684fa8473b9e02f834e8e Mon Sep 17 00:00:00 2001 From: Thomas Sibley <trs@bestpractical.com> Date: Wed, 3 Oct 2012 14:07:03 -0700 Subject: [PATCH] Reject and warn about bad header continuations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Newlines without a space or tab after them are invalid continuations and may lead to hidden or invalid headers once the object is stringified. Newlines with _only_ space after them before the next newline are also invalid continuations per RFC 2822 § 3.2.3. Refuse to insert headers with invalid continuations, but allow folding (when Modify is true) to fix them up automatically. --- lib/Mail/Header.pm | 10 ++++++++-- t/header.t | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/lib/Mail/Header.pm b/lib/Mail/Header.pm index 644215d..b275dd2 100644 --- a/lib/Mail/Header.pm +++ b/lib/Mail/Header.pm @@ -180,8 +180,14 @@ sub _fmt_line || $HDR_LENGTHS{$tag} || $self->fold_length; - _fold_line $line, $maxlen - if $modify && defined $maxlen; + if ($modify && defined $maxlen) { + # folding will fix bad header continuations for us + _fold_line $line, $maxlen; + } + elsif ($line =~ /\r?\n([^ \t]|\s+$)/om) { + return _error "Bad RFC 2822 header continuation, skipping '$tag': ", + "No whitespace or only whitespace after newline in '$line'\n"; + } $line =~ s/\n*$/\n/so; ($tag, $line); diff --git a/t/header.t b/t/header.t index 4585f2a..2459ce4 100644 --- a/t/header.t +++ b/t/header.t @@ -1,6 +1,6 @@ require Mail::Header; -print "1..25\n"; +print "1..55\n"; $h = new Mail::Header; @@ -173,3 +173,56 @@ printf "ok %d\n",++$t; print $h->as_string,"\n----\n",$headout,"\nnot " unless $h->as_string eq $headout; printf "ok %d\n",++$t; + +{ + my @warnings; + local $SIG{__WARN__} = sub { push @warnings, "@_"; warn @_; }; + + my @bad_headers = ( + "To: foo\@example.com\nBcc: evil\@example.com\n", # inserted header + "To: foo\n\n bar\n", # newline doesn't count as WSP + "To: foo\n \t\n", # continuation with only WSP + "To: foo\n \n bar\n", # middle continuation with only WSP + ); + + for my $header (@bad_headers) { + print "not " + unless $h = new Mail::Header [$header]; + printf "ok %d\n",++$t; + + print "not " + unless @warnings == 1 and shift(@warnings) =~ /bad rfc 2822 header continuation/i; + printf "ok %d\n",++$t; + + print "not " + if $h->get("To") or $h->get("Bcc"); + printf "ok %d\n",++$t; + + print "not " + unless $h = new Mail::Header [$header], Modify => 1; + printf "ok %d\n",++$t; + + print "not " + if @warnings; + printf "ok %d\n",++$t; + + (my $to = $header) =~ s/(^To: |\n(?!\z))//g; + $to =~ s/\s*(?=\n\z)//; + print "not " + unless $h->get("To") eq $to; + printf "ok %d\n",++$t; + + print "not " + if $h->get("Bcc"); + printf "ok %d\n",++$t; + } +} + +my $continued = "foo\@example.com,\n bar\@example.com\n"; +print "not " + unless $h = new Mail::Header ["To: $continued"]; +printf "ok %d\n",++$t; + +print "not " + unless $h->get("To") eq $continued; +printf "ok %d\n",++$t; -- 1.7.11.3
Subject: Re: [rt.cpan.org #79993] [PATCH] Reject and warn about bad header continuations
Date: Thu, 4 Oct 2012 11:18:27 +0200
To: Thomas Sibley via RT <bug-MailTools [...] rt.cpan.org>
From: Mark Overmeer <secretaris [...] nluug.nl>
Download (untitled) / with headers
text/plain 691b
* Thomas Sibley via RT (bug-MailTools@rt.cpan.org) [121003 22:26]: Show quoted text
> Queue: MailTools > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=79993 > > > Another patch which adds two tests for an additional sanity check.
Although this change may break peoples broken code, I have accepted them for 2.12 (not immediately released unless you need it). Thanks! -- MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
Subject: Re: [rt.cpan.org #79993] [PATCH] Reject and warn about bad header continuations
Date: Thu, 04 Oct 2012 16:21:22 -0700
To: bug-MailTools [...] rt.cpan.org
From: Thomas Sibley <tsibley [...] cpan.org>
Download (untitled) / with headers
text/plain 730b
On 10/04/2012 02:18 AM, Mark Overmeer via RT wrote: Show quoted text
> Although this change may break peoples broken code, I have accepted > them for 2.12 (not immediately released unless you need it). Thanks!
Thanks. I don't need an immediate release. I think it's very important to reject invalid continuations, but if you have thoughts on a way to effect the same change with less potential for breaking existing-but-wrong code, I'd certainly be in support that. I considered but rejected the idea of accepting the header up until the point of the invalid continuation and discarding the rest. It's hard for Mail::Header to know what the content is, and keeping some and not the rest might be more wrong (and invalid in ways unknowable).
Subject: Re: [rt.cpan.org #79993] [PATCH] Reject and warn about bad header continuations
Date: Thu, 04 Oct 2012 16:23:22 -0700
To: bug-MailTools [...] rt.cpan.org
From: Thomas Sibley <tsibley [...] cpan.org>
Download (untitled) / with headers
text/plain 485b
On 10/04/2012 04:21 PM, Thomas Sibley wrote: Show quoted text
> I considered but rejected the idea of accepting the header up until the > point of the invalid continuation and discarding the rest. It's hard > for Mail::Header to know what the content is, and keeping some and not > the rest might be more wrong (and invalid in ways unknowable).
The other option was always fixing the invalid continuations, but that breaks the contract of the Modify option and so I didn't think it was on the table.
Got fixed in 2.12


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.