On Sat, Aug 18, 2018 at 04:15:09PM +0100, Ricardo Mestre wrote:
> Hi,
>
> [...]
>
> 1) $ ac -w ./ac.c
> Segmentation fault (core dumped)
>
> This one will happen if the structure is big enough to write memory
> out-of-bounds through strlcpy(3) inside update_user(). The easiest way to
> check
> something is really wrong is to just check if secs is < 0 which should never
> happen. Maybe also adding || secs > LLONG_MAX as well wouldn't hurt? If we
> also
> use -p sometimes we can even see part of the contents of that file.
>
> $ ac -p -w ./ac.c
> opyright (c) 1994 Christopher G. -1360794071379245.75
> etain the above copyright
> * -1441250938215251.25
> user_list *Users = NULL;
> static -1653128621001371.00
> -") == 0)
> fp = stdin;
> else if -1655483745007375.25
> login time for 24hr period in de -1681363511581523.25
> total 2456170264876095.00
>
> With the diff this will be displayed instead:
>
> $ obj/ac -w ./ac.c
> ac: broken record. secs is -6052908641693484365
Nice find.
I think it would be better to replace the strlcpy with a
strncpy(3) and a comment explaining why, as the use of
strlcpy is the root cause of the segfault. name isn't a
C string so we shouldn't use strlcpy here.
There's a similar bug in ac():
447 default:
448 /*
449 * if they came in on a pseudo-tty, then it is
only
450 * a login session if the ut_host field is
non-empty
451 */
452 if (*usr.ut_name) {
453 if (strncmp(usr.ut_line, "tty", 3) != 0
||
454 strchr("pqrstuvwxyzPQRST",
usr.ut_line[3]) != NULL ||
455 *usr.ut_host != '\0')
456 head = log_in(head, &usr);
Technically that strchr should be a memchr, I think, as
usr.ut_line is similarly not guaranteed to be NUL-terminated.
> 2) $ ac -w ./ac.c -d
> Segmentation fault (core dumped)
>
> This will make it segfault on a different part inside of ac() function because
> of assigning a variable or checking its value with a null dereference. This
> happens because localtime(3) was given a bogus input too large and therefore
> returns NULL so we have to check that value before proceeding further.
>
> After applying the diff this is what will be displayed:
>
> $ obj/ac -w ./ac.c -d
> ac: localtime: Value too large to be stored in data type
This part of your diff is ok cheloha@.
Because the changes to address the two segfaults are distinct they
should go in separately.
> Comments? OK?
>
> Index: ac.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ac/ac.c,v
> retrieving revision 1.25
> diff -u -p -u -r1.25 ac.c
> --- ac.c 26 Apr 2018 12:42:51 -0000 1.25
> +++ ac.c 18 Aug 2018 14:41:04 -0000
> @@ -171,6 +171,9 @@ update_user(struct user_list *head, char
> {
> struct user_list *up;
>
> + if (secs < 0)
> + errx(1, "broken record. secs is %lld", secs);
> +
> for (up = head; up != NULL; up = up->next) {
> if (strncmp(up->name, name, sizeof (up->name) - 1) == 0) {
> up->secs += secs;
> @@ -410,6 +413,8 @@ ac(FILE *fp)
> prev = usr.ut_time;
> if (Flags & AC_D) {
> ltm = localtime(&usr.ut_time);
> + if (ltm == NULL)
> + err(1, "localtime");
> if (day >= 0 && day != ltm->tm_yday) {
> day = ltm->tm_yday;
> /*
> @@ -461,6 +466,8 @@ ac(FILE *fp)
>
> if (Flags & AC_D) {
> ltm = localtime(&usr.ut_time);
> + if (ltm == NULL)
> + err(1, "localtime");
> if (day >= 0 && day != ltm->tm_yday) {
> /*
> * print yesterday's total
>
