Comment # 18 on bug 1134132 from Matthias Gerstner

(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.