1 From 7bddc2ba16a2a15773c2ea8947059afa27727764 Mon Sep 17 00:00:00 2001

     2 From: Alan Coopersmith <[email protected]>

     3 Date: Mon, 16 Sep 2013 21:47:16 -0700

     4 Subject: [PATCH] Avoid use-after-free in dix/dixfonts.c: doImageText()

     5  [CVE-2013-4396]

     6 

     7 Save a pointer to the passed in closure structure before copying it

     8 and overwriting the *c pointer to point to our copy instead of the

     9 original.  If we hit an error, once we free(c), reset c to point to

    10 the original structure before jumping to the cleanup code that

    11 references *c.

    12 

    13 Since one of the errors being checked for is whether the server was

    14 able to malloc(c->nChars * itemSize), the client can potentially pass

    15 a number of characters chosen to cause the malloc to fail and the

    16 error path to be taken, resulting in the read from freed memory.

    17 

    18 Since the memory is accessed almost immediately afterwards, and the

    19 X server is mostly single threaded, the odds of the free memory having

    20 invalid contents are low with most malloc implementations when not using

    21 memory debugging features, but some allocators will definitely overwrite

    22 the memory there, leading to a likely crash.

    23 

    24 Reported-by: Pedro Ribeiro <[email protected]>

    25 Signed-off-by: Alan Coopersmith <[email protected]>

    26 Reviewed-by: Julien Cristau <[email protected]>

    27 ---

    28  dix/dixfonts.c |    5 +++++

    29  1 file changed, 5 insertions(+)

    30 

    31 diff --git a/dix/dixfonts.c b/dix/dixfonts.c

    32 index feb765d..2e34d37 100644

    33 --- a/dix/dixfonts.c

    34 +++ b/dix/dixfonts.c

    35 @@ -1425,6 +1425,7 @@ doImageText(ClientPtr client, ITclosurePtr c)

    36              GC *pGC;

    37              unsigned char *data;

    38              ITclosurePtr new_closure;

    39 +            ITclosurePtr old_closure;

    40  

    41              /* We're putting the client to sleep.  We need to

    42                 save some state.  Similar problem to that handled

    43 @@ -1436,12 +1437,14 @@ doImageText(ClientPtr client, ITclosurePtr c)

    44                  err = BadAlloc;

    45                  goto bail;

    46              }

    47 +            old_closure = c;

    48              *new_closure = *c;

    49              c = new_closure;

    50  

    51              data = malloc(c->nChars * itemSize);

    52              if (!data) {

    53                  free(c);

    54 +                c = old_closure;

    55                  err = BadAlloc;

    56                  goto bail;

    57              }

    58 @@ -1452,6 +1455,7 @@ doImageText(ClientPtr client, ITclosurePtr c)

    59              if (!pGC) {

    60                  free(c->data);

    61                  free(c);

    62 +                c = old_closure;

    63                  err = BadAlloc;

    64                  goto bail;

    65              }

    66 @@ -1464,6 +1468,7 @@ doImageText(ClientPtr client, ITclosurePtr c)

    67                  FreeScratchGC(pGC);

    68                  free(c->data);

    69                  free(c);

    70 +                c = old_closure;

    71                  err = BadAlloc;

    72                  goto bail;

    73              }

    74 -- 

    75 1.7.9.2

    76