Bug 632737 - remove Xorg setuid bit
remove Xorg setuid bit
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE 11.4
Classification: openSUSE
Component: X.Org
Factory
Other Other
: P5 - None : Normal (vote)
: ---
Assigned To: Stefan Dirsch
E-mail List
maint:released:11.4:40494
:
Depends on:
Blocks: 683822
  Show dependency treegraph
 
Reported: 2010-08-19 09:28 UTC by Ludwig Nussel
Modified: 2016-04-15 12:59 UTC (History)
9 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ludwig Nussel 2010-08-19 09:28:01 UTC
Time to re-evaluate the need for a setuid bit on /usr/bin/Xorg.
It's needed for starting X as unprivileged user, e.g. via startx. That method is deprecated in favor of a display manager since years. Also modern environments rely on device ACLs and polkit privileges which in turn depend on consolekit tracking the active console. That doesn't work with startx anyways. So the setuid bit is of limited use by default anyways.
No setuid bit also prevents exploitation of the kernel-heap-stack overflow problem via X as X cannot be started in a user controlled environment then.
Therefore I'd like to remove the setuid bit on Xorg for 11.4 from /etc/permissions.easy (no packaging change in X needed). Those who really need it can still set it again in permissions.local.
Any objections or concerns?
Comment 1 Stefan Dirsch 2010-08-19 10:04:03 UTC
No real objections from my side, but we definitely need a section in release notes for that change. In addition I would like to have this discussed on opensuse-factory@opensuse.org first. That way we would also be able to figure out
how many users are still using startx. I'm afraid there are more than you think.

