| Summary: | [PATCH] Remove bogus sleep(5) calls from libICE | ||||||
|---|---|---|---|---|---|---|---|
| Product: | xorg | Reporter: | Mike A. Harris <mharris> | ||||
| Component: | Lib/Xlib | Assignee: | Egbert Eich <eich> | ||||
| Status: | RESOLVED DUPLICATE | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | medium | CC: | eich, keithp | ||||
| Version: | unspecified | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| i915 platform: | i915 features: | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 351 | ||||||
| Attachments: |
|
||||||
|
Description
Mike A. Harris
2004-03-09 18:25:09 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. Created attachment 137 [details] [review] Remove pointless 5 second delays from libICE 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.
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. BTW, similar problems exist for /tmp/.font-unix, and Xauthority. I'm sure there are others I'm forgetting as well. 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. 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. 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?
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. 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.
>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. 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. 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.