Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elevation of Privileges Exploit #998

Closed
3 tasks done
Nonary opened this issue Mar 5, 2023 · 7 comments · Fixed by #969
Closed
3 tasks done

Elevation of Privileges Exploit #998

Nonary opened this issue Mar 5, 2023 · 7 comments · Fixed by #969
Labels
os:Windows OS is Windows

Comments

@Nonary
Copy link
Collaborator

Nonary commented Mar 5, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Is your issue described in the documentation?

  • I have read the documentation

Is your issue present in the nightly release?

  • This issue is present in the nightly release

Describe the Bug

All of the do/undo commands run in an elevated context which means it would be possible for those without administrator rights to execute scripts utilizing Sunshine at System level.

Expected Behavior

do/undo commands should execute under the users account, unprivileged like it does for launching applications.

Additional Context

This might be a complicated one to fix, because simply adding a checkbox to run elevated or not will not be an effective solution. The configuration file does not require administrator rights to modify, so as long as the file itself is editable by users this exploit will exist unless we either remove the elevation.

Host Operating System

Windows

Operating System Version

Windows 11

Architecture

32 bit

Sunshine commit or version

0.18.4

Package

Windows - installer

GPU Type

Nvidia

GPU Model

N/A

GPU Driver/Mesa Version

N/A

Capture Method (Linux Only)

N/A

Config

N/A

Apps

No response

Relevant log output

N/A
@cgutman
Copy link
Collaborator

cgutman commented Mar 14, 2023

Executing do/undo commands elevated is the expected behavior today. Some commands require admin to reconfigure system-wide settings for streaming, so we would need to provide that as an option.

The fact that apps.json is world-writable is the problem. Protecting it just has fundamental issues supporting both elevated and non-elevated launches of Sunshine.

I'm very tempted to just say "service is mandatory" for the installable version of Sunshine and users that want to run Sunshine unelevated can use the portable version. We simply support too many configurations today to be able to do something reasonable with the permissions and location of the config files.

@Nonary
Copy link
Collaborator Author

Nonary commented Mar 14, 2023

Executing do/undo commands elevated is the expected behavior today. Some commands require admin to reconfigure system-wide settings for streaming, so we would need to provide that as an option.

The fact that apps.json is world-writable is the problem. Protecting it just has fundamental issues supporting both elevated and non-elevated launches of Sunshine.

I'm very tempted to just say "service is mandatory" for the installable version of Sunshine and users that want to run Sunshine unelevated can use the portable version. We simply support too many configurations today to be able to do something reasonable with the permissions and location of the config files.

Yes, that is the main issue is the config file is editable without admin rights. If we fix that portion, then there really isn't a risk of elevation of privileges. However, it's still possible to elevate using users token #1036... assuming they have left UAC on.

Personally, I would rather have Sunshine in an unelevated context, there are many ways to safely elevate for running commands that need elevation.

@cgutman
Copy link
Collaborator

cgutman commented Mar 16, 2023

But you can't interact with the UAC prompt if Sunshine isn't elevated due to UIPI. Elevation on demand via UAC doesn't work unless Sunshine itself is elevated.

@Nonary
Copy link
Collaborator Author

Nonary commented Mar 16, 2023

But you can't interact with the UAC prompt if Sunshine isn't elevated due to UIPI. Elevation on demand via UAC doesn't work unless Sunshine itself is elevated.

That's true, which is why that portion of the code is only executed for the user impersonation portion. If they're not using it as a service, it wouldn't attempt to elevate it.

@ReenigneArcher
Copy link
Member

I was about to merge #1036, but I have one concern. I assume this would require the user to be physically at the server (or use some other remote connection method) to accept UAC prompts prior to the stream being started? If that's true it would not be great for the user experience of "remote" streaming.

I'm trying to think of what types of commands would require elevation. Of course trying to change anything in the registry would... does Qres require elevation? I'm guessing a good amount, if not all nircmd commands would require elevation, this seems to be a popular tool among users. https://www.nirsoft.net/utils/nircmd.html

The fact that apps.json is world-writable is the problem. Protecting it just has fundamental issues supporting both elevated and non-elevated launches of Sunshine.

I'm very tempted to just say "service is mandatory" for the installable version of Sunshine and users that want to run Sunshine unelevated can use the portable version. We simply support too many configurations today to be able to do something reasonable with the permissions and location of the config files.

Would writing the config/app files to the user's localappdata directory resolve these issues? This is a user directory, and as far as I know Windows require admin rights to access another user's home directory. If they run as a service those files would be saved for the System user... This is basically what we do on Linux as well.

This is more extreme, but another idea I have is to encode the contents of the files, perhaps with the secret key of the SSL certificate. That would also solve the issues we get when a user manually modifies the files (specifically, the apps.json) with incorrect syntax.

@Nonary
Copy link
Collaborator Author

Nonary commented Mar 17, 2023

@ReenigneArcher It does not, that was why the elevation tool was created. If I had simply launched it without using the helper process, it would have prevented Sunshine from functioning until the physically click Yes/NO on the prompt at their machine.

Because the new process runs unelevated, it is able to prompt UAC on the users screen without blocking the existing behavior of Sunshine.

This process also only happens for those that installed Sunshine as a service, otherwise it will run at same elevation as the user if ran manually.

@Nonary
Copy link
Collaborator Author

Nonary commented Mar 17, 2023

Would writing the config/app files to the user's localappdata directory resolve these issues?}

It would not, by design users have full write access to their own user profile. And most applications don't create a specific user for their software in this case. There are better alternatives to write protecting a file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os:Windows OS is Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants