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 2005/11/27 00:28:24 UTC
[Bug 4707] New: spamc crashes with -B and <6 chars of input
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
Summary: spamc crashes with -B and <6 chars of input
Product: Spamassassin
Version: 3.1.0
Platform: PC
OS/Version: Linux
Status: NEW
Severity: normal
Priority: P5
Component: spamc/spamd
AssignedTo: dev@spamassassin.apache.org
ReportedBy: metaur@telia.com
Hello,
spamc crashes when you use the -B option and give it less than 6 chars of input.
This is caused by some routine that subtracts 6 from an unsigned integer,
without checking if it's bigger than 5.
Here's an example of the crash:
$ perl -e 'print "A\n";' | /usr/bin/spamc -B
Segmentation fault
$
I have a patch for this issue.
// Ulf Harnhammar
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
sidney@sidney.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
Resolution|FIXED |
Target Milestone|Undefined |3.1.2
------- Additional Comments From sidney@sidney.com 2006-04-06 11:36 -------
This was never retargeted for 3.2 but was checked in to trunk after 3.1 was
branched off.
It's a buffer overflow seg fault so it really should be fixed in 3.1.
I'm reopening targeted to 3.1.2, and I'll put together a patch against 3.1
branch of what was finally checked into trunk for this.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From lwilton@earthlink.net 2005-11-27 21:42 -------
As I said, if you are doing a string compare, there are "other ways". In this
case the right one is probably something like
if (memicmp (ptr, "string", sizeof("string")-1) == 0)
Note: memicmp may be the PC flavor of the name, and Unixen may use a different
name for the function.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From metaur@telia.com 2005-11-27 00:29 -------
Created an attachment (id=3275)
--> (http://issues.apache.org/SpamAssassin/attachment.cgi?id=3275&action=view)
Fix for the bug
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From sidney@sidney.com 2005-11-28 05:53 -------
Loren, that's a lot fewer lines of code, but would be slower with the malloc and
free and copying then uppercasing the entire buffer. The buffer can be up to
256K and the string being searched for is likely to be near the beginning.
This code tests at about twice as fast as the existing code. It uses the same
initial search for '\n' that you have, but with memchr because the buffer is not
a c-string. Because we are looking for plain equality of specific ASCII letters,
or-ing with 0x20 can be used and is faster than tolower or using a non-portable
function like memicmp. The search pointer, p, is skipped past characters that we
know are not '\n' before resuming the search.
The existing code is so fast that this discussion probably is not worth having,
even if this is twice as fast :-) My benchmark test of the existing code found
the start of message in t/bsmtp in about a microsecond on my 1.8GHz iMac G5.
unsigned int i, j, p_len;
char prev;
char* p;
p = m->pre = m->raw;
while ((p_len = (m->raw_len - (p - m->raw))) > 6) {
char* q = memchr(p, '\n', p_len - 6);
if (q == NULL) break;
if ((((*(++q))|0x20) == 'd') &&
(((*(q+1))|0x20) == 'a') &&
(((*(q+2))|0x20) == 't') &&
(((*(q+3))|0x20) == 'a')) {
q+=4;
if (*q == '\r') ++q;
if (*(q++) == '\n') {
m->msg = q;
m->pre_len = q - m->raw;
m->msg_len = m->raw_len - m->pre_len;
break;
}
}
p = q+1;
}
Since it does work and is faster, I think I will check it in if nobody objects.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] [REVIEW] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
sidney@sidney.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard|Needs 2 votes for 3.1 branch|Needs 1 vote for 3.1 branch
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From lwilton@earthlink.net 2005-11-28 02:52 -------
Oops, that should be
for ( ; p = strchr (p+1, '\n') && sourcelen-(p-upperbuf) >= keylen; )
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] [REVIEW] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
sidney@sidney.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|spamc crashes with -B and <6|[REVIEW] spamc crashes with
|chars of input |-B and <6 chars of input
Status Whiteboard| |Needs 2 votes for 3.1 branch
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From sidney@sidney.com 2005-11-27 01:03 -------
This doesn't look exploitable, but it isn't very nice. It's the classic mistake
when working with unsigned ints in C. Your patch will work, but I see another
problem where m-raw[i-1] is referenced when i could still be == 0. Also I don't
like using an arithmetic expression to subtract from an unsigned int when it
isn't really necessary, so I'm going to commit a slightly different patch that
cleans up a bit more.
Thanks for catching this.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] [REVIEW] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
felicity@apache.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Status Whiteboard|Needs 1 vote for 3.1 branch |ready for commit
------- Additional Comments From felicity@apache.org 2006-04-06 16:26 -------
+1
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] [REVIEW] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
sidney@sidney.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|REOPENED |RESOLVED
Resolution| |FIXED
Status Whiteboard|ready for commit |
------- Additional Comments From sidney@sidney.com 2006-04-06 20:19 -------
Committed revision 392044.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From felicity@apache.org 2005-11-27 20:37 -------
On Sun, Nov 27, 2005 at 08:31:01PM +0100, bugzilla-daemon@bugzilla.spamassassin.org wrote:
> (tolower(m->raw[i+1]) == 'd') is the cannonical way to handle this sort of
> thing. It is actually a little less efficient than the repeated test you show,
> but it is generally more readable.
I haven't really looked at the code, and my C is fairly rusty,
but is there a reason we couldn't just do something like
"if (strncasecmp(m->raw+i, 'DATA', 4)==0)" or something? It seems like
checking byte-for-byte isn't the right way to go.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From sidney@sidney.com 2005-11-27 22:11 -------
You are all correct that what is there is ugly. I would not object to a patch
that looked better.
I started to write it differently and discovered that a "better" way did not
look all that much better. to_upper is a standard way to do this, but is not
more efficient, since that macro has to work for non-alphabetic characters,
doing more processing than the two comparisons here. A simple or-ing of x'40'
would work, but I bet after compiler optimization is not that much different in
performance, while looking a bit more opaque to a person reading the code. I
would like to do something that uses memicmp or _memicmp, but I'm not sure what
can of worms I would open up regarding cross-platform compatibility. memicmp is
deprecated in Visual Studio C++ 2005, for example, in favor of the ANSI/ISO C++
naming convention _memicmp. I don't know which platforms don't yet have
_memicmp. I don't want to introduce even more hacks for portability.
Note that this section of code is searching for the '\nDATA\n' string which
should appear quite early in the BSMTP formatted message that is passed to
spamc. Compared to passing the entire message over a network or pipe to spamd,
this one-time search for something that will be found quickly is not worth
putting in a lot of effort to optimize. If I get bored, I might do it anyway :-)
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From sidney@sidney.com 2005-11-28 10:38 -------
Heh, heh, yes, when I'm shaving off fractions of a microsecond I sometimes get
stuck in a mode of thinking in terms of pointers and registers and forget to
write like a human :-)
I noticed another potential array out of bounds reference in the code after this
section where it looks for the end of message "\n.\n" marker that could happen
with malformed input, and I realized that the "6" should be bigger to leave room
for a possible "\nDATA\r\n" and for the required closing "\n.\n".
So I've updated using the faster code, following Loren's suggestion to use
subscript syntax, and with a minor correction (q instead of q+1) that fixes a
very obscure edge case.
I also commented out a static const that is not used anywhere that for some
reason generates a compiler warning with gcc on my MacOS X system but not with
Fedora Core 4 gcc.
Committed to trunk revision 349405.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] [REVIEW] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From jm@jmason.org 2006-04-06 15:38 -------
+1
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From lwilton@earthlink.net 2005-11-27 20:31 -------
> glance having code like ((m->raw[i+1] == 'D') || (m->raw[i+1] == 'd')) and so
(tolower(m->raw[i+1]) == 'd') is the cannonical way to handle this sort of
thing. It is actually a little less efficient than the repeated test you show,
but it is generally more readable.
(Or if you are a real old-timer, you may prefer (toupper(m->raw[i+1]) == 'D') ).
Or if you are doing a string compare there are other ways to do this that are
readable and clean.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From sidney@sidney.com 2005-11-27 01:17 -------
> I see another problem where m-raw[i-1] is referenced
> when i could still be == 0
My mistake. I was looking at a partially edited copy in an edit window.
But I still want to clean it a bit rather than just add that test.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From lwilton@earthlink.net 2005-11-28 02:49 -------
Without looking at the code, and assuming this is a once per message thing, or
the source buffer is typically rather small (<10KB, say) I would probably do
something like
int sourcelen = strlen (sourcebuf);
char *upperbuf = malloc (sourcelen+1);
static char key[] = {"\nDATA\n"};
static int keylen = strlen(key);
if (upperbuf)
{
char *p = upperbuf-1;
strcpy (upperbuf, sourcebuf);
strupr (upperbuf);
for ( ; p = strchr (p+1, '\n') && sourcelen-(p-upperbuf) <= keylen; )
{
if (memcmp (p, key, keylen) == 0)
{
// we found the key!
break;
}
}
free (upperbuf);
}
If this is a once-per-line thing, I would keep the upperbuf around for the
length of the message, only expanding it as needed with a realloc now and then.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
------- Additional Comments From lwilton@earthlink.net 2005-11-28 06:29 -------
I'd suggest a slight variation for prettyness:
q++;
if ((q[0]|0x20) == 'd' &&
((q[1]|0x20) == 'a' &&
((q[2]|0x20) == 't' &&
((q[3]|0x20) == 'a') {
Personally I prefer the && in the front of the continuations lined up with
the 'if', but I admit that isn't the general Unix practice.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
sidney@sidney.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #3275 is|0 |1
obsolete| |
------- Additional Comments From sidney@sidney.com 2006-04-06 15:05 -------
Created an attachment (id=3457)
--> (http://issues.apache.org/SpamAssassin/attachment.cgi?id=3457&action=view)
patch against 3.1 branch
Here is the fix as finally applied in the form of a patch against the 3.1
branch. Please vote on this.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
[Bug 4707] spamc crashes with -B and <6 chars of input
Posted by bu...@bugzilla.spamassassin.org.
http://issues.apache.org/SpamAssassin/show_bug.cgi?id=4707
sidney@sidney.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution| |FIXED
------- Additional Comments From sidney@sidney.com 2005-11-27 09:21 -------
Committed revision 349191 to trunk.
Fixed the bug and added a regression test.
I didn't go for the bigger patch I was thinking of... It looks sloppy at first
glance having code like ((m->raw[i+1] == 'D') || (m->raw[i+1] == 'd')) and so
on. I thought about not repeating the arithmetic in the subscripts and not
repeating the array lookup. I even thought about having the loop use a memsrch
to find the next '\n' instead of looping through every character. But the time
spent to find the "\nDATA\n" string near the beginning of a BSMTP message just
doesn't justify making the code any more difficult to read, and it is the
compiler's job to optimize repeated use of expressions like that.
I did apply a somewhat different patch than Ulf proposed. Instead of checking
for a short message and bailing out, I avoided using a subtraction on an
unsigned int where there is danger of the unsigned int underflow that was
causing the seg fault problem.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.