Bugzilla Bug 234391

In ssmtp.c the function fd_gets more or less looks like this:

char *fd_gets(char *buf, int size, int fd)
{
        while((i < size) && (fd_getc(fd, &c) == 1)) {
                        buf[i++] = c;
        }
        buf[i] = (char)NULL;
        return(buf);
}

Coming out of the loop, i can be size, causing a 0-byte to be written past the
end of the buffer.

There are also lots of "char c = (char)NULL;" and "char *p = (char)NULL;"
occurrences that may be indicative of careless programming and may warrant a
code review.

I agree on the off-by-one error, but initializing variables to NULL before
using them seems a rather good practice to me, as it allows to find some bugs
more easily. We'll probably have to contact upstream.

Created an attachment (id=162630) [edit]
Proposed misc fixes

In addition to fixing the "(char)NULL" things and the off-by-one, this also
addresses a problem in from_format() that caused a call to strdup on a local
buffer with uninitialized contents.

This last problem was introduced in 2.62 and was the reason I started looking
at the source code in the first place (the From: line wasn't properly
formatted).

I have not looked at the rest of the source, so I'm not claiming this is a
complete solution.

Concerning the potential off-by-one:
There is no off-by-one error if 'int size' is not the buffer size, but the
maximum number of characters the buffer can contain (not counting the NUL).
From my reading of the code, this is what happens:

The only call of that function is here:

int smtp_read(int fd, char *response)
{
        do {
                if(fd_gets(response, BUF_SZ, fd) == NULL) {
...

smtp_read() is called at several places, but all buffers that are passed as
'response' have been allocated as 'BUF_SZ + 1'. Am I missing something?

You're right about the buffer size. 

Now if the strdup is not something that can be abused, then this can be demoted
to a regular ssmtp bug.

Created an attachment (id=163507) [edit]
Updated patch. Removed fix for off-by-one that wasn't an off-by-one.

Updated patch.

Created an attachment (id=165005) [edit]
ssmtp-unitialized-strdup.patch

Just the security-relevant hunk.

Opening to the public, please commit with the patch.

CVE-2008-3962 has been assigned.

2.62-r3 is inCVS.

As Tomas Hoger pointed out, this has been a re-introduction of bug 127592 since
that patch was dropped in the ebuild when 2.62 was bumped.

2.61 is also affected, but we patched it in 2006.

(In reply to comment #10)
> As Tomas Hoger pointed out, this has been a re-introduction of bug 127592 since
> that patch was dropped in the ebuild when 2.62 was bumped.
> 
> 2.61 is also affected, but we patched it in 2006.
> 

only 2.61-r2 and 2.61-r31 where patched, but not -2.61-r30 which i apparantly
used as a base for 2.62, thus the patch got dropped for 2.62. 2.61-r30 was
always p.masked iirc. 

Arches, please test and mark stable:
=mail-mta/ssmtp-2.62-r3
Target keywords : "alpha amd64 arm hppa ia64 m68k ppc ppc64 s390 sh sparc x86"

Stable for HPPA.

amd64/x86 stable

alpha/ia64/sparc stable

ppc and ppc64 stable

It was only stable for a short timeframe and the issue is almost impossible to
exploit. My vote is NO glsa.

no too, and closing.