Re: xlock mishandles malformed .signature/.plan

[tschweik () FIDUCIA DE (tschweik () FIDUCIA DE)]

Jochen Bauer wrote:

> 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.
[snip]


Stating ANSI-C that's wrong. The compiler is allowed to reorder local
variables --- the scenario described above may happen like that, but only
MAY. It depends on the compiler used to create the binaries and
optimizations switched on for this compiler. Additionally some compilers
don't really create a buffer the way you where expecting it: embedded in
between the other variables. Some compilers include code doing malloc for
the space needed at function entry. At function exit the space is freed
again. Overhead is bigger this way, but stack space is preserved.

Results are absolutely unpredictable if unintentionally overwriting a byte
that way.

--
Thomas Schweikle <tschweik () fiducia de>