Bug 1130388

Created attachment 801090 [details]
com.deepin.pkexec.deepin-clone.policy

Please reveiwe this file.

This package is at https://build.opensuse.org/package/show/X11:Deepin:Factory/deepin-clone

Thank you for opening the review bug. The program is of moderate size (about
8.500 lines of C++ code). Most of the code is probably concerned with GUI
management.

The other deepin code reviews from a while ago showed a couple of security
problems so we should be careful with this addition.

The security team is currently under a high load of reviews so it may take a
while until we can take care of this one.

I will start working on this review.

I just realize that there is still bug 1070943 pending for deepin-api. Do you
have a new version there with the patches included?

(In reply to Matthias Gerstner from comment #5)
> I just realize that there is still bug 1070943 pending for deepin-api. Do you
> have a new version there with the patches included?

Yes, I have updated bug 1070943. Thanks!

So I reviewed deepin-clone and there are a couple of issues regarding
security. They need to be fixed before we can accept the package:

1) in GUI mode deepin-clone creates "/tmp/.deepin-clone.log" and follows
  symlinks there. This file needs to be open()ed with
  O_NOFOLLOW|O_CREAT|O_EXCL.  Or even better using a non predictable temporary
  file in /tmp using QTemporaryFile.

2) temporaryMountDevice() uses a fixed path
  /tmp/.deepin-clone/mount/<block-dev-basename> to temporarily mount a file
  system there. These paths can be prepared by an attacker and symlinks will
  be followed during mounting. If the attacker quickly enters the mount then
  it probably can also prevent the following unmount. This logic can e.g. be
  triggered by running `deepin-clone -i /dev/sdX`.

  An attacker can thus cause the file system to be permanently mounted at an
  arbitrary location in the file system.

3) `Helper::getPartitionSizeInfo()` uses /tmp/partclone.log as a fixed path
  during execution of partclone. The same issues about symlink attacks etc.
  like in 1) apply here. This needs to be a non-predictable path, or a path in
  a directory not accessible to regular users.

4) similarly in `BootDoctor::fix()` the fixed path /tmp/repo.iso is created and
  the fixed directory /tmp/.deepin-clone is used. The same concerns as in 1)
  and 3) apply.

5) /tmp/.deepin-clone.log and /var/log/deepin-clone.log are both world
  readable. Log files whould not be world readable when they come from a
  program running as root as it may leak information valuable to an attacker.

6) in helper.cpp:986:

  ```
  if (QFile::exists(QString("/proc/%1").arg(process->pid()))) {
      process->terminate();
      process->waitForFinished();
  } else {
  ```

  I don't know if this check is really necessary but if it is then it is a
  race condition and could be used to kill an unrelated process in case the
  child PID is replaced by some other process unrelated to deepin-clone.

7) in GUI and pkexec mode deepin-clone opens the `PKEXEC_UID`s
  `~/.pam_environment` while following symlinks and not checking file types.
  While it can be argued that the executor that successfully was granted
  access by pkexec shouldn't be an attacker it would still be a better style
  not to open this file as root. It should be opened only with
  setfsuid(PKEXEC_UID).

Non security issues but style/robustness issues:

8) isBlockSpecialFile() returns true if the path starts with /dev? That sounds
  like a hack and could lead to surprises!
9) DDiskInfo::getInfo() under some circumstances also creates the file? Sounds
  like an unexpected side effect!

The issue 1) and 2) and probably also 3) and 4) could require CVE assignments
since these are realistic security issues that can be exploited by local
regular users in the system.

(In reply to Matthias Gerstner from comment #7)
> So I reviewed deepin-clone and there are a couple of issues regarding
> security. They need to be fixed before we can accept the package:
> 
> 1) in GUI mode deepin-clone creates "/tmp/.deepin-clone.log" and follows
>   symlinks there. This file needs to be open()ed with
>   O_NOFOLLOW|O_CREAT|O_EXCL.  Or even better using a non predictable
> temporary
>   file in /tmp using QTemporaryFile.
> 
> 2) temporaryMountDevice() uses a fixed path
>   /tmp/.deepin-clone/mount/<block-dev-basename> to temporarily mount a file
>   system there. These paths can be prepared by an attacker and symlinks will
>   be followed during mounting. If the attacker quickly enters the mount then
>   it probably can also prevent the following unmount. This logic can e.g. be
>   triggered by running `deepin-clone -i /dev/sdX`.
> 
>   An attacker can thus cause the file system to be permanently mounted at an
>   arbitrary location in the file system.
> 
> 3) `Helper::getPartitionSizeInfo()` uses /tmp/partclone.log as a fixed path
>   during execution of partclone. The same issues about symlink attacks etc.
>   like in 1) apply here. This needs to be a non-predictable path, or a path
> in
>   a directory not accessible to regular users.
> 
> 4) similarly in `BootDoctor::fix()` the fixed path /tmp/repo.iso is created
> and
>   the fixed directory /tmp/.deepin-clone is used. The same concerns as in 1)
>   and 3) apply.
> 
> 5) /tmp/.deepin-clone.log and /var/log/deepin-clone.log are both world
>   readable. Log files whould not be world readable when they come from a
>   program running as root as it may leak information valuable to an attacker.
> 
> 6) in helper.cpp:986:
> 
>   ```
>   if (QFile::exists(QString("/proc/%1").arg(process->pid()))) {
>       process->terminate();
>       process->waitForFinished();
>   } else {
>   ```
> 
>   I don't know if this check is really necessary but if it is then it is a
>   race condition and could be used to kill an unrelated process in case the
>   child PID is replaced by some other process unrelated to deepin-clone.
> 
> 7) in GUI and pkexec mode deepin-clone opens the `PKEXEC_UID`s
>   `~/.pam_environment` while following symlinks and not checking file types.
>   While it can be argued that the executor that successfully was granted
>   access by pkexec shouldn't be an attacker it would still be a better style
>   not to open this file as root. It should be opened only with
>   setfsuid(PKEXEC_UID).
> 
> Non security issues but style/robustness issues:
> 
> 8) isBlockSpecialFile() returns true if the path starts with /dev? That
> sounds
>   like a hack and could lead to surprises!
> 9) DDiskInfo::getInfo() under some circumstances also creates the file?
> Sounds
>   like an unexpected side effect!
> 
> The issue 1) and 2) and probably also 3) and 4) could require CVE assignments
> since these are realistic security issues that can be exploited by local
> regular users in the system.

