From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: newsyslog: fix negative size limit bug
To: Jan Klemkow <j.klemkow@wemelug.de>
Cc: Theo de Raadt <deraadt@openbsd.org>, tech@openbsd.org
Date: Wed, 23 Jul 2025 14:28:18 +0200

On Wed, Jul 23, 2025 at 01:41:36PM +0200, Jan Klemkow wrote:
> 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?

OK bluhm@

> 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);