followup for libX11 XKB security fixes

[Daniel Stone]

On 23 May 2013 19:33, Alan Coopersmith <alan.coopersmith at oracle.com> wrote:
> I'd originally coded all the checks for xkb->max_key_code, then in testing
> found that because Xlib accesses the xkb->max_key_code'th entry of the
> array,
> it needed to be xkb->max_key_code + 1, and I updated the checks - I guess I
> missed those two.   Sorry about that.

Seems right to me too.

Acked-by: Daniel Stone <daniel at fooishbar.org>

> Reviewed-by: Alan Coopersmith <alan.coopersmith at oracle.com>
>
>
> On 05/23/13 10:26 AM, Julien Cristau wrote:
>>
>> Hi,
>>
>> I noticed some inconsistencies in the XKB security changes in Xlib.
>> Resending to the public list now that the embargo is lifted.
>>
>> Here's another change I think is necessary:
>>
>> diff --git a/src/xkb/XKBGetMap.c b/src/xkb/XKBGetMap.c
>> index 0875dfd..c73e655 100644
>> --- a/src/xkb/XKBGetMap.c
>> +++ b/src/xkb/XKBGetMap.c
>> @@ -426,7 +426,7 @@ XkbServerMapPtr             srv;
>>
>>       if ( rep->totalVModMapKeys>0 ) {
>>         if (((int) rep->firstVModMapKey + rep->nVModMapKeys)
>> -            > xkb->max_key_code)
>> +            > xkb->max_key_code + 1)
>>             return BadLength;
>>         if (((xkb->server==NULL)||(xkb->server->vmodmap==NULL))&&
>>             (XkbAllocServerMap(xkb,XkbVirtualModMapMask,0)!=Success)) {
>> diff --git a/src/xkb/XKBNames.c b/src/xkb/XKBNames.c
>> index 6faef02..6df7406 100644
>> --- a/src/xkb/XKBNames.c
>> +++ b/src/xkb/XKBNames.c
>> @@ -180,7 +180,7 @@ _XkbReadGetNamesReply(      Display *
>> dpy,
>>             nKeys= xkb->max_key_code+1;
>>             names->keys= _XkbTypedCalloc(nKeys,XkbKeyNameRec);
>>         }
>> -       else if ( ((int)rep->firstKey + rep->nKeys) > xkb->max_key_code)
>> +       if ( ((int)rep->firstKey + rep->nKeys) > xkb->max_key_code + 1)
>>             goto BAILOUT;
>>         if (names->keys!=NULL) {
>>             if (!_XkbCopyFromReadBuffer(&buf,
>>
>> This makes the checks against max_key_code consistent, and means the
>> GetNames reply gets checked even when names->keys was just allocated.
>> Running 'xset q' here under xtrace shows GetNames returning firstKey=8
>> nKeys=248, which would trip the original check with maxKeyCode=255.
>> GetMap seems to return firstVModMapKey=0 nVModMapKeys=0
>> totalVModMapKeys=0 though, so I don't know how to trigger the first
>> check.
>>
>> Cheers,
>> Julien
>>
>
>
> --
>         -Alan Coopersmith-              alan.coopersmith at oracle.com
>          Oracle Solaris Engineering - http://blogs.oracle.com/alanc
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel