From: Jan Klemkow <jan@openbsd.org>
Subject: Re: newsyslog: fix negative size limit bug
To: Theo de Raadt <deraadt@openbsd.org>
Cc: Alexander Bluhm <bluhm@openbsd.org>, tech@openbsd.org
Date: Wed, 23 Jul 2025 13:41:36 +0200

On Tue, Jul 22, 2025 at 08:35:36PM -0600, Theo de Raadt wrote:
> Jan Klemkow <jan@openbsd.org> wrote:
> > -		if (isdigit((unsigned char)*q))
> > -			working->size = atoi(q) * 1024;
> > -		else
> > +		if (isdigit((unsigned char)*q)) {
> > +			working->size = strtonum(q, 0, INT64_MAX/1024, &errstr);
> > +			if (errstr) {
> > +				warnx("%s:%d: invalid size %s (%s)"
> > +				    " --> skipping", conf, lineno, q, errstr);
> > +				ret = 1;
> > +				goto nextline;
> > +			}
> > +			working->size *= 1024;
> > +		} else
> 
> The (pre-existing) use of isdigit() is pretty strange.
> 
> This is probably handling this:
> 
>                        If this field is replaced by an `*', or set to `0',
>                        then the size of the log file is not taken into account
>                        when determining when to trim the log file.  By
> 
> But basically you could put any garbage in the field, and it will behave
> like '*' or '0'?  That seems a bit imprecise.  It should probably parse for
> '*' specifically, then call strtonum() which would include the 0 case, and
> then anything else is an error.

ok?

Index: newsyslog.c
===================================================================
RCS file: /cvs/src/usr.bin/newsyslog/newsyslog.c,v
diff -u -p -r1.116 newsyslog.c
--- newsyslog.c	8 May 2025 15:30:41 -0000	1.116
+++ newsyslog.c	23 Jul 2025 09:58:49 -0000
@@ -92,6 +92,7 @@
 #include <limits.h>
 #include <pwd.h>
 #include <signal.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -474,6 +475,7 @@ parse_file(struct entrylist *list, int *
 	int lineno = 0;
 	int ret = 0;
 	FILE *f;
+	const char *errstr;
 	long l;
 
 	if (strcmp(conf, "-") == 0)
@@ -513,7 +515,14 @@ nextline:
 			if (*q == '\0') {
 				working->uid = (uid_t)-1;
 			} else if (isnumberstr(q)) {
-				working->uid = atoi(q);
+				working->uid = strtonum(q, 0, UID_MAX, &errstr);
+				if (errstr) {
+					warnx("%s:%d: invalid user %s (%s)"
+					    " --> skipping", conf, lineno, q,
+					    errstr);
+					ret = 1;
+					goto nextline;
+				}
 			} else if (uid_from_user(q, &working->uid) == -1) {
 				warnx("%s:%d: unknown user %s --> skipping",
 				    conf, lineno, q);
@@ -525,7 +534,14 @@ nextline:
 			if (*q == '\0') {
 				working->gid = (gid_t)-1;
 			} else if (isnumberstr(q)) {
-				working->gid = atoi(q);
+				working->gid = strtonum(q, 0, GID_MAX, &errstr);
+				if (errstr) {
+					warnx("%s:%d: invalid group %s (%s)"
+					    " --> skipping", conf, lineno, q,
+					    errstr);
+					ret = 1;
+					goto nextline;
+				}
 			} else if (gid_from_group(q, &working->gid) == -1) {
 				warnx("%s:%d: unknown group %s --> skipping",
 				    conf, lineno, q);
@@ -562,10 +578,18 @@ nextline:
 
 		q = parse = missing_field(sob(++parse), errline, lineno);
 		*(parse = son(parse)) = '\0';
-		if (isdigit((unsigned char)*q))
-			working->size = atoi(q) * 1024;
-		else
+		if (strcmp(q, "*") == 0) {
 			working->size = -1;
+		} else {
+			working->size = strtonum(q, 0, INT64_MAX/1024, &errstr);
+			if (errstr) {
+				warnx("%s:%d: invalid size %s (%s)"
+				    " --> skipping", conf, lineno, q, errstr);
+				ret = 1;
+				goto nextline;
+			}
+			working->size *= 1024;
+		}
 
 		working->flags = 0;
 		q = parse = missing_field(sob(++parse), errline, lineno);