Thank you for opening the review bug.

Can you tell us where to find the sources / the package containing Xorg.wrap?

What is the current concept in openSUSE to start the X server as root?

The wrapper is part of xserver sources (xorg-x11-server). It's built via 

  --enable-suid-wrapper 

configure option. Actually we already had this in place for some time, but disabled it again. (%{build_suid_wrapper} in xorg-x11-server specfile). And Egbert needed to change a lot in order to make it working, usable.

-------------------------------------------------------------------
Tue Apr 12 15:33:45 UTC 2016 - eich@suse.com

- Add permission verification for SUID wrapper
- Disable SUID wrapper per default until reviewed

-------------------------------------------------------------------
Tue Apr 12 13:59:48 UTC 2016 - eich@suse.com

-  n_Install-Avoid-failure-on-wrapper-installation.patch:
   rename to:
    N_Install-Avoid-failure-on-wrapper-installation.patch
-  u_xorg-wrapper-Drop-supplemental-group-IDs.patch:
   Drop supplementary group privileges.
-  u_xorg-wrapper-build-Build-position-independent-code.patch:
   Build position independent.

-------------------------------------------------------------------
Tue Apr 12 09:06:06 UTC 2016 - eich@suse.com

- n_Install-Avoid-failure-on-wrapper-installation.patch:
  Fix up build for wrapper.
- Place SUID wrapper into a separate package: 
  xorg-x11-server-wrapper

-------------------------------------------------------------------
Thu Apr  7 15:53:39 UTC 2016 - eich@suse.com

- Set configure option --enable-suid-wrapper for TW:
  This way, the SUID wrapper is built which allows to run the Xserver
  as root even though the the DM instance runs as user. This allows to
  support drivers which require direct HW access.

Honestly I never tried the X wrapper. Maybe Egbert still remembers how good it worked, when he tried it.

