[PATCH xserver] xorg-wrapper: when starting the server as root, reset its environment

[Hans de Goede]

Hi,

On 18-10-15 19:26, Julien Cristau wrote:
> When the server is privileged, we shouldn't be passing the user's
> environment directly.
>
> Signed-off-by: Julien Cristau <jcristau at debian.org>

I've no real objections against this, and I can see this being a good
thing from a security pov, but I'm afraid this may cause regressions.

Before we had the wrapper the server itself used to be suid-root,
and none of the code for dealing with that has been removed (the server
can still be build that way). So I would expect the server to sanitize
its environment itself...

So I've 2 questions:

1) Is there any concrete reason why this is necessary ?
2) Does anyone know of any use-cases this may break ?

Regards,

Hans



> ---
>   hw/xfree86/xorg-wrapper.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> It's possible some variables should be passed, in which case we could
> use a whitelist; in my testing this patch seemed to work, though.
>
> diff --git a/hw/xfree86/xorg-wrapper.c b/hw/xfree86/xorg-wrapper.c
> index 22e97ad..d6efb23 100644
> --- a/hw/xfree86/xorg-wrapper.c
> +++ b/hw/xfree86/xorg-wrapper.c
> @@ -190,6 +190,7 @@ int main(int argc, char *argv[])
>       int total_cards = 0;
>       int allowed = CONSOLE_ONLY;
>       int needs_root_rights = -1;
> +    char *const envp[1] = { NULL, };
>
>       progname = argv[0];
>
> @@ -265,7 +266,10 @@ int main(int argc, char *argv[])
>       }
>
>       argv[0] = buf;
> -    (void) execv(argv[0], argv);
> +    if (getuid() == geteuid())
> +        (void) execv(argv[0], argv);
> +    else
> +        (void) execve(argv[0], argv, envp);
>       fprintf(stderr, "%s: Failed to execute %s: %s\n",
>           progname, buf, strerror(errno));
>       exit(1);
>