[Bug 7304] New: spamc: string comparison in option parsing can read out of bounds

[bu...@bugzilla.spamassassin.org]

https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7304

            Bug ID: 7304
           Summary: spamc: string comparison in option parsing can read
                    out of bounds
           Product: Spamassassin
           Version: SVN Trunk (Latest Devel Version)
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: spamc/spamd
          Assignee: dev@spamassassin.apache.org
          Reporter: hanno@hboeck.de

Created attachment 5380
  --> https://bz.apache.org/SpamAssassin/attachment.cgi?id=5380&action=edit
patch to replace memcmp with strncmp

The function spamc_getopt_long() can read out of bounds in certain situations.
This can be tested by compiling spamc with address sanitizer
(-fsanitize=address in CFLAGS/LDFLAGS) and running e.g.
/spamc --xxxxx
(every long opt with 5 chars or more will trigger it)

The source of the error is this code:
         if((memcmp(longopt+2, longopts[i].name, longoptlen)) == 0) {

The problem here is that memcmp always reads all bytes in both variables and
thus if longoptlen is more than the length of longopts[i].name it will read
invalid memory.

The fix is simple: use strncmp instead of memcmp. See attached patch, please
apply.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7304] spamc: string comparison in option parsing can read out of bounds

[bu...@bugzilla.spamassassin.org]

https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7304

Hanno Boeck <ha...@hboeck.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hanno@hboeck.de

--- Comment #2 from Hanno Boeck <ha...@hboeck.de> ---
You're making specific assumptions about the implementation of memcmp, however
there's nothing in the C standard that allows you to make such assumptions. The
comparison could be implemented in any way, e.g. by comparing chunks or
starting the comparison at the end of the buffer.

strncmp does exactly what you want - it ends reading the buffer at the first
zero byte.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7304] spamc: string comparison in option parsing can read out of bounds

[bu...@bugzilla.spamassassin.org]

https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7304

--- Comment #1 from Karsten Bräckelmann <gu...@rudersport.de> ---
Thanks Hanno for the report. However, I believe the described issue to not be a
problem.

First of all, the memcmp function does not "read all bytes of both variables",
but compares the source strings' raw memory byte-for-byte, up until the first
mismatching byte or the maximum byte number is reached.

The spamc.c longoptions[] name members are null terminated C strings. In the
case of a command-line argument longer than the compared-to longopts[i].name
string, that null char will definitely terminate memcmp, preventing
out-of-bounds operation. (With no earlier character mismatch, that null char is
guaranteed to mis-match because null cannot be included in the command-line
argument.)

I believe this to be not a bug. Not closing just yet, though, open for
additional feedback.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7304] spamc: string comparison in option parsing can read out of bounds

[bu...@bugzilla.spamassassin.org]

https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7304

Mark Martinec <Ma...@ijs.si> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|Undefined                   |3.4.2

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7304] spamc: string comparison in option parsing can read out of bounds

[bu...@bugzilla.spamassassin.org]

https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7304

--- Comment #3 from Hanno Boeck <ha...@hboeck.de> ---
Here's a detailed blogpost about this type of issue:
http://trust-in-soft.com/memcmp-requires-pointers-to-fully-valid-buffers/

Ultimately it doesn't hurt to fix it and it improves testability, as ASAN
complains about these types of errors.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7304] spamc: string comparison in option parsing can read out of bounds

[bu...@bugzilla.spamassassin.org]

https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7304

Karsten Bräckelmann <gu...@rudersport.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Karsten Bräckelmann <gu...@rudersport.de> ---
OK, that blog post shows that memcmp under some very specific conditions and
implementation can in fact read past the differing character, accessing
out-of-bounds memory.

It does, however, *not* show that strncmp is immune to this.

FWIW, does the definition of strncmp include that specification detail that
memchr features but memcmp does not? I don't have a copy of the various C
standards handy...


(In reply to Hanno Boeck from comment #3)
> Ultimately it doesn't hurt to fix it and it improves testability, as ASAN
> complains about these types of errors.

Granted.

Committed to trunk and stable 3.4 branch.

Sending        spamc/getopt.c
Committed revision 1804326.

Sending        spamc/getopt.c
Committed revision 1804327.

Thanks again for the report, Hanno. Closing RESOLVED FIXED.

-- 
You are receiving this mail because:
You are the assignee for the bug.