Integer overflow in syslogd

[Michael Savage]

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").

Mike

[Michael McConville]

Post by Michael Savage
I found an integer overflow in syslogd which can be triggered by
#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 ) );
}
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.

Post by Michael Savage
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.

Post by Michael Savage
+ 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) {

Post by Michael Savage
+ *gt = '\0';
+ possiblepri = strtonum(p + 1, 0, INT_MAX, &errstr);
+ if (!errstr)

if (errstr == NULL)

Post by Michael Savage
+ 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?

Post by Michael Savage
}
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]
}
}

[Michael Savage]

Here's a patch with less fragile parsing code.

Mike

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 12 Feb 2016 20:51:41 -0000
@@ -1324,6 +1324,30 @@ usage(void)
exit(1);
}

+size_t
+parsepriority(const char *msg, int *pri)
+{
+ size_t nlen;
+ char buf[11];
+ const char *errstr;
+ int maybepri;
+
+ if (*msg++ == '<') {
+ nlen = strspn(msg, "1234567890");
+ if (nlen > 0 && nlen < sizeof(buf) && msg[nlen] == '>') {
+ memcpy(buf, msg, nlen);
+ buf[nlen] = '\0';
+ maybepri = strtonum(buf, 0, INT_MAX, &errstr);
+ if (errstr == NULL) {
+ *pri = maybepri;
+ return nlen + 2;
+ }
+ }
+ }
+
+ return 0;
+}
+
/*
* Take a raw input line, decode the message, and print the message
* on the appropriate log files.
@@ -1337,13 +1361,7 @@ printline(char *hname, char *msg)
/* 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;
- }
+ p += parsepriority(p, &pri);
if (pri &~ (LOG_FACMASK|LOG_PRIMASK))
pri = DEFUPRI;

@@ -1374,19 +1392,16 @@ printsys(char *msg)
{
int c, pri, flags;
char *lp, *p, *q, line[MAXLINE + 1];
+ size_t prilen;

(void)snprintf(line, sizeof line, "%s: ", _PATH_UNIX);
lp = line + strlen(line);
for (p = msg; *p != '\0'; ) {
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;
- } else {
+ prilen = parsepriority(p, &pri);
+ p += prilen;
+ if (prilen == 0) {
/* kernel printf's come out on console */
flags |= IGN_CONS;
}

[Michael McConville]

Post by Michael Savage
Here's a patch with less fragile parsing code.

Comments inline.

Post by Michael Savage
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 12 Feb 2016 20:51:41 -0000
@@ -1324,6 +1324,30 @@ usage(void)
exit(1);
}

A one- or two-sentence comment describing the function may be nice, as
it isn't immediately obvious what it does. Not entirely necessary
though.

Post by Michael Savage
+size_t
+parsepriority(const char *msg, int *pri)

Nitpick, but I'd probably slightly prefer parse_priority.

Post by Michael Savage
+{
+ size_t nlen;
+ char buf[11];
+ const char *errstr;
+ int maybepri;
+
+ if (*msg++ == '<') {
+ nlen = strspn(msg, "1234567890");
+ if (nlen > 0 && nlen < sizeof(buf) && msg[nlen] == '>') {
+ memcpy(buf, msg, nlen);
+ buf[nlen] = '\0';

The above two lines should probably be strlcpy(buf, msg, nlen+1).

Post by Michael Savage
+ maybepri = strtonum(buf, 0, INT_MAX, &errstr);
+ if (errstr == NULL) {
+ *pri = maybepri;
+ return nlen + 2;
+ }
+ }
+ }
+
+ return 0;
+}
+
/*
* Take a raw input line, decode the message, and print the message
* on the appropriate log files.
@@ -1337,13 +1361,7 @@ printline(char *hname, char *msg)
/* 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;
- }
+ p += parsepriority(p, &pri);
if (pri &~ (LOG_FACMASK|LOG_PRIMASK))
pri = DEFUPRI;
@@ -1374,19 +1392,16 @@ printsys(char *msg)
{
int c, pri, flags;
char *lp, *p, *q, line[MAXLINE + 1];
+ size_t prilen;
(void)snprintf(line, sizeof line, "%s: ", _PATH_UNIX);
lp = line + strlen(line);
for (p = msg; *p != '\0'; ) {
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;
- } else {
+ prilen = parsepriority(p, &pri);
+ p += prilen;
+ if (prilen == 0) {
/* kernel printf's come out on console */
flags |= IGN_CONS;
}

Looking at the old code again, it seems like the '>' was effectively
optional, and a stray '<' could zero pri. Was this a bug or a feature?

Otherwise, looks good to me. Thanks again!

[Michael Savage]

I've added a comment and replaced memcpy with strlcpy as suggested.

Post by Michael McConville
Nitpick, but I'd probably slightly prefer parse_priority.

Me too, but it gets called by printline/printsys so I copied that. If
anyone has stronger feelings about it I'll change it to whatever.

Post by Michael McConville
Looking at the old code again, it seems like the '>' was effectively
optional, and a stray '<' could zero pri. Was this a bug or a feature?

The old code was pretty loose about what it accepted, and I think the
new behaviour makes more sense. man syslogd has a paragraph about it:

The message sent to syslogd should consist of a single line. The
message can contain a priority code, which should be a preceding decimal
number in angle braces, for example, ``<5>''. This priority code should
map into the priorities defined in the include file <sys/syslog.h>.

Mike


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 12 Feb 2016 22:35:52 -0000
@@ -1325,6 +1325,34 @@ usage(void)
}

/*
+ * Parse a priority code of the form "<123>" into pri, and return the
+ * length of the priority code including the surrounding angle brackets.
+ */
+size_t
+parsepriority(const char *msg, int *pri)
+{
+ size_t nlen;
+ char buf[11];
+ const char *errstr;
+ int maybepri;
+
+ if (*msg++ == '<') {
+ nlen = strspn(msg, "1234567890");
+ if (nlen > 0 && nlen < sizeof(buf) && msg[nlen] == '>') {
+ strlcpy(buf, msg, nlen + 1);
+ maybepri = strtonum(buf, 0, INT_MAX, &errstr);
+ if (errstr == NULL) {
+ *pri = maybepri;
+ return nlen + 2;
+ }
+ }
+ }
+
+ return 0;
+}
+
+/*
* Take a raw input line, decode the message, and print the message
* on the appropriate log files.
*/
@@ -1337,13 +1365,7 @@ printline(char *hname, char *msg)
/* 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;
- }
+ p += parsepriority(p, &pri);
if (pri &~ (LOG_FACMASK|LOG_PRIMASK))
pri = DEFUPRI;

@@ -1374,19 +1396,16 @@ printsys(char *msg)
{
int c, pri, flags;
char *lp, *p, *q, line[MAXLINE + 1];
+ size_t prilen;

(void)snprintf(line, sizeof line, "%s: ", _PATH_UNIX);
lp = line + strlen(line);
for (p = msg; *p != '\0'; ) {
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;
- } else {
+ prilen = parsepriority(p, &pri);
+ p += prilen;
+ if (prilen == 0) {
/* kernel printf's come out on console */
flags |= IGN_CONS;
}

[Alexander Bluhm]

Post by Michael Savage
I've added a comment and replaced memcpy with strlcpy as suggested.

Commited, Thanks

bluhm