Bug 297 - [PATCH] Remove bogus sleep(5) calls from libICE
Summary: [PATCH] Remove bogus sleep(5) calls from libICE
Status: RESOLVED DUPLICATE of bug 306
Alias: None
Product: xorg
Classification: Unclassified
Component: Lib/Xlib (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: Egbert Eich
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 351
  Show dependency treegraph
 
Reported: 2004-03-09 18:25 UTC by Mike A. Harris
Modified: 2004-05-05 17:35 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Remove pointless 5 second delays from libICE (2.04 KB, patch)
2004-03-09 18:27 UTC, Mike A. Harris
no flags Details | Splinter Review

Description Mike A. Harris 2004-03-09 18:25:09 UTC
There is no libICE component, so filing against 'xlib' component as that is
the closest match.


Details (from Havoc Pennington in Red Hat bugzilla bug 66751):

It's in the "lib/xtrans" directory in Xlib source code, trans_mkdir, it makes a
directory in /tmp, checks its ownership, if not owned by root it does sleep(5).
Do a default fresh install of 7.2 or 7.3 and log in with a new user account.
Note that it triggers the warning about root and sleeps for 5 seconds, then
continues.
This happens when the GNOME/KDE session managers start up ICE.

This is either a) a real security issue or b) a pointless 5-second login delay.

Here is the exact code from Xtransutil.c:
            if (updateOwner && !updatedOwner) {
                PRMSG(1, "mkdir: Owner of %s should be set to root\n",
                      path, 0, 0);
                sleep(5);
            }
            if (updateMode && !updatedMode) {
                PRMSG(1, "mkdir: Mode of %s should be set to %04o\n",
                      path, mode, 0);
                sleep(5);
            }

So the fix is either to create a setuid helper or something so that libICE can
always set the owner of that dir to root, or just remove the sleep. But the
current situation is just dumb.
Comment 1 Mike A. Harris 2004-03-09 18:26:49 UTC
The libICE as it is, just inserts pointless 5 second delays which do nothing
to improve security in any way, and just irritate users, or cause them to
file bug reports about the problem.

We've patched out these bogus 5 second delays for several XFree86 releases now,
and I've just ported the patch to xorg-x11 sources.

Please apply, or acknowledge and I'll apply.
Comment 2 Mike A. Harris 2004-03-09 18:27:26 UTC
Created attachment 137 [details] [review]
Remove pointless 5 second delays from libICE
Comment 3 Egbert Eich 2004-03-11 12:17:47 UTC
Mike, have you checked the log message of the commit that added these sleeps?

date: 1999/06/12 07:18:29;  author: dawes;  state: Exp;  lines: +65 -4
2727. Increase the MAXSCREENS value from 4 to 16 (Egbert Eich).
2726. Add support for printing out PCI-PCI bridge header information correctly
      (Egbert Eich).
2725. New RAC (Resource Access Control) code (Egbert Eich).
2723. An attempt at allowing the server to correct the /tmp/.X11-unix
      directory when it is possible to do so safely (David Dawes).

It is 2723 that is an issue here. The sleeps seem to attempt to fix a possible 
race here. This issue should at least be investigated before removing them.
Comment 4 Mike A. Harris 2004-03-11 14:15:34 UTC
Yes, I investigated why it was added, and I agree that there is a problem
there, but I disagree with the current non-fix, as sleep(5) does not fix the
problem at all.

Perhaps a better fix would be a setuid/setgid application that can
actually change the permissions and ownership of the directory itself.

I still don't believe "sleep(5)" helps the problem, but it does inconvenience
people with a 5 second delay, which is unacceptable.  I'll revert the fix
if you request, but the current code is definitely broken and bad, and gives
no incentive for the real issue to be addressed.
Comment 5 Mike A. Harris 2004-03-11 14:16:50 UTC
BTW, similar problems exist for /tmp/.font-unix, and Xauthority.  I'm sure
there are others I'm forgetting as well.
Comment 6 Egbert Eich 2004-03-12 02:05:47 UTC
Why does this directory have the wrong owner and permission in the first place?
The changelog message added by David is not all too inclusive, but what I get from
this is that it appears that the login client starts before the server is up and
running and finds the wrong permission on this directory. 
When would the server go and change the permission on this directory? Only when it
sets up its listening socket - that means at startup time. 
So the idea is to add some grace period so that the server has enough time to do
the change.
Comment 7 Egbert Eich 2004-03-12 03:21:08 UTC
Mike, why are you committing this to the release branch without discussing this
issue? I don't think the problem behind this is sufficiently understood. The
sleeps have been in the code since 1999. Therefore I don't think it should be
removed shortly before we do this release.
Comment 8 Jim Gettys 2004-03-12 18:52:53 UTC
These sleeps are really bogus.

