xlock mishandles malformed .signature/.plan

[aaron () UG CS DAL CA (Aaron Campbell)]

I found a bug last night in xlock that I felt should be known.

xlock iteratively searches for .xlocktext, .plan, and then a .signature
file in the invoker's home directory, and upon finding one, opens it for
reading. The problem is here, in xlock/xlock.c, under the read_plan()
function:

static void
read_plan()
{
        FILE       *planf = NULL;
        char        buf[121];
        char       *home = getenv("HOME");
        char       *buffer;
        int         i, j, cr;
[...]
                planf = my_fopen(buffer, "r");
        }
        if (planf != NULL) {
                for (i = 0; i < TEXTLINES; i++) {
                        if (fgets(buf, 120, planf)) {
                                cr = strlen(buf) - 1;
[...]
                                buf[cr] = '\0';
[...]

If we generate a poisonous .signature file, for example, containing at
least one line that begins with a NULL byte, fgets() will evaluate to true
but strlen(buf) will return 0 and cr will point outside of buf, obviously
bad.

It's hard to tell how serious this is, but I'm sure it could be harmful in
some situations/environments. At any rate, a bug that should definitely be
fixed, especially since xlock is normally set-user-ID root.

The maintainer of xlockmore has been notified. Below is a patch from Todd
Miller that has already been applied to OpenBSD -current. Please save this
then edit instead of copy/paste, a couple of lines are longer than 80
columns.

And of course, if it doesn't apply, try the -l flag to patch(1) in case the
whitespace messed up somewhere along the way before you e-mail me to
complain it doesn't work.

  .  _  _  _ _ . .   _ _ .  . _  _  _ . .
 :  |-||-||<|_||\|  |_|-||\/||-'|->|_-|_|_  DalTech, Halifax, NS, Canada
  `---------------------------------------- [http://www.biodome.org/~fx] -


--- xlock.c.orig        Wed Nov  4 20:33:47 1998
+++ xlock.c     Wed Nov  4 20:34:28 1998
@@ -2524,7 +2524,7 @@
        char        buf[121];
        char       *home = getenv("HOME");
        char       *buffer;
-       int         i, j, cr;
+       int         i, j, len;

        if (!home)
                home = "";
@@ -2587,13 +2587,12 @@
        }
        if (planf != NULL) {
                for (i = 0; i < TEXTLINES; i++) {
-                       if (fgets(buf, 120, planf)) {
-                               cr = strlen(buf) - 1;
-                               if (buf[cr] == '\n') {
-                                       buf[cr] = '\0';
+                       if (fgets(buf, 120, planf) && (len = strlen(buf)) > 0) {
+                               if (buf[len - 1] == '\n') {
+                                       buf[--len] = '\0';
                                }
                                /* this expands tabs to 8 spaces */
-                               for (j = 0; j < cr; j++) {
+                               for (j = 0; j < len; j++) {
                                        if (buf[j] == '\t') {
                                                int         k, tab = 8 - (j % 8);

@@ -2603,12 +2602,11 @@
                                                for (k = j; k < j + tab; k++) {
                                                        buf[k] = ' ';
                                                }
-                                               cr += tab;
-                                               if (cr > 120)
-                                                       cr = 120;
+                                               len += tab;
+                                               if (len > 120)
+                                                       len = 120;
                                        }
                                }
-                               buf[cr] = '\0';

                                plantext[i] = (char *) malloc(strlen(buf) + 1);
                                (void) strcpy(plantext[i], buf);