PID recycling enables an unprivileged user to generate and read a crash report for a privileged process

Hello Kev, thanks for the report; we'll take a look at get back to you soon.

Thanks

Sorry for the delay, we are still investigating this issue.

Have you assigned a CVE for this bug? If so, please could you let me know what it is?

Thanks,

Kev

Hi Kev,

Not yet - Seth can you please assign one?

Kev, there seems to be a few different issues here - the lock file being able to be controlled by a regular user means they can control the execution of Apport which makes your exploit workable, but I get the feeling that whilst this is clearly an issue, that even if we mitigate this there is still the issue that Apport could be made to race against PID reuse anyway and so it seems this is still worthy of it's own CVE. I am wondering if it would be sufficient to have Apport NOT set the real process ID when doing the initial drop_privileges() but to instead set the effective user ID only (or perhaps the file-system user ID only here) - then you would not be able to pause it as a regular user (but I am not familiar enough with the rest of Apport to know if this will break other parts of it).

Also we could perhaps try and do some checking when writing out the crash file that the PID which we are writing out still has the same user ID as the one which we originally were called for (and this is already available in the real_pid variable).

I would be keen to get your thoughts on these ideas and perhaps any other thoughts you might have on how best to resolve this.

Hi Alex,

Yes, I agree: the lock file being controllable by a regular user is not the vulnerability. It's useful for writing an exploit, but it's not a vulnerability by itself.

As far as I can see, PID recycling means that there is fundamentally no way for apport to confirm that the contents of /proc/[pid] correspond to the process that actually crashed. So I think the only thing that you can do is to make sure that the files in /proc/[pid] are owned by the correct user. It would still be possible to play games with PID recycling but there would be no benefit to attacker if it doesn't enable them to read information belonging to a different user. So my suggestion would be to drop privileges during add_proc_info:

https://git.launchpad.net/ubuntu/+source/apport/tree/apport/report.py?h=applied/ubuntu/bionic-devel&id=20c98691144e843bf1ab8428603beedd34e993ad#n507

I think that should prevent apport from reading the /proc/[pid]/maps file of a process that belongs to the wrong user.

Thanks,

Kev

My apologies Kev, all, I hadn't realized I missed this one when assigning CVEs earlier.

I'm not sure what the flaw is.

I'm also not sure about the fix:

- line 455: drop_privileges(True)
- line 500: info.add_proc_info(pid)

so privileges are partially dropped at this point already, no?

I'll feel better about assigning a CVE number once I understand a proposed fix.

Thanks

Hi Seth,

Here's my suggestion. Note that it would probably be cleaner to do this inside add_proc_info, rather than at the call site as I have done here. I have done it this way because it's the most concise way to present the idea. I copied the technique that Alex used to fix bug 1830858.

Thanks,

Kev

diff --git a/data/apport b/data/apport
index 03f93d12..af2c3333 100755
--- a/data/apport
+++ b/data/apport
@@ -497,7 +497,16 @@ try:

     # We already need this here to figure out the ExecutableName (for scripts,
     # etc).
- info.add_proc_info(pid)
+
+ euid = os.geteuid()
+ try:
+ # Drop permissions temporarily to make sure that we don't
+ # include information in the crash report that the user should
+ # not be allowed to access.
+ os.seteuid(os.getuid())
+ info.add_proc_info(pid)
+ finally:
+ os.seteuid(euid)

     if 'ExecutablePath' not in info:
         error_log('could not determine ExecutablePath, aborting')

It looks like the formatting of my diff got messed up, so here it is again as an attachment.

I was testing an approach that involved updating apport to access any files under /proc/pid using a file descriptor, which would prevent reading from recycled pid (ie. after the directory was gone any access would fail), but that got a bit messy: doing it that way required changes to the Report class and that class is also used by other tools, so I was having to update too many files. Although it is a sane thing to do, this would required a bit more design and work to be done properly.

For now I believe Kevin's fix is the better way to address this issue.

Tiago, I'd like to suggest keeping that work around, it'd be nice to be rid of the pid race in the future.

Please use CVE-2019-15790 for reading /proc/pid/ files with elevated privileges.

Thanks

Ok a CRD has been confirmed for this issue - Tuesday next week - 2019/10/29

Thanks for letting me know.

Kev

This bug was fixed in the package apport - 2.20.1-0ubuntu2.20

---------------
apport (2.20.1-0ubuntu2.20) xenial-security; urgency=medium

  * SECURITY UPDATE: apport reads arbitrary files if ~/.config/apport/settings
    is a symlink (LP: #1830862)
    - apport/fileutils.py: drop permissions before reading user settings file.
    - CVE-2019-11481
  * SECURITY UPDATE: TOCTTOU race conditions and following symbolic
    links when creating a core file (LP: #1839413)
    - data/apport: use file descriptor to reference to cwd instead
      of strings.
    - CVE-2019-11482
  * SECURITY UPDATE: fully user controllable lock file due to lock file
    being located in world-writable directory (LP: #1839415)
    - data/apport: create and use lock file from /var/lock/apport.
    - CVE-2019-11485
  * SECURITY UPDATE: per-process user controllable Apport socket file
    (LP: #1839420)
    - data/apport: forward crashes only under a valid uid and gid,
      thanks Stéphane Graber for the patch.
    - CVE-2019-11483
  * SECURITY UPDATE: PID recycling enables an unprivileged user to
    generate and read a crash report for a privileged process (LP: #1839795)
    - data/apport: drop permissions before adding proc info (special thanks
      to Kevin Backhouse for the patch)
    - data/apport, apport/report.py, apport/ui.py: only access or open
      /proc/[pid] through a file descriptor for that directory.
    - CVE-2019-15790

 -- Tiago Stürmer Daitx <email address hidden> Tue, 29 Oct 2019 05:23:08 +0000