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

[Arvind Umrao]

Your fixes are better than mine. I have tested your fixes in SPARC and 
x86 machine with all the possible range of timeout values.

Thanks and Regards
-Arvind



On 04/14/10 08:08, Keith Packard wrote:
> On Wed, 14 Apr 2010 07:22:24 +0530, Arvind Umrao<Arvind.Umrao at Sun.COM>  wrote:
>    
>> 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.
>>      
> Go look at TimerSet again. For relative timers, it does the following:
>
>     timer->delta = millis;
>     millis += now;
>
>     if ((int) (millis - now)<= 0) {
>        ...
>
> All you have to do is ensure that 'millis' is less than MAXINT as
> TimerSet adds 'now' and then subtracts it back off before comparing with
> 0.
>
> The only change needed here was to change the ~0 to MAXINT, plus the
> removal of the assert.
>
> I think this patch will suffice (it compiles, but I haven't run it to check):
>
> diff --git a/Xext/security.c b/Xext/security.c
> index af8d205..e81e560 100644
> --- a/Xext/security.c
> +++ b/Xext/security.c
> @@ -279,10 +279,10 @@ SecurityComputeAuthorizationTimeout(
>       /* maxSecs is the number of full seconds that can be expressed in
>        * 32 bits worth of milliseconds
>        */
> -    CARD32 maxSecs = (CARD32)(~0) / (CARD32)MILLI_PER_SECOND;
> +    CARD32 maxSecs = (CARD32)(MAXINT) / (CARD32)MILLI_PER_SECOND;
>
>       if (seconds>  maxSecs)
> -    { /* only come here if we want to wait more than 49 days */
> +    { /* only come here if we want to wait more than 24 days */
>   	pAuth->secondsRemaining = seconds - maxSecs;
>   	return maxSecs * MILLI_PER_SECOND;
>       }
> @@ -320,8 +320,6 @@ SecurityAuthorizationExpired(
>   {
>       SecurityAuthorizationPtr pAuth = (SecurityAuthorizationPtr)pval;
>
> -    assert(pAuth->timer == timer);
> -
>       if (pAuth->secondsRemaining)
>       {
>   	return SecurityComputeAuthorizationTimeout(pAuth,
>
>