Michael Savage wrote:
> I found an integer overflow in syslogd which can be triggered by
> compiling and running:
>
> #include <err.h>
> #include <string.h>
> #include <sys/types.h>
>
> int main( int argc, char ** argv ) {
> const char * msg = "<999999999999> hello";
> return sendsyslog( msg, strlen( msg ) );
> }
>
>
> The problematic code is a hand-rolled int parser in printline/printsys:
>
> pri = 0;
> while (isdigit((unsigned char)*++p))
> pri = 10 * pri + (*p - '0');
> if (*p == '>')
> ++p;
>
>
> I've attached a patch which converts the loop to strtonum, but doesn't
> exactly mirror the behaviour of the old code in funny cases, like
> sendsyslog("<1no closing angle bracket").
Thanks for this.
A few comments, some trivial:
> Index: syslogd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.177
> diff -u -p -r1.177 syslogd.c
> --- syslogd.c 20 Jul 2015 19:49:33 -0000 1.177
> +++ syslogd.c 11 Feb 2016 20:31:55 -0000
> @@ -1331,18 +1331,23 @@ usage(void)
> void
> printline(char *hname, char *msg)
> {
> - int pri;
> - char *p, *q, line[MAXLINE + 4 + 1]; /* message, encoding, NUL */
> + int pri, possiblepri;
A shorter name would be nice. pos_pri, or something like that.
> + char *p, *q, line[MAXLINE + 4 + 1], *gt; /* message, encoding, NUL */
> + const char *errstr;
>
> /* test for special codes */
> pri = DEFUPRI;
> p = msg;
> if (*p == '<') {
> - pri = 0;
> - while (isdigit((unsigned char)*++p))
> - pri = 10 * pri + (*p - '0');
> - if (*p == '>')
> - ++p;
> + gt = strchr(p, '>');
> + if (gt) {
if (gt != NULL) {
> + *gt = '\0';
> + possiblepri = strtonum(p + 1, 0, INT_MAX, &errstr);
> + if (!errstr)
if (errstr == NULL)
> + pri = possiblepri;
> + *gt = '>';
> + p = gt + 1;
> + }
This block modifies msg. It seems that the existing code doesn't. Are
you sure that won't have consequences elsewhere?
> }
> if (pri &~ (LOG_FACMASK|LOG_PRIMASK))
> pri = DEFUPRI;
> @@ -1372,8 +1377,9 @@ printline(char *hname, char *msg)
> void
> printsys(char *msg)
> {
> - int c, pri, flags;
> - char *lp, *p, *q, line[MAXLINE + 1];
> + int c, pri, flags, possiblepri;
> + char *lp, *p, *q, line[MAXLINE + 1], *gt;
> + const char *errstr;
>
> (void)snprintf(line, sizeof line, "%s: ", _PATH_UNIX);
> lp = line + strlen(line);
> @@ -1381,11 +1387,15 @@ printsys(char *msg)
> flags = SYNC_FILE | ADDDATE; /* fsync file after write */
> pri = DEFSPRI;
> if (*p == '<') {
> - pri = 0;
> - while (isdigit((unsigned char)*++p))
> - pri = 10 * pri + (*p - '0');
> - if (*p == '>')
> - ++p;
> + gt = strchr(p, '>');
> + if (gt) {
> + *gt = '\0';
> + possiblepri = strtonum(p + 1, 0, INT_MAX,
> &errstr);
> + if (!errstr)
> + pri = possiblepri;
> + *gt = '>';
> + p = gt + 1;
> + }
Same as above. Also, maybe this should be broken out into a helper
function?
Maybe more robust logic would look something like this:
size_t nlen;
char nstr[11];
if (*p++ == '<') {
nlen = strspn(p, "0123456789")
if (nlen > 0 && nlen < 11 && p[nlen] == '>') {
strlcpy(nstr, p, sizeof(nstr));
[strtonum logic here]
}
}
