[PATCH] Xext: "xauth generate" with large timeout crashes Xorg #27134 .

[Arvind Umrao]

Keith Packard wrote:
> On Tue, 13 Apr 2010 16:50:30 +0530, Arvind Umrao <Arvind.Umrao at Sun.COM> wrote:
>   
>> On 04/13/10 10:57, Keith Packard wrote:
>>     
>>> On Tue, 13 Apr 2010 10:16:20 +0530, Arvind Umrao<Arvind.Umrao at Sun.COM>  wrote:
>>>
>>>    
>>>       
>>>>        CARD32 maxSecs = (CARD32)(~0) / (CARD32)MILLI_PER_SECOND;
>>>> +    CARD32 nowSec = GetTimeInMillis()/ (CARD32)MILLI_PER_SECOND;
>>>>
>>>> -    if (seconds>   maxSecs)
>>>> -    { /* only come here if we want to wait more than 49 days */
>>>> -	pAuth->secondsRemaining = seconds - maxSecs;
>>>> -	return maxSecs * MILLI_PER_SECOND;
>>>> +    CARD32 maxPossibleSec = maxSecs - nowSec;
>>>>      
>>>>         
>>> What happens when GetTimeInMillis() returns (CARD32)(~0)?
>>>
>>>    
>>>       
>> Yes I agree you found problem in my fix. When GetTimeInMillis() returns 
>> (CARD32)(~0) , execution should go to overflow timeout, not in regular 
>> timeout. I have made the correction please review it and give your 
>> comments again.
>>     
>
> The actual requirement for the milliseconds value is that it not
> fail the comparison below:
>
>     if ((int) (millis - now) <= 0)
>
> This means keeping the milliseconds value within 31 bits of the now
> value. There should be no reason to ever use GetTimeInMillis() to range
> check the incoming value.
>   
Please again, reconfirm should I remove GetTimeInMillis(). If I do not 
keep the milliseconds value within 31 bits of  'now' value, Security 
Authorization will get expired immediately with setting timeout to zero, 
when user supply large value of timeout. 

> In addition, I'd suggest that the assert in SecurityAuthorizationExpired
> be removed; that's a bogus test with pAuth->timer is NULL, as it will be
> when TimerSet is first invoked.
>   
Yes I do agree with you. Assert in SecurityAuthorizationExpired should 
be removed and it is bogus. I will recreate patch and sent it for review.