Bug 1029 - Hard failure if socket directories cannot be chowned to root is bad
Summary: Hard failure if socket directories cannot be chowned to root is bad
Alias: None
Product: xorg
Classification: Unclassified
Component: Lib/xtrans (show other bugs)
Version: git
Hardware: All All
: high normal
Assignee: Egbert Eich
QA Contact:
: 1174 (view as bug list)
Depends on:
Blocks: 351
  Show dependency treegraph
Reported: 2004-08-10 17:29 UTC by Torrey T. Lyons
Modified: 2004-09-01 01:24 UTC (History)
7 users (show)

See Also:
i915 platform:
i915 features:

Set XtransFailSoft YES for cygwin (396 bytes, patch)
2004-09-01 10:48 UTC, Alexander Gottwald
no flags Details | Splinter Review
Set XtransFailSoft for Darwin (566 bytes, patch)
2004-09-01 13:14 UTC, Torrey T. Lyons
no flags Details | Splinter Review

Description Torrey T. Lyons 2004-08-10 17:29:17 UTC
The most recent revision to lib/xtrans/Xtransutil.c causes significant problems for non-setuid Xservers 
such as XDarwin. Here is the cvs commit info:

revision 1.4
date: 2004-07-30 21:00:20 +0000;  author: eich;  state: Exp;  lines: +90 -19
2004-07-30  Egbert Eich  <eich@freedesktop.org>

        * lib/xtrans/Xtransutil.c: (trans_mkdir):
        fail hard if socket directories cannot be chowned to root or
        chmod'ed to the requested mode if 'sticky' bit is requested for
        this directory  instead of just print a warning that will remain
        unnoticed most of the times.

And the comment from the file:

	 * 'sticky' bit requested: assume application makes
	 * certain security implications. If effective user ID
	 * is != 0: fail as we may not be able to meet them.

This seems draconian as non-setuid servers can never satisfy this requirement. What real security risks 
make it worth a hard failure? If nothing else this test should be dropped when the Imakefile setting 
InstallXserverSetUID is false.
Comment 1 Kevin E. Martin 2004-08-10 21:17:33 UTC
xfs uses same same Xtransutil.c code and since it is not setuid root, it cannot
chown to root and bails.  Perhaps the code should check to make sure it's
running as root before failing hard.
Comment 2 Kevin E. Martin 2004-08-10 21:25:31 UTC
Ignore my previous comment -- too tired

The relevant piece of info is that xfs uses this code and is bailing since it is
not run setuid root.  Perhaps this change should be backed out or solved in
another way.
Comment 3 Kevin E. Martin 2004-08-11 16:06:17 UTC
On today's release wrangler's call, it was suggested to either:

1. Create a helper program that is setuid root to create the dir, or
2. Revert the changes back to X11R6.7 version

Any thoughts on which way is better?
Comment 4 Torrey T. Lyons 2004-08-11 16:47:56 UTC
Creating a setuid root help program might be the best solution for the long term. However, given the 
time constraints for this release, it seems preferable to revert to the X11R6.7 version. Clients have been 
using this library for a long time with its old behavior.
Comment 5 Kristian Høgsberg 2004-08-13 06:08:07 UTC
Comment from Alan Cox: 

#1 doesn't help on fundamentally non setuid using systems (or AFS)
#2 doesn't fix the problem

A combination of #1 and a .cf configured policy is needed. The suid 
helper itself is pretty trivial

	#define FIXED_PATH	wherever

	int main(int argc, char *argv[])
		exit(chown(FIXED_PATH, 0, 0) ? errno: 0);

(Avoid things like error messages because then you get into all
 the close(1); close(2) type tricks with older systems)

Comment 6 Kevin E. Martin 2004-08-13 13:39:42 UTC
Alan mentions in a followup that the simple setuid root helper is not as trivial
a fix as first thought.  This was discussed on today's release wranglers call
but no decision was made.  The general feeling was that either we revert the
change or we rely on the vendors to create the directories in question at
startup time.

In addition, those file systems/platforms that cannot setuid root still need
another solution if we do not revert this change.

Other suggestions are welcome.  We need to decide on a solution for Monday's
release wranglers call.
Comment 7 Egbert Eich 2004-08-23 09:25:24 UTC
I've metioned the problems for non-SUID programs several times and outlined
possible solutions: Among them where:
1. Create the directories in question at install time with correct ownership/
   permissions. This would be suitalbe for most Linux distros - at least as
   long as the tmp filesystem is persistent.
2. If the fs is not persistent run a boot script that creates those directories
   after the filesystem has been created.
3. Use PAM or whatever other login mechanism that is capable of running scripts
   with root wonership during the login process to create those dirs.
It was suggested to describe possible solutions in the release notes.
I thought about a suid helper program, too, as it would do the same as an SUID
Xserver, but since the directory name is not known beforehand (X11, ICE, xfs use
different names already), we would have to be able to pass the directory name as
a command line argument, and I did not know how to do this in a fully
unexploitable way.
Comment 8 Kevin E. Martin 2004-08-24 21:40:16 UTC
At this point, the consensus appears to be to leave the test in.