Thanks! I will report to upstream.

Upstream have fixed this security issue in new version 1.1.3, could you check this fix?

The Fix: https://github.com/linuxdeepin/deepin-clone/pull/18
The new version building: https://build.opensuse.org/package/show/X11:Deepin:Factory/deepin-clone

So lets see what has been improved:

(In reply to matthias.gerstner@suse.com from comment #7)
> 1) in GUI mode deepin-clone creates "/tmp/.deepin-clone.log" and follows
>   symlinks there. This file needs to be open()ed with
>   O_NOFOLLOW|O_CREAT|O_EXCL.  Or even better using a non predictable temporary
>   file in /tmp using QTemporaryFile.

This file has been moved to /var/log/deepin-clone.log. At least the symlink
attack should be avoided this way.

> 2) temporaryMountDevice() uses a fixed path
>   /tmp/.deepin-clone/mount/<block-dev-basename> to temporarily mount a file
>   system there. These paths can be prepared by an attacker and symlinks will
>   be followed during mounting. If the attacker quickly enters the mount then
>   it probably can also prevent the following unmount. This logic can e.g. be
>   triggered by running `deepin-clone -i /dev/sdX`.

The mount is now performed in QStandardPaths::RuntimeLocation which is
/run/user/$UID on Linux. It's a bit unusual to use that for this purpose but
it should address the symlink attack.

> 3) `Helper::getPartitionSizeInfo()` uses /tmp/partclone.log as a fixed path
>   during execution of partclone. The same issues about symlink attacks etc.
>   like in 1) apply here. This needs to be a non-predictable path, or a path in
>   a directory not accessible to regular users.

The file has been moved to /var/log/partclone.log. The symlink attack should
be addressed this way.

> 4) similarly in `BootDoctor::fix()` the fixed path /tmp/repo.iso is created and
>   the fixed directory /tmp/.deepin-clone is used. The same concerns as in 1)
>   and 3) apply.

This has been moved to /var/cache/deepin-clone. /var/cache is usually only
writable by root. Should handle this attack vector.

> 5) /tmp/.deepin-clone.log and /var/log/deepin-clone.log are both world
>   readable. Log files whould not be world readable when they come from a
>   program running as root as it may leak information valuable to an attacker.

As far as I can see this item has not been addressed.

> 6) in helper.cpp:986:
> 
>   ```
>   if (QFile::exists(QString("/proc/%1").arg(process->pid()))) {
>       process->terminate();
>       process->waitForFinished();
>   } else {
>
>   ```
>   I don't know if this check is really necessary but if it is then it is a
>   race condition and could be used to kill an unrelated process in case the
>   child PID is replaced by some other process unrelated to deepin-clone.

This item has also not been adressed, only a Chinese comment was added that I
can't fully understand. It seems to be a kind of workaround for something but
this approach is not good security wise.

> 7) in GUI and pkexec mode deepin-clone opens the `PKEXEC_UID`s
>   `~/.pam_environment` while following symlinks and not checking file types.
>   While it can be argued that the executor that successfully was granted
>   access by pkexec shouldn't be an attacker it would still be a better style
>   not to open this file as root. It should be opened only with
>   setfsuid(PKEXEC_UID).

The parsing of this file has been removed, instead an environment variable
D_QT_THEME_CONFIG_PATH pointing to the users ~/.config directory is setup. I'm
not quite sure where this environment variable is used at all at the moment.
So the status is unclear to me.

> Non security issues but style/robustness issues:
> 
> 8) isBlockSpecialFile() returns true if the path starts with /dev? That sounds
>   like a hack and could lead to surprises!

This is still the same.

> 9) DDiskInfo::getInfo() under some circumstances also creates the file? Sounds
>   like an unexpected side effect!

So the issues 5), 6), 8) and 9) have not been addressed yet. For 7) I'm not
sure.

The more severe issues have been fixed at least. 5) and 6) should still be
addressed before we accept it, the others ... well they look very hacky and I
think something should be done as well.

There has been no response from the packager or from upstream for a long time.
Closing this and related deepin bugs.

Assign to justforlxz@gmail.com

(In reply to hillwoodroc@gmail.com from comment #13)
> Assign to justforlxz@gmail.com

this time actually assign :)