And I still would prefer having modeset X driver running on generic KMS driver running on generic framebuffer, which has been developed by Thomas, but still has issues for X autoconfiguration. :-( Adding Thomas therefore.

I reviewed the wrapper code and it looks mostly sane to me.

One minor suggestion is to skip the root detection and privdrop logic if the
wrapper already runs as root:

```
--- xserver-1.20.9.orig/hw/xfree86/xorg-wrapper.c
+++ xserver-1.20.9/hw/xfree86/xorg-wrapper.c
@@ -231,6 +231,10 @@ int main(int argc, char *argv[])
             break;
         }
     }
+    else {
+        // don't perform any checks or privdrops if we're root anyway
+        needs_root_rights = 1;
+    }

 #ifdef WITH_LIBDRM
     /* Detect if we need root rights, except when overriden by the config */
```

The other thing that caught my attention is concerning these lines:

```
    if (getuid() == geteuid())
        (void) execv(argv[0], argv);
    else
        (void) execve(argv[0], argv, empty_envp);
```

So the code takes precautions that a regular user can't specify environment
variables when the Xserver is started as root. But `argv` is passed on
unfiltered.

The Xserver accepts a plethora of command line options. A couple of them deal
with files. And a couple of them seem also the setuid-root aware, like
`-logfile`, where the man page says:

```
This option is only available when the server is run as root (i.e, with real-uid 0).
```

I think there was a CVE a couple of years ago regarding local exploits using
this switch. `man Xserver` lists more interesting switches that could escalate
privileges:

-displayfd fd
-auth authorization-file
+extension<extensionName>
-listen trans-type
-xkbmap filename

I don't really want to check all of the Xorg code for safe treatment of these
switches in the setuid-root context. My question is rather, if we should allow
any switches to be passed in the setuid-root mode. Or at least, if a small
whitelist of switches should only be allowed.

Thanks for the code review, Matthias! Your comments make a lot of sense to me! AFAIK gdm only uses a few Xserver options and these are even hardcoded in gdm and cannot be configured easily like it's possible with xdm and maybe also other DMs. So I think we should simply asking our GNOME developers, which options they need and generate a whitelist from this information. Does this sound reasonable?

Matthias, have you also checked the patches we're already applying against the wrapper in the xorg-x11-server specfile, i.e.

N_Install-Avoid-failure-on-wrapper-installation.patch
u_xorg-wrapper-Drop-supplemental-group-IDs.patch
u_xorg-wrapper-build-Build-position-independent-code.patch

Sorry I don't know how to configure the X server whitelist in xdm, could you show the configuration? Referring this configuration I can know what gdm will do.

And I'm not sure whitelist is useful. In a system if X server need root priority to start, that means every user needs to be in whitelist when the user want to start gnome user session. If we want to forbid all user to start X server as root, we can set Xorg.wrap config 'needs_root_rights = no'.

Not sure, whether we're on the same page here. Aren't we talking about a whitelist for Xserver options? There's no whitelist for anything in xdm I would be aware of. xdm still startx Xserver as root.

s/startx/starts

There is no whitelist in gdm. If we want a normal user is not shown in login user list, we have a trick way to do it.

(In reply to sndirsch@suse.com from comment #6)
> Matthias, have you also checked the patches we're already applying against the wrapper in the xorg-x11-server specfile, i.e.

I looked at the fully patched version of the wrapper. But I can have a look at
the individual patches, too. One seems to affect the build process.

(In reply to xiaoguang.wang@suse.com from comment #10)
> There is no whitelist in gdm. If we want a normal user is not shown in login
> user list, we have a trick way to do it.

We are not takling about whitelisting users, we are are talking about
whitelisting command line arguments passed from the Xorg.wrap setuid-root
program to the Xorg server.

So that when a non-root user invokes Xorg.wrap, and the Xorg server needs to
be run with root privileges, that only a subset of the supported command line
arguments will be allowed.

(In reply to Matthias Gerstner from comment #12)
> We are not takling about whitelisting users, we are are talking about
> whitelisting command line arguments passed from the Xorg.wrap setuid-root
> program to the Xorg server.
> 
> So that when a non-root user invokes Xorg.wrap, and the Xorg server needs to
> be run with root privileges, that only a subset of the supported command line
> arguments will be allowed.

Sorry, my bad, I misunderstand whitelist before.
gdm starts X server with fix arguments, if you need I can show it here.

(In reply to sndirsch@suse.com from comment #6)
> Matthias, have you also checked the patches we're already applying against
> the wrapper in the xorg-x11-server specfile, i.e.
> 
> N_Install-Avoid-failure-on-wrapper-installation.patch
> u_xorg-wrapper-Drop-supplemental-group-IDs.patch
> u_xorg-wrapper-build-Build-position-independent-code.patch

The patches are okay. The N_Install-Avoid-failure-on-wrapper-installation.patch
is a bit strange (why should a user with UID!=0 && EUID==0 be building this
package? That would be unwise. But I guess the intention is simply to avoid
error output here.

@Matthias 
About N_Install-Avoid-failure-on-wrapper-installation.patch: Unfortunately I don't know more than it is mentioned in the patch itself. Egbert wrote this patch and added it.

@xiaoguang
Yes, providing the fix arguments for Xserver would be helpful.

The X arguments in gdm:

/usr/bin/X vt2 (could be vt2-7)
-displayfd x
-auth xxxx 
-background none 
-noreset 
-keeptty 
-verbose 7 (could be 7 or 3)
-core
-listen tcp
-nolisten tcp

(In reply to xiaoguang wang from comment #16)
> The X arguments in gdm:
> 
> /usr/bin/X vt2 (could be vt2-7)
> -displayfd x
> -auth xxxx 
> -background none 
> -noreset 
> -keeptty 
> -verbose 7 (could be 7 or 3)
> -core

Ok

> -listen tcp
> -nolisten tcp

Both? Please explain. "-nolisten tcp" is the default since xorg-server 1.17. So when are you using "-listen tcp"? Hope it's not the default. Otherwise I would expect a security ticket to be opened immediately against gdm. ;-)

(In reply to Stefan Dirsch from comment #17)
> > -listen tcp
> > -nolisten tcp
> 
> Both? Please explain. "-nolisten tcp" is the default since xorg-server 1.17.
> So when are you using "-listen tcp"? Hope it's not the default. Otherwise I
> would expect a security ticket to be opened immediately against gdm. ;-)

The default is no "-nolisten" and no "-listen".
When defined "HAVE_XSERVER_THAT_DEFAULTS_TO_LOCAL_ONLY"(sorry I'm not clear what is used for),
and allowing remote connection, add "-listen tcp".
or not allowing remote connection, add "-nolisten tcp".

(In reply to xiaoguang wang from comment #18)
> (In reply to Stefan Dirsch from comment #17)
> > > -listen tcp
> > > -nolisten tcp
> > 
> > Both? Please explain. "-nolisten tcp" is the default since xorg-server 1.17.
> > So when are you using "-listen tcp"? Hope it's not the default. Otherwise I
> > would expect a security ticket to be opened immediately against gdm. ;-)
> 
> The default is no "-nolisten" and no "-listen".

? Not sure I undesrtood that ;-)

> When defined "HAVE_XSERVER_THAT_DEFAULTS_TO_LOCAL_ONLY"(sorry I'm not clear
> what is used for),

I don't knnow where one can find this in gdm, but from the meaning this would be set to "yes" for all xservers since version 1.17,
i.e. sle12-sp2. There is also 

## Type:        yesno
## Default:     no
#
# TCP port 6000 of Xserver. When set to "no" (default) Xserver is
# started with "-nolisten tcp". Only set this to "yes" if you really
# need to. Remote X service should run only on trusted networks and
# you have to disable firewall for interfaces, where you want to
# provide this service. Use ssh X11 port forwarding whenever possible.
#
DISPLAYMANAGER_XSERVER_TCP_PORT_6000_OPEN="no"

in /etc/sysconfig/displaymanager, but I'm not sure whether this is uses by gdm. Maybe only by xdm and other DMs.

> and allowing remote connection, add "-listen tcp". or not allowing remote connection, add "-nolisten tcp".

Ok. Where is this being configured? config file? gdm UI? Somewhere else?

s/undesrtood/understood

The code in file daemon/gdm-x-session.c in gdm:
>         /* If we were compiled with Xserver >= 1.17 we need to specify
>          * '-listen tcp' as the X server dosen't listen on tcp sockets
>          * by default anymore. In older versions we need to pass
>          * -nolisten tcp to disable listening on tcp sockets.
>          */
> #ifdef HAVE_XSERVER_THAT_DEFAULTS_TO_LOCAL_ONLY
>         if (allow_remote_connections) {
>                 g_ptr_array_add (arguments, "-listen");
>                 g_ptr_array_add (arguments, "tcp");
>         }
> #else
>         if (!allow_remote_connections) {
>                 g_ptr_array_add (arguments, "-nolisten");
>                 g_ptr_array_add (arguments, "tcp");
>         }
> #endif

When Xserver >= 1.17, the HAVE_XSERVER_THAT_DEFAULTS_TO_LOCAL_ONLY is defined. Then when allow_remote_connections is true, add '-listen tcp'.
The allow_remote_connections is from DisallowTCP in file /etc/gdm/custom.conf, and the default value of allow_remote_connections is false.

Thanks a lot for detailed explanation. In short, we wold need "-listen" in Xserver option whitelist for gdm. And this is *not* the default. I wanted to make this sure.

Created attachment 841705 [details]
OptionFilter.c

Standalone Option Filter sample code

If this is the right approach I can try to add this option filter to ./hw/xfree86/xorg-wrapper.c.

Created attachment 841726 [details]
OptionFilter.c

Ok. That one looks more promising. Xsrever doesn't accept empty options ...

Created attachment 841732 [details]
OptionFilter.c

This one can execute itself. Unfortunately when trying to integrate the code into xorg wrapper I'm running into buffer overflows ...

Created attachment 841745 [details]
u_xorg-wrapper-Xserver-Options-Whitelist-Filter.patch

Ok. I have a working patch for the filter.

Created attachment 841748 [details]
u_xorg-wrapper-Xserver-Options-Whitelist-Filter.patch

This patch can be reviewed.

I have a package working in principal

  https://build.opensuse.org/package/show/home:sndirsch:branches:X11:XOrg/xorg-x11-server

but building still includes a warning:

[  138s] xorg-x11-server-wrapper.x86_64: E: permissions-file-setuid-bit (Badness: 10) /usr/bin/Xorg.wrap is packaged with setuid/setgid bits (04755)

Also I added a config file for the wrapper.

# cat /etc/X11/Xwrapper.config 

# rootonly, console, anybody
allowed_users=anybody
# yes, no, auto
needs_root_rights=no

Unfortunately running Xorg via Xwrapper does not really work for me as normal user.

# Xorg.wrap  -displayfd 2
Xorg.wrap: /usr/bin/Xorg -displayfd 2 
[...]
(EE) parse_vt_settings: Cannot open /dev/tty0 (Permission denied)

It works as root though.

(In reply to Stefan Dirsch from comment #29)
> Unfortunately running Xorg via Xwrapper does not really work for me as
> normal user.
> 
> # Xorg.wrap  -displayfd 2
> Xorg.wrap: /usr/bin/Xorg -displayfd 2 
> [...]
> (EE) parse_vt_settings: Cannot open /dev/tty0 (Permission denied)

Without Egbert's patch

  u_xorg-wrapper-Drop-supplemental-group-IDs.patch

this changes to 

  Fatal server error:
  (EE) parse_vt_settings: Cannot open /dev/tty0 (Permission denied)
 
Xwrapper completely unpatched gives the same result. Seems this Wrapper simply doesn't work and probably never did. No idea
what other distributors, which are claimed to be using it, are doing. Maybe they are heavily patching it, Who knows ...

I need more than "Debian and Fedora are using it since years sucessfully"!

Ouch. I understood this wrong. It needs to be

  needs_root_rights=yes

in /etc/X11/Xwrapper.config. Then it works also as regular user.

(In reply to Matthias Gerstner from comment #5)
> One minor suggestion is to skip the root detection and privdrop logic if the
> wrapper already runs as root:
> 
> ```
> --- xserver-1.20.9.orig/hw/xfree86/xorg-wrapper.c
> +++ xserver-1.20.9/hw/xfree86/xorg-wrapper.c
> @@ -231,6 +231,10 @@ int main(int argc, char *argv[])
>              break;
>          }
>      }
> +    else {
> +        // don't perform any checks or privdrops if we're root anyway
> +        needs_root_rights = 1;
> +    }
> 
>  #ifdef WITH_LIBDRM
>      /* Detect if we need root rights, except when overriden by the config */
> ```

I don't see what this alone would change. The value of needs_root_rights is checked against 0 and -1 but nowhere against 1.

> I think there was a CVE a couple of years ago regarding local exploits using
> this switch. `man Xserver` lists more interesting switches that could
> escalate
> privileges:
> 
> -displayfd fd
> -auth authorization-file

Unfortunately both are needed by gdm.

> -listen trans-type

This as well.

(In reply to sndirsch@suse.com from comment #31)
> Ouch. I understood this wrong. It needs to be
> 
>   needs_root_rights=yes
> 
> in /etc/X11/Xwrapper.config. Then it works also as regular user.

This means that the auto detection is overridden and the Xserver is always
started as root. No surprise this works ;-). But then we also wouldn't need
the wrapper in the first place.

(In reply to Matthias Gerstner from comment #33)
> (In reply to sndirsch@suse.com from comment #31)
> > Ouch. I understood this wrong. It needs to be
> > 
> >   needs_root_rights=yes
> > 
> > in /etc/X11/Xwrapper.config. Then it works also as regular user.
> 
> This means that the auto detection is overridden and the Xserver is always
> started as root. No surprise this works ;-). But then we also wouldn't need
> the wrapper in the first place.

Well, I was testing with needs_root_rights=no. ;-) But indeed you're right. The same happens with "auto", so autodetection
apparently does not check for access to /dev/ttyX. :-(

But who knows. Maybe gdm gets access via ACLs to /dev/ttyX via systemd when gdm gets started.

@xiaoguang Could you please test this with the current packages from

  https://build.opensuse.org/package/show/home:sndirsch:branches:X11:XOrg/xorg-x11-server

Plesae play around with "auto" and "yes" for needs_root_rights in /etc/X11/Xwrapper.config when using gdm. gdm needs to use Xorw.wrap instead of Xorg now. Xorg.sh may also work. Of course Xorg.wrap should be started as user gdm for testing, not any longer as user root.

Let's do some testing as long as Matthias is on vacation. ;-)

You should at least update xog-x11-server and (install new) xorg-x11-server-wrapper.

(In reply to Stefan Dirsch from comment #36)

> Plesae play around with "auto" and "yes" for needs_root_rights in
> /etc/X11/Xwrapper.config when using gdm. gdm needs to use Xorw.wrap instead
> of Xorg now. Xorg.sh may also work. Of course Xorg.wrap should be started as
> user gdm for testing, not any longer as user root.
For my understanding, that means I need to add patch on gdm to change X command from '/usr/bin/X' to '/usr/bin/Xorg.wrap', is that right?

On other distros(Fedaro, Ubuntu), the `/usr/bin/Xorg' was replaced by Xorg.sh, they needn't to change X server command on gdm.

I will link `/usr/bin/X` to `/usr/bin/Xorg.sh` to do the test.

1. Link `/usr/bin/X` to `/usr/bin/Xorg.sh`
2. needs_root_rights=auto
   with nomodeset, no issue.
   without nomodeset, no issue.
3. needs_root_rights=yes
   with nomodeset, no issue.
   without nomodeset, no issue.

Looks. good. I'm wondering which driver you were using. I'm using qemu with "-vga qxl" and qxl X driver and needs_root_rights=auto doesn't work for me (same issue with modeset X driver). I need to set needs_root_rights=yes. Otherwise I'm getting

(EE) parse_vt_settings: Cannot open /dev/tty0 (Permission denied)

(In reply to Stefan Dirsch from comment #40)
> Looks. good. I'm wondering which driver you were using. I'm using qemu with
> "-vga qxl" and qxl X driver and needs_root_rights=auto doesn't work for me
> (same issue with modeset X driver). I need to set needs_root_rights=yes.
> Otherwise I'm getting
> 
> (EE) parse_vt_settings: Cannot open /dev/tty0 (Permission denied)

I use virtio.

Same issue with virtio. Maybe gdm gets access to /dev/tty0 somehow. You could figure out with

  getfacl /dev/tty0

(In reply to Stefan Dirsch from comment #42)
> Same issue with virtio. Maybe gdm gets access to /dev/tty0 somehow. You
> could figure out with
> 
>   getfacl /dev/tty0

suse@localhost:~> getfacl /dev/tty0
getfacl: Removing leading '/' from absolute path names
# file: dev/tty0
# owner: root
# group: tty
user::rw-
group::-w-
other::---

Then I don't know. Maybe the wrapper is still being started as root and not by gdm itself ...

(In reply to Stefan Dirsch from comment #44)
> Then I don't know. Maybe the wrapper is still being started as root and not
> by gdm itself ...

Could you please verify that?

(In reply to Stefan Dirsch from comment #45)
> (In reply to Stefan Dirsch from comment #44)
> > Then I don't know. Maybe the wrapper is still being started as root and not
> > by gdm itself ...
> 
> Could you please verify that?

1. needs_root_rights=auto  without nomodeset
> root      1006  0.0  0.4 236156  8604 ?        Sl   Sep23   0:00 /usr/sbin/gdm
> root      1483  0.0  0.5 167940 11896 ?        Sl   Sep23   0:00  \_ gdm-session-worker [pam/gdm-password]
> suse      1511  0.0  0.2 158200  5472 tty2     Ssl+ Sep23   0:00      \_ /usr/libexec/gdm/gdm-x-session --run-script /usr/bin/gnome-session
> suse      1513  0.0  3.8 238768 77112 tty2     Sl+  Sep23   0:05          \_ /usr/bin/Xorg vt2 -displayfd 3 -auth /run/user/1000/gdm/Xauthority -background none -norese
> suse      1518  0.0  0.7 258272 15244 tty2     Sl+  Sep23   0:00          \_ /usr/libexec/gnome-session-binary

2. needs_root_rights=auto  with nomodeset
> root      1031  0.0  0.4 236156  8612 ?        Sl   08:01   0:00 /usr/sbin/gdm
> root      1521  0.0  0.6 167940 12188 ?        Sl   08:02   0:00  \_ gdm-session-worker [pam/gdm-password]
> suse      1548  0.0  0.2 158200  5468 tty2     Ssl+ 08:02   0:00      \_ /usr/libexec/gdm/gdm-x-session --run-script /usr/bin/gnome-session
> root      1550  1.5  2.8 222652 58196 tty2     Sl+  08:02   0:01          \_ /usr/bin/Xorg vt2 -displayfd 3 -auth /run/user/1000/gdm/Xauthority -background none -nore
> suse      1555  0.0  0.7 258272 15268 tty2     Sl+  08:02   0:00          \_ /usr/libexec/gnome-session-binary

Ok. Makes sense. With "nomodeset"  KMS cannot be used, so X drivers like "fbdev" need to be used instead, which need root rights, so X gets started as root.

I made things easier for everyone now.

-------------------------------------------------------------------
Thu Sep 24 01:40:17 UTC 2020 - Stefan Dirsch <sndirsch@suse.com>

- n_xorg-wrapper-rename-Xorg.patch
  * moved Xorg to Xorg.bin and Xorg.sh to Xorg (boo#1175867)
- change default for needs_root_rights to auto in Xwrapper.config
  (boo#1175867)

So just use X or Xorg to start Xserver/Xwrapper now.

--> https://build.opensuse.org/package/show/home:sndirsch:branches:X11:XOrg/xorg-x11-server

(In reply to sndirsch@suse.com from comment #32)
> (In reply to Matthias Gerstner from comment #5)
> > One minor suggestion is to skip the root detection and privdrop logic if the
> > wrapper already runs as root:
> > 
> > ```
> > --- xserver-1.20.9.orig/hw/xfree86/xorg-wrapper.c
> > +++ xserver-1.20.9/hw/xfree86/xorg-wrapper.c
> > @@ -231,6 +231,10 @@ int main(int argc, char *argv[])
> >              break;
> >          }
> >      }
> > +    else {
> > +        // don't perform any checks or privdrops if we're root anyway
> > +        needs_root_rights = 1;
> > +    }
> > 
> >  #ifdef WITH_LIBDRM
> >      /* Detect if we need root rights, except when overriden by the config */
> > ```
> 
> I don't see what this alone would change. The value of needs_root_rights is checked against 0 and -1 but nowhere against 1.

it would simply skip the checks like

```
if (needs_root_rights == -1) {
	// checks DRM stuff
}
```

will not be executed when root is running the wrapper. Would be pointless
anyways. Even worse for the privilege drop logic, root dropping privileges to
root is not very sensible :-P

> > I think there was a CVE a couple of years ago regarding local exploits using
> > this switch. `man Xserver` lists more interesting switches that could
> > escalate
> > privileges:
> > 
> > -displayfd fd
> > -auth authorization-file
> 
> Unfortunately both are needed by gdm.
> 
> > -listen trans-type
> 
> This as well.

Okay if this is the case we can either drop the idea of whitelisting
parameters altogether, or grant a rather generous whitelist.

I still need to review your patch in attachment 841748 [details], I will do some in the
next couple of days.

> but building still includes a warning:
> 
>   [  138s] xorg-x11-server-wrapper.x86_64: E: permissions-file-setuid-bit
>   (Badness: 10) /usr/bin/Xorg.wrap is packaged with setuid/setgid bits (04755)

The warning will vanish once I whitelist /usr/bin/Xorg.wrap via the
permissions package.

About needs_root_rights. You're right. Thanks for explanation. I'll add a patch. Thanks for reviewing the patch! I somewhat expected you can/need to do something in permission files to fix this build warning. ;-)

Created attachment 842064 [details]
refactored and extended argument whitelisting patch

I took the liberty to rewrite your patch in attachment 841748 [details] and you can find
the result in attachment 842064 [details]:

- simplified the option parsing code a bit
- changed the "ignore forbidden argument" logic into an "abort on forbidden
  argument" logic. This is safer and avoids surprises on the user's end that
  could occur if the desired command line arguments aren't effective but the
  Xorg server is still started.
- tried to adjust to the coding style present in the file (mostly the
  function name)
- added some logic to apply the option filtering only to non-root users when
  Xorg is actually started as root. This should allow for full flexibility if
  root calls the wrapper or if the Xorg server only runs with user privileges.

Comment on attachment 842064 [details]
refactored and extended argument whitelisting patch

>Index: xserver-1.20.9/hw/xfree86/xorg-wrapper.c
>===================================================================
>---
>+++ hw/xfree86/xorg-wrapper.c	2020-09-29 12:52:59.256970275 +0200
>@@ -191,6 +191,60 @@
>     return 0;
> }
> 
>+static int check_vt_range(long int vt)
>+{
>+    if (vt >= 2 && vt <= 7 ) {
>+        return 1;
>+    }
>+
>+    return 0;
>+}
>+
>+/* Xserver option whitelist filter (boo#1175867) */
>+static int option_filter(int argc, char* argv[]){
>+
>+    for(int pos=1; pos<argc; pos++) {
>+        const char *arg = argv[pos];
>+
>+        if (strlen(arg) == 3 && !strncmp(arg,"vt", 2) && check_vt_range(strtol(arg+2, NULL, 10)) == 1) {
>+            /* vtX (vt2-vt7) */
>+            continue;
>+        } else if(!strcmp(arg,"-displayfd") ||
>+                  !strcmp(arg,"-auth") ||
>+                  !strcmp(arg,"-background") ||
>+                  !strcmp(arg,"-verbose") ||
>+                  !strcmp(arg,"-listen")) {
>+            /* -displayfd x
>+               -auth xxxx
>+               -backgound none
>+               -verbose 7 (7 or 3)
>+               -listen tcp
>+            */
>+            if ((pos+1) < argc) {
>+                pos++;
>+            } else {
>+                fprintf(stderr, "%s: Missing argument for Xserver option \"%s\". Aborting.\n",
>+                        progname, arg);
>+                return 0;
>+            }
>+        } else if (!strcmp(arg,"-noreset") ||
>+                   !strcmp(arg,"-keeptty") ||
>+                   !strcmp(arg,"-core")) {
>+            /* -noreset
>+               -keeptty
>+               -core
>+            */
>+            continue;
>+        } else {
>+            fprintf(stderr, "%s: Xserver option \"%s\" invalid or not in whitelist. Aborting.\n",
>+                    progname, arg);
>+            return 0;
>+        }
>+    }
>+
>+    return 1;
>+}
>+
> int main(int argc, char *argv[])
> {
> #ifdef WITH_LIBDRM
>@@ -250,11 +304,14 @@
> 
>             close(fd);
>         }
>+        /* If we've found cards, and all cards support kms, drop root rights */
>+        if (total_cards && kms_cards == total_cards) {
>+            needs_root_rights = 0;
>+        }
>     }
> #endif
> 
>-    /* If we've found cards, and all cards support kms, drop root rights */
>-    if (needs_root_rights == 0 || (total_cards && kms_cards == total_cards)) {
>+    if (needs_root_rights == 0) {
>         gid_t realgid = getgid();
>         uid_t realuid = getuid();
>         int ngroups = 0;
>@@ -326,6 +383,15 @@
>     }
> 
>     argv[0] = buf;
>+
>+    if (needs_root_rights == 1 && getuid() != 0)
>+    {
>+        /* Xserver option whitelist filter (boo#1175867) */
>+        if (option_filter(argc, argv) == 0) {
>+            exit(1);
>+        }
>+    }
>+
>     if (getuid() == geteuid())
>         (void) execv(argv[0], argv);
>     else

No idea how it came to my comment #53. Apparently I pressed some wrong button when trying to mark the patch as "patch".

Anyway, I had a look at the patch and it makes perfectly sense to me. Thanks a lot for working on this! I've already applied it and 
it does 100% what it claims.  :-)

Packages for testing will be available shortly via

  https://build.opensuse.org/package/show/home:sndirsch:branches:X11:XOrg/xorg-x11-server

@xiaoguang Please give them a try as in comment #39, so this ticket can be closed finally. Also consider my comment #48. Thanks!

Install latest package xorg-x11-server and xorg-x11-server-wrapper from https://build.opensuse.org/package/show/home:sndirsch:branches:X11:XOrg/xorg-x11-server

Do the test:
1. needs_root_rights=auto
   with nomodeset, no issue.
   without nomodeset, no issue.
2. needs_root_rights=yes
   with nomodeset, no issue.
   without nomodeset, no issue.

Great news. So I took the liberty and submitrequested what we have right now to factory/TW.

  https://build.opensuse.org/request/show/838622

Matthias, could you please whitelist  /usr/bin/Xorg.wrap via the permissions package, so this may be accepted in the end?

Thanks for integrating everything.

I just submitted the whitelisting to Factory via OBS sr#838724.

This is an autogenerated message for OBS integration:
This bug (1175867) was mentioned in
https://build.opensuse.org/request/show/838733 Factory / permissions

This is an autogenerated message for OBS integration:
This bug (1175867) was mentioned in
https://build.opensuse.org/request/show/838746 Factory / xorg-x11-server

So let's close it.

This is an autogenerated message for OBS integration:
This bug (1175867) was mentioned in
https://build.opensuse.org/request/show/931965 15.3 / permissions

openSUSE-SU-2021:1520-1: An update that solves three vulnerabilities and has 27 fixes is now available.

Category: security (moderate)
Bug References: 1028975,1029961,1093414,1133678,1148788,1150345,1150366,1151190,1157498,1160285,1160764,1161335,1161779,1163588,1167163,1169614,1171164,1171173,1171569,1171580,1171686,1171879,1171882,1173221,1174504,1175720,1175867,1178475,1178476,1183669
CVE References: CVE-2019-3687,CVE-2019-3688,CVE-2020-8013
JIRA References: 
Sources used:
openSUSE Leap 15.3 (src):    permissions-20200127-lp153.24.3.1