What should be done for operating and file systems that do not support setuid?

One possibility would be to have a configuration option that would allow each OS
to choose whether or not it should fail hard.
Comment 9 Kevin E. Martin 2004-08-25 21:41:53 UTC
(In reply to comment #8)
> At this point, the consensus appears to be to leave the test in.
> What should be done for operating and file systems that do not support setuid?
> One possibility would be to have a configuration option that would allow each OS
> to choose whether or not it should fail hard.

We talked about this solution during the release wranglers call today, and
agreed that adding a configuration option would be best.  Could someone who has
a system that installs non-setuid servers please create a patch that would add
the option?  Thanks!
Comment 10 Dave Airlie 2004-08-25 22:34:35 UTC
*** Bug 1174 has been marked as a duplicate of this bug. ***
Comment 11 Dave Airlie 2004-08-25 22:36:56 UTC
I've marked my bug 1174 to this... I've also add mharris to the CC list (as
installing Xorg from CVS on an FC2 will stop X working as the FC2 xfs startup
script rm -rf /tmp/.font-unix at the start which clearly isn't a good idea
anymore.. xfs also -droppriv so it never creates the directory again and the X
server falls over...)
Comment 12 Egbert Eich 2004-08-27 08:22:19 UTC
Assigning this to myself.
Comment 13 Egbert Eich 2004-08-27 12:03:44 UTC
I'm still at loss here:
What should be changed now? 

1. On every system we have binaries that use this code which are non-suid.
   On some systems all binaries may be non-suid, however this problem exists
   There are solutions for those cases that are non-suid so it is not that this
   code would never work on non-suid systems. They just should be documented. 
   The security problem doesn't go away for non-suid binaries, it becomes more
2. Another thing are platforms that don't support the sticky bit. There we have
   no choice but to accept a compromise. I don't think a compromise should be
   made if an individual fs doesn't support the sticky bit while the others on
   the same platform do. The answer here would probably be to not use the fs.

So what should I do? 
a. Allow to accept that the owner and mode cannot be set to what is expected.
b. Allow to accept that the mode cannot be set to what is expected.

Comment 14 Kevin E. Martin 2004-08-27 13:35:04 UTC
For this release (to make it simple), how about a configuration option that
allows people to revert to the 6.7 behavior.  For example, have an option named
something like XtransHardFail, which defaults to YES, but can be overridden in
the host.def file.  If YES, you get the current behavior, and if NO, you get the
6.7 behavior.

We can work on optimizing the failure handling cases after the current release.
Comment 15 Egbert Eich 2004-08-27 15:47:58 UTC
OK, I've added this. Set 
#define XtransFailSoft
in host.def to set the old behavior.
Please test and add to your OS if required.

Please close the ticket when OK.
Comment 16 Alexander Gottwald 2004-08-29 05:33:25 UTC
is this correct? 
#ifdef XtransFailSoft 
# define XtransFailDefine -DFAIL_HARD
# define XtransFailDefine /**/
At least the naming is mixed up.
I'd suggest 
#ifndef XtransFailSoft
# define XtransFailSoft NO
#if XtransFailSoft
# define XtransFailDefine /**/
# define XtransFailDefine -DFAIL_HARD
Comment 17 Egbert Eich 2004-08-30 07:19:31 UTC
Thanks, Alexander. That's what I'm going to commit in a minute.
The options where turned around. 
I shouldn't do these things late a night.
I also agree that YES/NO settings are better than looking at what
is defined - although we have examples of both in the config files.
Comment 18 Egbert Eich 2004-08-30 08:21:26 UTC
Please add the appropriate setting if it differs from the default for your OS
by Aug. 31, 2004.
Comment 19 Kevin E. Martin 2004-09-01 10:24:12 UTC
On today's release wranglers call, we decided to set a deadline for patches of
today at 6pm EDT.  If patches are available by then, are small and
self-contained, they can still be integreated into the release.  Otherwise, we
will move the remaining open bugs to the release notes bug 999 for documentation.

For this bug, there is a solution available currently: Each platform that needs
to disable the hard failure can add a line to their host.def file.

However, if any of those platform maintainers would like to submit a patch to
include the line in their default configuration file, please attach a patch to
this bug.
Comment 20 Alexander Gottwald 2004-09-01 10:48:57 UTC
Created attachment 805 [details] [review]
Set XtransFailSoft YES for cygwin
Comment 21 Torrey T. Lyons 2004-09-01 13:14:29 UTC
Created attachment 807 [details] [review]
Set XtransFailSoft for Darwin

Default to old soft failure on Darwin so default build works out of the box.
Distributors who provide additional mechanisms to create needed directories can
change to hard failure as desired.

Note: The error message still sounds ominous with soft failure even though the
directory is actually created.
Comment 22 Kevin E. Martin 2004-09-01 18:24:43 UTC
Thank you both for the patches.  I have committed them (and changed the cygwin
patch to put an #ifndef around it so that you can override this value in the
host.def file if ever necessary).

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.