From: Jan Klemkow <jan@openbsd.org>
Subject: Re: newsyslog: fix negative size limit bug
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Tue, 22 Jul 2025 23:57:58 +0200

On Tue, Jul 22, 2025 at 11:06:18PM GMT, Alexander Bluhm wrote:
> On Tue, Jul 22, 2025 at 10:45:56PM +0200, Jan Klemkow wrote:
> > Hi,
> > 
> > If you use a size limit of 2 GByte or more in newsyslog.conf, it went
> > negaive and does not work as expected.  This is caused by using atoi(3)
> > for parsing the size limit from the config file.  Changing this function
> > to strtoll(3) fix this issue.
> > 
> > ok?
> 
> Usually we don't like atoi(3) and replace it with strtonum(3).
> Could you remove all atoi() and implement strtonum() range checking
> and error message?

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	22 Jul 2025 21:54:40 -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,9 +578,16 @@ 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 (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
 			working->size = -1;
 
 		working->flags = 0;