Next one to ask:Egbert.
Comment 2 Egbert Eich 2010-08-19 10:25:40 UTC
(In reply to comment #0)
> No setuid bit also prevents exploitation of the kernel-heap-stack overflow
> problem via X as X cannot be started in a user controlled environment then.

Why would this be a relevant security risk over an Xserver that runs as root and to which clients (at least with the right credentials) can connect to and send all sorts of bogus requests to?
What would be the 'user control' you are talking about? - Do you suspect any security risk in any of the command line switches that are available to non-root users?
The much bigger security risk to mee seems to be the Xserver running as root and not dropping root privileges. I suspect there is still some more work ahead to overcome this.
Comment 3 Matthias Hopf 2010-08-19 10:53:58 UTC
Ludwig, Xorg s-bit is already disabled in permissions.secure for a long time.
IMHO that is good enough for people that really care for security.

There was no exploitable security hole in how many years?
The latest security issue was in fact a kernel issue AFAIU.

Egbert's right, we have a much higher risk with the fact that the Xserver is running as root anyway, and that won't go away.
Comment 4 Ludwig Nussel 2010-08-19 11:34:13 UTC
(In reply to comment #1)
> how many users are still using startx. I'm afraid there are more than you
> think.

Question is whether we need to have a default configuration for that
use case. Those who still use startx are hopefully smart enough to
set the setuid bit themselves anyways.

(In reply to comment #3)
> Ludwig, Xorg s-bit is already disabled in permissions.secure for a long time.
> IMHO that is good enough for people that really care for security.

permissions 'secure' are not the default though.

> There was no exploitable security hole in how many years?

So it's about time that a new one is discovered you mean? :-)
Seriously, if the setuid bit isn't needed anymore by the majority of
users we should simply not enable that feature in the default
config anymore.

> The latest security issue was in fact a kernel issue AFAIU.

Yes but the exploit does not work if X is started via DM.

> Egbert's right, we have a much higher risk with the fact that the Xserver is
> running as root anyway, and that won't go away.

One step at a time, low hanging fruits first :-)
Comment 6 Egbert Eich 2010-08-19 15:22:52 UTC
I wonder why comments here are set private. This is an openSUSE bug and private comments should not exist there.
This is different if a specific security issue is discussed here however we discuss possible security breach scenarios. This knowledge is public although the general public may not be aware of those while every serious attacker is.
Thus making this discussion public can only serve to educate more people on the risks.
My comment (#2) was accidentally set private because for some strange reason the 'restrict' mark is set by default for me and I forgot to unset it before I committed the comment and for some other odd reason did unsetting the private bit fail.

Now to the issue at stake:
I expect to see numerous bug reports when people suddenly cannot run startx any more as modifying /etc/permissions.local is not the first thing which comes to their mind.
Thus if we want to do this change I strongly recommend to extend the startx script to test if the user running it is not root and if so fail with a message educating him why the change was made and what exactly to do to make startx work again for him.
Comment 7 Stefan Dirsch 2010-08-19 16:05:32 UTC
I don't believe anybody here has set his comment(s) private by intenation. This issue has been reported as Bug #631857.
Comment 8 Hans-Peter Holler 2010-08-20 08:51:22 UTC
You are not authorized to access bug #631857. Cool.
Comment 9 Stefan Dirsch 2010-08-20 09:09:11 UTC
(In reply to comment #8)
> You are not authorized to access bug #631857. Cool.

That's indeed internal Novell discussion, but I believe I can share the subject

  #631857 - bugzilla comments are internal per default

and the outcome. The bug has been closed as fixed (for openSUSE products if
I understood correctly).

Let's hope that this default gets reverted as soon as possible for the openSUSE products.
Comment 10 Ludwig Nussel 2010-12-02 10:24:15 UTC
sr#54298
Comment 11 Stefan Dirsch 2010-12-03 03:24:20 UTC
(In reply to comment #10)
> sr#54298

------------------------------------------------------------------
Thu Dec  2 09:29:24 UTC 2010 - lnussel@suse.de

- print warning if xinit fails and Xorg has no setuid bit
  (bnc#632737)
[...]

Thanks, Ludwig! Also forwarded as SR to openSUSE:Factory and been accepted.
Comment 12 Stefan Dirsch 2010-12-03 03:50:19 UTC
54380  State:new     By:sndirsch     When:2010-12-03T04:43:21
        submit:       X11:XOrg/xorg-x11-server  ->  openSUSE:Factory       
        Descr: - remove Xorg setuid bit (bnc #632737)

The required change in permissions package is already in openSUSE:Factory.

-------------------------------------------------------------------
Thu Dec  2 10:20:11 UTC 2010 - lnussel@suse.de

 - remove Xorg setuid bit (bnc#632737)

==> closing as fixed.
Comment 13 Will Stephenson 2011-03-18 08:40:08 UTC
Looks like the release notes weren't updated yet http://www.suse.de/relnotes/i386/openSUSE/11.4/RELEASE-NOTES.en.html
Could you update these, people are confused.

Is the correct way to start a session from the command line (for debugging purposes or whatever):

(as root)
> XOrg :1&

(as user)
> DISPLAY=:1 eval `dbus-launch startkde`

I'd like this in the release notes.
Comment 14 Stefan Dirsch 2011-03-18 09:36:22 UTC
(In reply to comment #13)
> Looks like the release notes weren't updated yet
> http://www.suse.de/relnotes/i386/openSUSE/11.4/RELEASE-NOTES.en.html
> Could you update these, people are confused.
> 
> Is the correct way to start a session from the command line (for debugging
> purposes or whatever):
> 
> (as root)
> > XOrg :1&
> 
> (as user)
> > DISPLAY=:1 eval `dbus-launch startkde`
> 
> I'd like this in the release notes.

This sounds like a weird approach. Isn't it easier to edit /etc/permissions.local?

# setuid bit on Xorg is only needed if no display manager, ie startx
# is used. Beware of CVE-2010-2240.
#
#/usr/bin/Xorg                 root:root       4711

remove a "#" and run SuSEconfig afterwards? This is also the recommendation
when you run "startx".
Comment 15 Will Stephenson 2011-03-18 10:57:41 UTC
It is.  If that's the easiest way then let's document it as that in the release notes.
Comment 16 Karl Eichwalder 2011-03-30 16:07:08 UTC
I propose to add this snippet:

Removing the Xorg setUID Bit
============================
The setuid bit on /usr/bin/Xorg is needed for starting X as
unprivileged user, e.g. via startx.  That method is deprecated in
favor of a display manager since years. Additionally modern
environments rely on device ACLs and polkit privileges, which in
turn depend on consolekit tracking the active console.

No setuid bit also prevents exploitation of the kernel-heap-stack
overflow problem via X as X cannot be started in a user
controlled environment anymore.  Therefore we removed the
setuid bit on Xorg from /etc/permissions.easy.

Users who actually need it, can set it again in
/etc/permissions.local by removing the comment sign from this
line:

#/usr/bin/Xorg                 root:root       4711

and running SuSEconfig afterwards.
Comment 17 Ludwig Nussel 2011-03-31 06:13:14 UTC
(In reply to comment #16)
> No setuid bit also prevents exploitation of the kernel-heap-stack
> overflow problem via X as X cannot be started in a user
> controlled environment anymore.  Therefore we removed the
> setuid bit on Xorg from /etc/permissions.easy.

The actual security problem was fixed in the kernel. Removing the
setuid bit is a preventive measurement against potential similar
problems in the future.

> Users who actually need it, can set it again in
> /etc/permissions.local by removing the comment sign from this
> line:
> 
> #/usr/bin/Xorg                 root:root       4711
> 
> and running SuSEconfig afterwards.

SuSEconfig --module permissions, SuSEconfig alone does not set
permissions anymore.
Comment 18 Karl Eichwalder 2011-03-31 07:11:13 UTC
Thanks for feedback.  Next try:

Removing the Xorg setUID Bit
============================
The setuid bit on /usr/bin/Xorg is needed for starting X as
unprivileged user, e.g. via startx.  That method is deprecated in
favor of a display manager since years. Additionally modern
environments rely on device ACLs and polkit privileges, which in
turn depend on consolekit tracking the active console.

The actual security problem was fixed in the kernel. Removing the
setuid bit is a preventive measurement against potential similar
problems in the future.

Users who depend on the old configuration, can set the setuid again
in /etc/permissions.local by removing the comment sign from the
following line:

#/usr/bin/Xorg                 root:root       4711

and running 'SuSEconfig --module permissions' afterwards.
Comment 19 Karl Eichwalder 2011-03-31 11:44:40 UTC
  <!-- bnc#632737 -->
With small wording changes, now in svn:

  <sect3 id="tec.xorg-setUID" status="2011-03-31">
   <title>Removing the Xorg setUID Bit</title>
   <para>
The setUID bit on <filename>/usr/bin/Xorg</filename> is needed for starting X
as an unprivileged user, e.g., via <command>startx</command>.  This method is
deprecated in favor of using a display manager since years. Additionally,
modern environments rely on device ACLs and polkit privileges, which in turn
depend on consolekit tracking the active console.</para>
   <para>
The actual security problem was fixed in the kernel. Removing the
setUID bit is a preventive measurement against potential similar
problems in the future.
</para>
   <para>
Users who depend on the old configuration, can set the setUID bit themself
in <filename>/etc/permissions.local</filename> by removing the comment sign from the
following line:</para>

<screen>#/usr/bin/Xorg                 root:root       4711</screen>

<para>
and running <command>SuSEconfig --module permissions</command> afterwards.</para>
  </sect3>
Comment 20 Karl Eichwalder 2011-03-31 11:45:07 UTC
I'll release it with Bug 683822.
Comment 21 Stefan Dirsch 2011-03-31 13:53:30 UTC
> The actual security problem was fixed in the kernel. Removing the
> setuid bit is a preventive measurement against potential similar
> problems in the future.

Why not simply removing this paragraph completely? I'm afraid people are going to ask: "Which security problem?" after reading this.
Comment 22 Will Stephenson 2011-03-31 15:49:47 UTC
Nativespeakerization, and I never saw it written 'setUID':

  <sect3 id="tec.xorg-setUID" status="2011-03-31">
   <title>Removing the Xorg setuid bit</title>
   <para>
The setuid bit on <filename>/usr/bin/Xorg</filename> is needed for starting X
as an unprivileged user, e.g., via <command>startx</command>.  This method has been deprecated for years in favor of using a display manager. Modern environments rely on device ACLs and polkit privileges, which in turn
depend upon consolekit tracking the active console, which is performed by the display manager.</para>
   <para>
The actual security problem was fixed in the kernel. Removing the
setuid bit is a preventive measurement against potential similar
problems in the future.
</para>
   <para>
Users who depend on the old configuration can set the setuid bit themselves
in <filename>/etc/permissions.local</filename> by removing the comment sign
from the
following line:</para>

<screen>#/usr/bin/Xorg                 root:root       4711</screen>

<para>
and running <command>SuSEconfig --module permissions</command>
afterwards.</para>
  </sect3>
Comment 23 Karl Eichwalder 2011-04-01 07:17:21 UTC
Thanks for your help, Will!

Stefan, I removed the para.
Comment 24 Swamp Workflow Management 2011-05-03 13:01:28 UTC
Update released for: release-notes-openSUSE
Products:
openSUSE 11.4 (i586)
Comment 25 Bernhard Wiedemann 2011-06-15 21:18:04 UTC
This is an autogenerated message for OBS integration:
This bug (632737) was mentioned in
https://build.opensuse.org/request/show/73652 11.4 / release-notes-openSUSE
Comment 26 Bernhard Wiedemann 2011-10-31 21:03:00 UTC
This is an autogenerated message for OBS integration:
This bug (632737) was mentioned in
https://build.opensuse.org/request/show/89843 Tumbleweed / permissions
Comment 27 Christian Boltz 2012-09-24 20:08:08 UTC
Looks like /usr/bin/Xorg was removed from /etc/permissions* in 12.2, but nobody removed the permissions handling from the xorg-x11-server package:

# rpm -V xorg-x11-server 
/usr/bin/Xorg: cannot verify root:root 0755 - not listed in /etc/permissions

A similar error happens at package installation (%set_permissions in %post).

I removed those macro calls. After this change, "verify(not mode)" for /usr/bin/Xorg is a bad idea, so I removed it.

I sent SR 135729 with the fixes. Please double-check my changes - it's my first permissions-related change ;-)
Comment 28 Marcus Meissner 2012-09-24 20:25:07 UTC
no, please revoke this sr.

this was intended.

/etc/permissions.local  has this for users that want it, commented out.
Comment 29 Christian Boltz 2012-09-24 21:10:09 UTC
(In reply to comment #28)
> no, please revoke this sr.

OK, done for now.

> this was intended.
> 
> /etc/permissions.local  has this for users that want it, commented out.

OK, that's an argument - but it still leaves an error message for (guessed) 99% of the users. Intentionally forcing an error message doesn't sound like a good idea to me ;-)

If you don't want to drop the permissions handling in xorg-x11-server, you should include /usr/bin/Xorg in /etc/permissions again (with mode 755 of course) to avoid the error message.
Comment 30 Stefan Dirsch 2012-09-25 08:49:12 UTC
(In reply to comment #27)
> # rpm -V xorg-x11-server 
> /usr/bin/Xorg: cannot verify root:root 0755 - not listed in /etc/permissions

Sorry, no idea how to address that.

> A similar error happens at package installation (%set_permissions in %post).

Here we could first grep for ^/usr/bin/Xorg in /etc/permissions* before running
the macros. Woult this make sense, Marcus?

> I removed those macro calls. After this change, "verify(not mode)" for
> /usr/bin/Xorg is a bad idea, so I removed it.

See above.
Comment 31 Karl Eichwalder 2012-09-25 11:44:10 UTC
If something release notes related is needed, please clone the bug and assign it to me.
Comment 32 Ludwig Nussel 2012-10-01 13:08:51 UTC
(In reply to comment #30)
> (In reply to comment #27)
> > # rpm -V xorg-x11-server 
> > /usr/bin/Xorg: cannot verify root:root 0755 - not listed in /etc/permissions
> 
> Sorry, no idea how to address that.
> 
> > A similar error happens at package installation (%set_permissions in %post).
> 
> Here we could first grep for ^/usr/bin/Xorg in /etc/permissions* before running
> the macros. Woult this make sense, Marcus?

Since the %verify(not mode) isn't changeable at runtime you either have to use the permissions handling or don't. Doesn't make sense to only have the %verifyscript macro conditional then. IMO both removing permission handling from the package as well as adding a 0755 entry to /etc/permissions are valid solutions. The former has the effect that rpm -V would complain if someone adds an entry to permissions.local but maybe that is even desirable.
Comment 33 Stefan Dirsch 2012-10-01 15:00:54 UTC
Ok. Christian, could you provide a proposal, i.e. an appropriate submitrequest for openSUSE:Factory? Thanks.
Comment 34 Christian Boltz 2012-10-01 18:03:28 UTC
I just reopened SR 135729 to remove the permission handling from xorg-x11-server.spec. Marcus didn't really like this (see comment 28), but it's the best solution IMHO.
Comment 35 Stefan Dirsch 2012-10-01 18:11:27 UTC
Ok. Accepted. it.
Comment 36 Bernhard Wiedemann 2012-10-01 19:00:13 UTC
This is an autogenerated message for OBS integration:
This bug (632737) was mentioned in
https://build.opensuse.org/request/show/136543 Factory / xorg-x11-server
Comment 37 Bernhard Wiedemann 2016-04-15 12:59:33 UTC
This is an autogenerated message for OBS integration:
This bug (632737) was mentioned in
https://build.opensuse.org/request/show/54322 Factory / permissions
https://build.opensuse.org/request/show/54344 Factory / xorg-x11