Review from SUSE security team
Quoting from https://bugzilla.opensuse.org/1217186
It seems we actually never reviewed this fontinst service before. The code for it is pretty old. I am not very happy with the D-Bus interface and the actions implemented there.
To start off, the only exported D-Bus method, org.kde.fontinst.manage, is actually a one-catches all method that multiplexes the following individual function calls:
QString method = args["method"].toString();
// qDebug() << method;
if ("install" == method) {
result = install(args);
} else if ("uninstall" == method) {
result = uninstall(args);
} else if ("move" == method) {
result = move(args);
} else if ("toggle" == method) {
result = toggle(args);
} else if ("removeFile" == method) {
result = removeFile(args);
} else if ("reconfigure" == method) {
result = reconfigure();
} else if ("saveDisabled" == method) {
result = saveDisabled();
These should all be individual D-Bus methods with individual Polkit actions associated with them. Only then will it be possible to actually benefit from Polkit by changing authentication settings on a fine grained level.
The single manage action that we have now is set to auth_admin_keep
for the
logged in user. Although this way only people that have admin access anyway
are able to call the method, there are a couple of worries here. A look at the
individual sub-actions:
- install(): this can be used to copy arbitrary file paths to arbitrary locations, the new files end up with mode 0644.
- uninstall(): this allows to remove arbitrary file paths, as long as their parent directories have a writable bit set.
- move(): this allows to move arbitrary paths complete with new owner uid and group gid to arbitrary new locations.
- toggle(): this takes raw XML that also seems to specify font paths that are to be enabled or disabled.
- removeFile(): does what is says on the label.
- configure(): saves modified font directories and invokes a small bash script
fontinst_x11
that prepares font directories and triggers a font refresh at the X server.
This range of freedom looks like the fontinst helper is actually the new kio D-Bus layer that allows arbitrary file operations. It is way beyond the scope of what a fontinst helper should do. If an administrator wants to allow regular users to modify system fonts by lower the authentication requirements for this Polkit action then these regular uses will easily be able to gain full root privileges by using this D-Bus API.
I strongly recommend investing some efforts here for KDE6 with a major interface change:
- provide separate D-Bus methods and Polkit actions for each "sub-method" we currently have.
- instead of providing arbitrary source paths, the D-Bus clients should pass on an already open file descriptor. This way it is made sure that the caller actually has permissions to access the file.
- instead of providing arbitrary destination paths, the helper should make sure that fonts are only installed in system font directories.
- instead of allowing arbitrary uid/gid ownership changes the font files should always be owned by root:root mode 0644 if the service is running on the system bus. If the service is running on the session bus then no ownership change will be possible anyway (since the service is running unprivileged).
Otherwise the code could certainly benefit from investing some love, in parts
it still uses C APIs (like fopen()
) and it generally looks pretty
convoluted.
Here is one additional misc finding:
- in
Misc::setFilePerms()
there is aumask()
save & restore call around achmod()
invocation. This doesn't make sense.chmod()
doesn't even use the umask.