Apport get_config incorrectly drops privileges

Bug #1903332 reported by Marc Deslauriers
258
This bug affects 1 person
Affects Status Importance Assigned to Milestone
apport (Ubuntu)
Fix Released
Undecided
Unassigned

Bug Description

Privilege dropping code here drops uid before gid instead of the correct order of gid before uid. Likely this code fails and is caught by the try statement:

https://git.launchpad.net/ubuntu/+source/apport/tree/apport/fileutils.py?h=applied/ubuntu/focal-devel&id=94d98694e19b5614f3a764a9e4dcf171ad5039a5#n339

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Please also do the python equivalant of setgroups(0, NULL) before dropping the gid (you could also use a one element list that is the dropped-to group). To add them back, you'll need to do the setgroups() after raising the uid. Drop order is setgroups(), setegid(), seteuid(); raise order is seteuid(), setegid(), setgroups().

Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

What groups do you think the root user would be in that would require doing that?

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Assuming we aren't talking about a setuid application, it's possible for the supplementary groups to contain 'root' (eg, run under sudo, ssh root logins, etc).

If we are talking about a setuid application, we shouldn't blindly drop them (and there is no reason to since root isn't going to be part of the group set in a setuid application anyway).

Please verify that the code in question isn't running under setuid (IME it is not) and assuming it is not running under setuid, please drop the supplementary groups to ensure we're covered all the cases when temporarily dropping and the group is present.

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Marc and I also discussed this on IRC wrt setgroups(0, NULL) and initgroups() to the user's groups. IME, if the requirements/expectations of the code in question is fine with just the primary group, setgroups(0, NULL) is safest, but if that isn't the case, we should consider initgroups.

Revision history for this message
Marc Deslauriers (mdeslaur) wrote :
Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

Perhaps we should simply drop support for those home directory files. It looks like the main one in this bug hasn't worked in a while and perhaps by fixing it we will introduce more security issues that were masked by it being broken...

Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

I guess we can't drop support for the second one.

Revision history for this message
Marc Deslauriers (mdeslaur) wrote :

OK, a little more information about the main issue in this bug. The privilege dropping code was introduced to fix CVE-2019-11481, from bug 1830862, which had two parts:

- File was read as root
- There was no limit on the size of the file so a symlink to /dev/zero would cause apport to hang

The patch fixed #1. The patch did not specifically address #2, but since the privilege dropping code was broken resulting in a failure, the result was that #2 was no longer an issue.

Fixing the privilege dropping code will re-introduce #2.

The file is parsed using ConfigParser(), and there is no API to limit the size of the file being read. There are two possible ways around the issue:

1- Implement a simple key-value parser in a few lines of code that includes a limit on the number of lines read from the file. Since apport only uses the [main] header, parsing headers is not needed.

2- Read the file with a line limit before passing it to ConfigParser().

I suspect #1 would give us more control in limiting and addressing further attacks in the future.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apport - 2.20.11-0ubuntu50.1

---------------
apport (2.20.11-0ubuntu50.1) groovy-security; urgency=medium

  * Various security hardening fixes (LP: #1903332)
    - apport/fileutils.py: drop privileges in the correct order, limit
      settings file size.
    - apport/apport/report.py: properly drop privileges, limit ignore file
      size.
    - data/apport: drop supplemental groups.

 -- Marc Deslauriers <email address hidden> Tue, 10 Nov 2020 15:03:57 -0500

Changed in apport (Ubuntu):
status: New → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apport - 2.20.11-0ubuntu27.12

---------------
apport (2.20.11-0ubuntu27.12) focal-security; urgency=medium

  * Various security hardening fixes (LP: #1903332)
    - apport/fileutils.py: drop privileges in the correct order, limit
      settings file size.
    - apport/apport/report.py: properly drop privileges, limit ignore file
      size.
    - data/apport: drop supplemental groups.

 -- Marc Deslauriers <email address hidden> Tue, 10 Nov 2020 15:03:57 -0500

Changed in apport (Ubuntu):
status: New → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apport - 2.20.9-0ubuntu7.20

---------------
apport (2.20.9-0ubuntu7.20) bionic-security; urgency=medium

  * Various security hardening fixes (LP: #1903332)
    - apport/fileutils.py: drop privileges in the correct order, limit
      settings file size.
    - apport/apport/report.py: properly drop privileges, limit ignore file
      size.
    - data/apport: drop supplemental groups.

 -- Marc Deslauriers <email address hidden> Tue, 10 Nov 2020 15:03:57 -0500

Changed in apport (Ubuntu):
status: New → Fix Released
information type: Private Security → Public Security
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.