The way this can happen is if, for example, you intermix use of xdm/gdm/kdm with
startx.

If started by startx, then the directory can get created by the user; when
started by xdm/gdm/kdm, they may get created by root.

There may be DOS attacks you can perform by messing around with the ownership
and protection of certain key X directories: e.g. .X11-unix, .ICE-unix.

But the most these sleeps can be said to do is to slow down a failure that
should get fixed elsewhere (e.g. in {x,g,k}dm).

Unless there is something else in the system setting the ownership/protection
(which I don't think there is), these can't help prevent any attacks that
could occur otherwise.

So I think removing these sleeps is fine: but we should also make sure we 
generate a bug and fix to the ownership problems.

Keith, is this correct?
Comment 9 Egbert Eich 2004-03-15 05:42:49 UTC
Well, if one looks at the entire patch that went in in 1999 the code that attempts
to change the mode and user was added instead of just failing.
Before that there was only code that was setting the mode right when the
directory is created. However when it existed no attempt was made to set the
mode but the function failed. The user was not verified at all. 
The sleep was added to make the user aware of the log message telling the user
about the security problem. If we were strict we would replace the sleep(5) by a
return 0 - which would restore the original error behavior.
This would mean Xservers that are run by a user and don't have the suid but set
(Xvnc, Xnest, Xvfb) would fail if something else hadn't created the .X11-unix
directory. The same is true for .ICE-unix. Here trans_mkdir always seems to be
called from a user. 

It looks like that the current behavior is less strict than the original if the
mode was wrong. In this case the function should probably not just issue a
warning but fail entirely.
I don't know how the ownership problem should be fixed:
1. Those directories could be created at system installation time,
2.  a small suid root utility could be called to perform just this job (such an
utility might by itself introduce a security problem).
3. We can just do it like now by just issuing a warning. In this case I'd leave
in the sleep(5) as the problem is not just the sleep but the installation which
doesn't provide the application with the permissions required to create a
directory owned by root.
Therefore the sleep itself is not bogus but the fact that we ignore the warning
which is supposed to be emphasized by the sleep.
Comment 10 Jim Gettys 2004-03-15 10:11:10 UTC
Ok, we think we should do the following:

1) remove the sleeps; they never help, and even the messages are never seen by
   typical users.
2) do some investigation of libICE to understand the access issues. If there is
   a real issue beyond denial of service, we should make it exit rather than
   just doing a printf and return.
3) enter another bug to track a proper fix for this bogus code.
4) Open a discussion on the desktop lists about fixing this properly; whether
   via {x,g,k}dm and startx or other solutions.
Comment 11 Mike A. Harris 2004-03-15 10:25:11 UTC
>Ok, we think we should do the following:
>
>1) remove the sleeps; they never help, and even the messages are never seen by
>   typical users.

I agree.  Above, it says the sleeps are there to emphasize the problem, but
I say to that - Emphasize the problem to whom?  As you say here Jim,
end users generally are very unlikely to ever see the messages anyway, and
if they do, there isn't much they can do about it.  They're just inconvenienced
by unnecessary 5 second delays, which is why we removed them in our X
packages over a year or so ago.  ;o)

The only thing the messages do do, is generate additional bug reports, and
cause X to take longer to start, which garners complaints too.  The only
thing the sleep calls would theoretically motivate, would be to motivate
a developer to fix the real problem - but that hasn't happened in 3 years or
so, which defenestrates that idea.


>2) do some investigation of libICE to understand the access issues. If there
>   is a real issue beyond denial of service, we should make it exit rather
>   than just doing a printf and return.

An exit would create a DoS in itself though no?

>3) enter another bug to track a proper fix for this bogus code.

Agreed.

>4) Open a discussion on the desktop lists about fixing this properly; whether
>   via {x,g,k}dm and startx or other solutions.

Yeah, that might be the best way.  In order to do that we probably need to
have someone put together a brief document covering the problem, and it's
history, and why what is there now does not solve the problem, etc. so that
everyone reading it is on the same page, and not having to learn what we
already know about the issue.

No immediate hurry for that though, but we could track the bug that gets
filed to fix this as "mustfix for next release" perhaps.

Thanks for following up.
Comment 12 Egbert Eich 2004-03-24 10:53:35 UTC
I've commented out the sleeps for now but won't close this as it needs to be put
on the table again for security audit.
Therefore I move the dependency onto the next version.
Comment 13 Egbert Eich 2004-05-07 03:35:22 UTC
The security issue gets discussed on 306 now.

*** This bug has been marked as a duplicate of 306 ***


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.