[hylafax-devel] Re: Security patches for hylafax-v4.0pl2

[Dmitry Bely <dbely@mail.ru>]

John Holland <john@zoner.org> writes:

> I had a bit of time last weekend, so in the interest of patching the setuid 
> uucp buffer overflow in faxalter and getting the FreeBSD/OpenBSD port 
> current, I did a beginnings of a security audit on hylafax-v4.0pl2.  Below 
> are patches for various unbounded string copies in the source.
> 
> The functions that I checked are:
> 
> strcpy
> strcat
> sprintf
> vsprintf
> 
> There were a few instances of possible unbounded character string copies 
> that I did not alter.  The fix was non-obvious and buried deep in support 
> functions.  I also did not check any possible race conditions.
> 
> What is the status of hylafax-v4.0pl2?  Is any work being done on it?  Is 
> another patch level forthcoming?  Or is the effort being targeted to 
> 4.1beta?  Would my time be better spent looking at the latter?
> 
> 
> 
> 
> 
> diff -cr ./faxalter/faxalter.c++ 
> /usr/ports/comms/hylafax_new/hylafax-v4.0pl2/faxalter/faxalter.c++
> *** ./faxalter/faxalter.c++	Sat Feb 14 05:48:38 1998
> --- /usr/ports/comms/hylafax_new/hylafax-v4.0pl2/faxalter/faxalter.c++	Fri 
> Jun  9 15:01:00 2000
> ***************
> *** 185,191 ****
>        va_list ap;
>        va_start(ap, fmt0);
>        char fmt[1024];
> !     sprintf(fmt, "%s %s\n", groups ? "JGPARM" : "JPARM", fmt0);
>        script.append(fxStr::vformat(fmt, ap));
>        va_end(ap);
>    }
> --- 185,191 ----
>        va_list ap;
>        va_start(ap, fmt0);
>        char fmt[1024];
> !     snprintf(fmt, 1024, "%s %s\n", groups ? "JGPARM" : "JPARM", fmt0);

Frankly speaking, I don't like such fixes. First, snprintf-class functions
are non-standard (they are GNU extensions), and so may not be present in
any system. And second (more important) -- why use this ancient C-style
string manipulation functions with endless buffer overruns in the modern
C++ software? Better rewrite the code (not so much work) to use fxStr (or
standard string class which is preferable) instead.

e.g., the simplest (but dirty, because it  still use snprintf) solution:

util/Str.c++:

fxStr
fxStr::format(const char* fmt ...)
{
    char buf[4096];
    va_list ap;
    va_start(ap, fmt);
-    vsprintf(buf, fmt, ap);
+    vsnprintf(buf, sizeof(buf), fmt, ap);
    va_end(ap);
    return fxStr(buf);
}

fxStr
fxStr::vformat(const char* fmt, va_list ap)
{
    char buf[4096];
-    vsprintf(buf, fmt, ap);
+    vsnprintf(buf, sizeof(buf), fmt, ap);
    return fxStr(buf);
}

...

ClassModem.c++:


CallStatus
ClassModem::dial(const char* number, fxStr& emsg)
{
    protoTrace("DIAL %s", number);
-    char buf[256];
-    sprintf(buf, (const char*) conf.dialCmd, number);
+    fxStr buf = fxStr::format( (const char*) conf.dialCmd, number );
    emsg = "";
    CallStatus cs = (atCmd(buf, AT_NOTHING) ? dialResponse(emsg) : FAILURE);
    if (cs != OK && emsg == "")
	emsg = callStatus[cs];
    return (cs);
}

(BTW, still *dangerous*, because format string here is read from
config.modem. It will definitely crash if you specify there something like
ModemDialCmd: ATDT%s%s%s%s%s%s)

Hope to hear from you soon,
Dmitry




____________________ HylaFAX(tm) Developers Mailing List ____________________
 To unsub: mail -s unsubscribe hylafax-devel-request@hylafax.org < /dev/null