[PATCH] Xext: "xauth generate" with large timeout crashes Xorg #27134 .
Arvind Umrao
Arvind.Umrao at Sun.COM
Wed Apr 14 23:43:19 PDT 2010
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,
>>
>
More information about the xorg-devel
mailing list