Re: xlock mishandles malformed .signature/.plan

[jtb () THEO2 PHYSIK UNI-STUTTGART DE (Jochen Thomas Bauer)]

Aaron Campbell wrote:

> 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.

I think it is not too serious, as only the following thing will happen:
(well, at least on an Intel x86)

The local variable "char *home", that will be filled with a pointer to a string
containing the path to the user's home directory, is declared right after the
local varibale "char buf[121]", so this pointer will be located right above the
space left for "char buf[121]" on the stack (remember, the stack grows to
lower addresses on an Intel x86). This means that if fgets returns NULL and
therefore c==-1, buf[cr] = '\0' will overwrite the most significant byte
of the pointer "char *home" with NULL (the least significant byte if you are on a
big endian machine). However, if you take a closer look at the
code you will see that

1.)
                            cr = strlen(buf) - 1;
                                if (buf[cr] == '\n') {
                                        buf[cr] = '\0';
                                }
So buf[-1] must have a value of buf[-1]=10 in order to get overwritten by NULL.

2.)
The pointer "char *home" has at this point already been used to construct the
full path to the .plan, .signature or .xlocktext file, and the result has already
been written to a buffer pointed to by "char *buffer".

So, IMHO, there is no security hole created by this bug.


Jochen Bauer
Institute for Theoretical Physics
University of Stuttgart, Germany