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

[Arvind Umrao]

Second thought, after some more testing.  It seems your fixes are not 
better than mine. When epoch time is GetTimeInMillis() -  
(CARD32)(MAXINT),  ie  Sun Jan 10 2038 11:09:28 GMT+0530 (IST), security 
authorization will expire with timeout reset to Zero.

I thing we should start using CARD64 for storing millisecs.  What you 
suggest?

Thanks and Regards
-Arvind

On 04/15/10 10:32, Arvind Umrao wrote:
> 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,
>>
>