You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spamassassin.apache.org by bu...@bugzilla.spamassassin.org on 2011/05/07 15:24:48 UTC
[Bug 6583] New: get_content_preview should be safe
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
Bug #: 6583
Summary: get_content_preview should be safe
Product: Spamassassin
Version: 3.3.1
Platform: All
OS/Version: All
Status: NEW
Severity: minor
Priority: P3
Component: Libraries
AssignedTo: dev@spamassassin.apache.org
ReportedBy: hege@hege.li
Classification: Unclassified
Documentation:
"This is returned as a multi-line string, with the lines separated by C<\n>
characters, containing a fully-decoded, safe, plain-text sample of the first
few lines of the message body."
The preview isn't actually "binary safe". I would assume something like that.
There are many cases where SA ends up scanning binary bodies.
--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
[Bug 6583] log_message and get_content_preview should print safe
Posted by bu...@bugzilla.spamassassin.org.
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
Henrik Krohns <he...@hege.li> changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|[review] log_message and |log_message and
|get_content_preview should |get_content_preview should
|print safe |print safe
--- Comment #7 from Henrik Krohns <he...@hege.li> 2011-05-09 17:55:56 UTC ---
Very nice insights Mark, didn't notice Logger/* .. I'll look into this later,
if someone doesn't before me.
--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
[Bug 6583] [review] log_message and get_content_preview should print
safe
Posted by bu...@bugzilla.spamassassin.org.
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
Henrik Krohns <he...@hege.li> changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|get_content_preview should |[review] log_message and
|be safe |get_content_preview should
| |print safe
--- Comment #4 from Henrik Krohns <he...@hege.li> 2011-05-08 07:00:36 UTC ---
(In reply to comment #3)
> I'm not sure if [:print:] is going to have the same locale problems as lc(), or
> if it'll be fine due to using whatever character set the body was encoded in.
> Looks like there's a simpler fix:
>
> $str =~ s/\P{IsPrint}/?/gs;
Again I'd like to be cautious with introducing untested features.. since
especially old versions of Perl had even crashes with utf8 related things. And
I don't really think we need to care utf8 with my proposal here, we just escape
characters for the sake of readability.
Here's what I propose, atleast for trunk.. if someone really wants to directly
see some high characters like latin special characters or euro symbols let me
know.
Index: PerMsgStatus.pm
===================================================================
--- PerMsgStatus.pm (revision 1100366)
+++ PerMsgStatus.pm (working copy)
@@ -626,6 +626,9 @@
$str = Mail::SpamAssassin::Util::wrap($str, " ", "Content preview: ", 75,
1);
$str =~ s/^Content preview:\s+//gs;
+ # Bug 6583: replace unsafe characters with decimal octets "\000" - "\255"
+ $str =~ s/([\x00-\x09\x0b-\x1f\x7f-\xff])/sprintf("\\%03d",ord($1))/egs;
+
return $str;
}
Index: Logger.pm
===================================================================
--- Logger.pm (revision 1098151)
+++ Logger.pm (working copy)
@@ -174,9 +174,10 @@
# the subclasses having to understand multi-line logs
my $first = 1;
foreach my $line (split(/\n/, $message)) {
- # replace control characters with "_", tabs and spaces get
- # replaced with a single space.
- $line =~ tr/\x09\x20\x00-\x1f/ _/s;
+ # tabs and spaces get replaced with a single space.
+ $line =~ tr/\x09\x20/ /s;
+ # Bug 6583: replace unsafe characters with decimal octets "\000" - "\255"
+ $line =~ s/([\x00-\x09\x0b-\x1f\x7f-\xff])/sprintf("\\%03d",ord($1))/egs;
if ($first) {
$first = 0;
} else {
--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
[Bug 6583] [review] log_message and get_content_preview should print
safe
Posted by bu...@bugzilla.spamassassin.org.
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
Kevin A. McGrail <km...@pccc.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |kmcgrail@pccc.com
Target Milestone|3.3.2 |3.4.0
--- Comment #5 from Kevin A. McGrail <km...@pccc.com> 2011-05-09 16:42:22 UTC ---
Too untested and too many questions to consider for 3.3.X.
--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
[Bug 6583] log_message and get_content_preview should print safe
Posted by bu...@bugzilla.spamassassin.org.
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
Kevin A. McGrail <km...@pccc.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|3.4.0 |3.4.1
--- Comment #8 from Kevin A. McGrail <km...@pccc.com> ---
Moving all open bugs where target is defined and 3.4.0 or lower to 3.4.1 target
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 6583] get_content_preview should be safe
Posted by bu...@bugzilla.spamassassin.org.
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
Henrik Krohns <he...@hege.li> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |hege@hege.li
--- Comment #1 from Henrik Krohns <he...@hege.li> 2011-05-07 13:28:06 UTC ---
Any objections? Or change it to just very bad control characters?
--- lib/Mail/SpamAssassin/PerMsgStatus.pm (revision 1100366)
+++ lib/Mail/SpamAssassin/PerMsgStatus.pm (working copy)
@@ -619,6 +619,9 @@
$str =~ s/[-_\*\.]{10,}//gs;
$str =~ s/\s+/ /gs;
+ # Bug 6583: it's supposed to be "safe", so replace unprintable with ?
+ $str =~ s/[^[:print:]]/?/gs;
+
# add "Content preview:" ourselves, so that the text aligns
# correctly with the template -- then trim it off. We don't
# have to get this *exactly* right, but it's nicer if we
--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
[Bug 6583] get_content_preview should be safe
Posted by bu...@bugzilla.spamassassin.org.
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
Darxus <Da...@ChaosReigns.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |Darxus@ChaosReigns.com
--- Comment #3 from Darxus <Da...@ChaosReigns.com> 2011-05-07 14:23:11 UTC ---
I'm not sure if [:print:] is going to have the same locale problems as lc(), or
if it'll be fine due to using whatever character set the body was encoded in.
Looks like there's a simpler fix:
$str =~ s/\P{IsPrint}/?/gs;
>From the perlre man page:
But if the "locale" or "encoding" pragmas are not used and the string is not
"utf8", then "[[:xxxxx:]]" (and "\w", etc.) will not match characters
0x80-0xff; whereas "\p{IsXxxxx}" will force the string to "utf8" and can match
these characters (as Unicode).
[2] "\p{IsPrint}" matches characters 0x09-0x0d but "[[:print:]]" does not.
--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
[Bug 6583] [review] log_message and get_content_preview should print
safe
Posted by bu...@bugzilla.spamassassin.org.
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
--- Comment #6 from Mark Martinec <Ma...@ijs.si> 2011-05-09 17:06:14 UTC ---
Some comments, before I forget:
Considering that logging has three different back-ends internally
(file, stderr and syslog) and that it has a documented open interface
for third-party backends (which is used at least by amavisd-new to
integrate SpamAssassin logging with its own), I'd prefer that each
backend logger takes care of its own limitations, which may not be
the same for each.
For example a file-based logging may be happy with logging UTF-8
text, while the syslog may not be. A syslog backend may have line
length limitations, while others may not.
A concern with the amavisd backend logger is that it does its own
sanitation of nonprintable characters, so if SpamAssassin does it
at the common Logger.pm routine, we'd end up with doubled backslashes
- not too bad, but ugly nevertheless.
So I'd move the sanitation line from Logger.pm into Logger/*.pm modules.
And perhaps the Logger/File.pm is a candidate for less strict sanitation,
like allowing 8-bit characters through.
Btw, the suggested s/// does not cope with character codes above 255,
which may happen to find its way into a string being sanitized.
Also the '\' is not being doubled, which means that the result is not
uniquely reversible (if it happens that a '\nnn' is literally in a string).
Perhaps something like the following would do:
$str =~ s{([^\040-\176])}
{ $1 eq '\\' ? '\\\\' :
sprintf(ord($1) > 255 ? '\\x{%04x}' : '\\%03o', ord($1)) }egs;
An alternative for sanitizing strings with characters above 255 is
to encode them first into utf-8, and only then \ooo -quote them.
Not sure what subset of characters should be allow to pass through
un-encoded in PerMsgStatus::get_content_preview().
--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
[Bug 6583] get_content_preview should be safe
Posted by bu...@bugzilla.spamassassin.org.
https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6583
--- Comment #2 from Henrik Krohns <he...@hege.li> 2011-05-07 13:38:24 UTC ---
Or better yet, show unprintable as hex "\xFF".
I'd also like to do this with the whole debug output (atleast the matched rules
part), since it's crapped my terminal many times..
--
Configure bugmail: https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.