Hi,

ac(8) has 2 separate problems where it can segfault if we pass a bogus file to
read connect time data instead of the default /var/log/wtmp. I've looked at all
other BSDs and since the code is based on the same source it seems they suffer
from the same problem as well, although I didn't test them.

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

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

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

Reply via email to