Skip to content

cinnamon-schema scripts: harden against dangerous input#7670

Closed
mgerstner wants to merge 1 commit intolinuxmint:masterfrom
mgerstner:schema_scripts_security
Closed

cinnamon-schema scripts: harden against dangerous input#7670
mgerstner wants to merge 1 commit intolinuxmint:masterfrom
mgerstner:schema_scripts_security

Conversation

@mgerstner
Copy link
Copy Markdown
Contributor

These scripts are supposed to be run via pkexec as root. The current
implementation is very naive by trusting input parameters and passing
them to cp and rm via a shell subprocess. This allows wildcards to be
interpreted or path components to be added to the schema file to remove.
This can lead to the scripts performing way different operations than
originally intended.

With this change a couple of attack vectors are addressed:

  • removal of files outside of /usr/share/glib-2.0/schemas is prevented
  • reading from special devices that could cause the script to block
    infinitely or to cause disk space exhaustion in the target directory is
    prevented
  • copying of files not owned by the user running pkexec is denied to
    prevent information leaks
  • copying of overly large input files is prevented

These scripts are supposed to be run via pkexec as root. The current
implementation is very naive by trusting input parameters and passing
them to cp and rm via a shell subprocess. This allows wildcards to be
interpreted or path components to be added to the schema file to remove.
This can lead to the scripts performing way different operations than
originally intended.

With this change a couple of attack vectors are addressed:

- removal of files outside of /usr/share/glib-2.0/schemas is prevented
- reading from special devices that could cause the script to block
infinitely or to cause disk space exhaustion in the target directory is
prevented
- copying of files not owned by the user running pkexec is denied to
prevent information leaks
- copying of overly large input files is prevented
@collinss
Copy link
Copy Markdown
Member

Maybe I'm missing something here but this seems a little bit overkill to me. The only place these scripts are ever used is in Spices.py, to install a schema that an applet, desklet or extension includes in it's installation directory. In that case I know of no way that code injection could be accomplished unless (maybe) the user tries to manually install a spice that is compromised, and in that case we give warnings that that method of installation should only be used by an applet dev to test his code. These scripts also can't be run without elevated permissions, so if something has the privileges to run this script, it wouldn't need to use code injection to accomplish it's nefarious purposes.

Now, I'm no security expert by any stretch, but it seems to me that this is a really big change for something that isn't really that much of a risk, when we could just incorporate the functionality right into Spices.py itself and avoid the whole issue.

Can you give me a concrete example of how someone could use this to cause problems for the average user without that user doing something that would leave it vulnerable even without the existence of this script?

@mgerstner
Copy link
Copy Markdown
Contributor Author

@collinss well actually I'm suggesting this as part of a security review for openSUSE (see openSUSE bug).

I can understand the applicative view you have on this. However, you are using polkit as an authentication framework here and also ship polkit policy files (e.g. files/usr/share/polkit-1/actions/org.cinnamon.schema-install.policy). In these policy files auth_admin_keep is specified for the two schema scripts. This makes the scripts accessible in a broader context than just the call in Spices.py. Also, system integrators or users might get the idea to relax those policy settings to even allow calling the scripts without any password authentication or only user password authentication.

In this context the current script implementation would provide a privilege escalation. Hypothetical example: You just installed that cool Applet a minute ago and entered your admin password to install the schema file. Through auth_admin_keep your graphical user session will be allowed to reexecute /usr/bin/cinnamon-schema-install with arbitrary parameters without password authentication for a couple of minutes. If you now accidentally leave your desktop unguarded for a minute, or your user account is already compromised for some reason, then an attacker can use /usr/bin/cinnamon-schema-install to install arbitrary files into /usr/share/glib-2.0/schemas like say /root/secret_file.txt, and read them. Similarly /usr/bin/cinnamon-schema-remove could be used to remove more or less arbitrary files from the system and make it unusable.

Generally programs executed via pkexec with relaxed authentication settings need to be designed similar to setuid binaries. The current implementation is a bad example and such things have the tendency to be copy/pasted into different contexts until something really bad happens.

@mgerstner
Copy link
Copy Markdown
Contributor Author

@collinss on a different topic: I wonder why it is necessary to install those gsettings schemas at all into the system. Isn't there a way to keep them in a user's home directory?

@collinss
Copy link
Copy Markdown
Member

I wonder why it is necessary to install those gsettings schemas at all into the system. Isn't there a way to keep them in a user's home directory?

@mgerstner yeah, I was just wondering that myself.

@collinss
Copy link
Copy Markdown
Member

So I did some digging, and found that it is possible to install schemas to a non-system folder (eg. ~/.local/share/glib-2.0/schemas/) but then the applet would not be able to access it by default. There are a couple different ways to get it working though:

  • add the path to the XDG_DATA_DIRS environment variable
  • modify the applet with some extra lines of code so that it points the gsettings api to that folder.

I'm not sure if the first one is a good idea or not. I haven't really worked much with environment variables so I have no idea what best-practices are in that regard. The second one would require modifying all the existing applets that use custom schemas, but I believe it can be done so that it's backwards-compatible, so that may be the best course.

@collinss
Copy link
Copy Markdown
Member

After some discussion on the team we decided that at this point it's not really worth preserving support for this feature. Instead, we have opted to remove this feature and modify the 3 existing applets in our repositories that still use a schema file.

@Odyseus
Copy link
Copy Markdown
Contributor

Odyseus commented Jun 27, 2018

Hello, everybody.

@collinss: Just an FYI. gsettings schemas doesn't need to be installed on the system (/usr/share/glib-2.0/schemas) for an xlet to use this feature. An xlet can decide from what location the schema can be loaded. I have several xlets that uses gsettings schemas, and in one of them I fallback to a custom location to search for the compiled schema in case the schema isn't installed in the system.

The only disadvantages of not installing a schema globally, that I found so far, are that the schema cannot be exported/imported (with dconf dump/load for example) and the schema keys cannot be modified/read by the gsettings command (with gsettings set/get for example). Only from the mechanism created by the xlets themselves can they be modified/read (or dconf-editor). And all of this can simply be fixed by a mechanism provided by the xlets themselves to install the schemas globally.

...Instead, we have opted to remove this feature...

What does that entails exactly? If it means that you will remove the ability to automatically install/remove a gsettings schema on xlet install/removal, I can live with that. But I really hope that you will not remove the ability to define an external settings application to use instead of the Cinnamon's xlets settings system (external-configuration-app key on an xlet metadata.json file).

@mtwebster
Copy link
Copy Markdown
Member

34043b7

We've left in the ability to define an external program. I realize there are alternate ways of using dconf that would have avoided system-wide modification, but we'd prefer to encourage the use of the xlet setting api, as it's much simpler all around to use, is user-specific, and avoids having to write your own gui to configure your applet.

@Odyseus
Copy link
Copy Markdown
Contributor

Odyseus commented Jun 27, 2018

Hello, @mtwebster.

We've left in the ability to define an external program.

Thank you very much for that. I really appreciate it. 👍

...we'd prefer to encourage the use of the xlet setting api...

I'm 100% with you on that. But, sometimes an xlet needs the power of a custom GUI to handle its settings because the native API isn't capable of certain things. And most importantly, building a custom GUI from scratch is super fun. LOL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants