https://bugzilla.suse.com/show_bug.cgi?id=1134132 https://bugzilla.suse.com/show_bug.cgi?id=1134132#c18 --- Comment #18 from Matthias Gerstner <matthias.gerstner@suse.com> --- (In reply to matthias.gerstner@suse.com from comment #2)

This dde-file-manager-daemon is a security nightmare! None of the D-Bus methods it exports are protected by polkit so just any user can call them. While this is not generally a problem it *is* a problem here, because the exported methods allow a bunch of privileged operations to everybody.

Findings about the daemon in general:

- in the D-Bus configuration file com.deepin.filemanager.daemon.conf we find the following:

``` <!-- Only root can own the service --> <policy user="root"> <allow own="com.deepin.filemanager.daemon"/> </policy>

<!-- Allow anyone to invoke methods on the interfaces --> <policy context="default"> <allow own="com.deepin.filemanager.daemon"/> ```

There are two "own=" lines one for root and one for everybody else. This allows any user to register this daemon (or actually: any program) on the D-Bus system bus. It allows to hijack other users' D-Bus connections. This is especially bad since for example in the method `setUserSharePassword` cleartext passwords are passed.

This is still the same in the current package. This needs to be fixed.

- the daemon creates a log file in `/var/cache/deepin/dde-file-manager-daemon`. First of all this is not the right place for a log file. Log files belong to /var/log or /var/lib at best. Furthermore the log file is world readable which should be avoided for logs originating from privileged programs.

The file seems not to be created any more, but I don't know what is used instead. Probably into the systemd journal?

- the checkAuthorization() call (which is unused actually at the moment) uses the deprecated polkit UnixProcessSubject with PIDs again. Like in the other Deepin programs this needs to be changed to use the D-Bus SystemBusName subject.

This is now used and also uses the system bus name. Looks okay.

Findings in the UserShareManager interface:

- setUserSharePassword: allows to set arbitrary users' smd password. Changes the database in /var/lib/samba/private. If at all then this must only be allowed for the caller's username.

This is now Polkit authenticated. The name is still passed unfiltered to `smbpasswd`, though. It could be a command line parameter "--evil". This should be filtered.

- addGroup: calls `groupadd` so regular users can create arbitrary groups. Not acceptable. Not acceptable interface without admin protection. - addUserToGroup: allows arbitrary users to add arbitrary other users to arbitrary other groups. Luckily doesn't work on SUSE, because `/usr/sbin/adduser` is called but we have `useradd`. Not acceptable interface without admin-protection. - restartSambaService calls `smbd restart`, shouldn't this go out to systemd instead? Must also be authenticated.

These methods no longer exist.

Findings in the TagManagerDaemon:

- disposeClientData: manages some tags for files that are stored in a per filesystem SQLite database. Doesn't look trustworthy. Why should everybody be able to fiddle with tags?

This no longer seems to exist.

Findings in UsbFormatter:

- mkfs: create jfs, ext2/3/4, btrfs, swap, hfs, dosfs, xfs, reiserfs on arbitrary paths. This can overwrite arbitrary regular files, too, if they are large enough. Not acceptable interface without admin-authentication.

This no longer seems to exist.

Findings in DeviceInfoManager:

The methods in this interface are somehwat okay but they still allow to call lsblk and various low level file system information tools on arbitrary block devices or other paths.

This no longer seems to exist. Newly added D-Bus methods: - com.deepin.filemanager.daemon.MountControl.Mount: allows to mount file systems, not Polkit authenticated. There is support for "common" file systems, which is not implemented though. Then there's "cifs" which allows pretty much freedom about passing strings to "mkdir()" and to "mount()", creates directories with user controlled data in /media/<username>/samba. In principle allows "../" elements in paths, only the `mkdir()` luckily fails to follow them. Then there's a "dlnfs" file system plugin which looks for a "dlnfs" program before continuing. I couldn't find out what program this is supposed to be. It's not packaged on openSUSE anyway. A user controlled path is passed as command line argument to this program, which might allow privilege escalation, but I cannot say for sure, since I don't know the program. - com.deepin.filemanager.daemon.MountControl.Unmount: allows to unmount some file systems. The plugins that deal with this use libmount mtab for finding out about already existing mounts. This could be tricked by users that are allowed to mount FUSE based file systems. They can choose arbitrary strings as "source device" that might confuse the logic in the plugins. - com.deepin.filemanager.daemon.UserShareManager.CloseSmbShareByShareName: this tries to unmount a share via "smbcontrol smbd close-share %1", where %1 is a user controlled string. There is no Polkit authentication. There are some checks that the resulting path is not a symlink or an escape out of /var/lib/samba/usershares. If somebody managed to mount a samba share under a weird name like "myshare --extra-parameter=this", then the resulting QProcess execution of smbcontrol might result in side effects. - com.deepin.filemanager.daemon.UserShareManager.EnableSmbServices: allow to enable smbd and nmbd without authentication. This must be authenticated. - com.deepin.filemanager.daemon.AccessControlManager: + SetAccessPolicy(): uses an unsafe utils::isValidInvoker() helper function that checks the callers /proc/%1/exe. This is a racy check that can we circumvented by unprivileged callers by replacing their process with one of the whitelisted ones. + SetVaultAccessPolicy(): use the same unsafe check + Chmod(): Allows to change the mode of arbitrary paths. Luckily Polkit authenticated. But there is a file existence check issue in there. If the file doesn't exist, then the call exists early without authenticating. Thus a caller can determine whether certain files exist or not, by observing whether Polkit authentication requests are sent out or not. Overall this service still isn't fit for inclusion for a lot of reasons. Most if not all of the D-Bus methods need to Polkit authenticated (they can have a 'yes' setting for active users, if the methods don't allow highly privileged things. But nobody & friends should not be allowed to call all these methods. Other shaky authentication tests should be dropped like this utils::isValidInvoker(). The D-Bus configuration "own" line still needs to be fixed. -- You are receiving this mail because: You are on the CC list for the bug.