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

[Keith Packard]

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,

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20100413/70a6b177/attachment-0001.pgp>