Bug 7304 - spamc: string comparison in option parsing can read out of bounds
Summary: spamc: string comparison in option parsing can read out of bounds
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamc/spamd (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: PC Linux
: P2 normal
Target Milestone: 3.4.2
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-28 14:22 UTC by Hanno Boeck
Modified: 2017-08-07 12:47 UTC (History)
1 user (show)


Attachment Type Modified Status Actions Submitter/CLA Status
patch to replace memcmp with strncmp patch None Hanno Boeck [NoCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Hanno Boeck 2016-02-28 14:22:46 UTC 
Created attachment 5380 [details]
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.
Comment 1 Karsten Bräckelmann 2017-08-06 23:14:35 UTC 
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.
Comment 2 Hanno Boeck 2017-08-07 07:19:09 UTC 
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.
Comment 3 Hanno Boeck 2017-08-07 07:23:40 UTC 
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.
Comment 4 Karsten Bräckelmann 2017-08-07 12:47:59 UTC 
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.