Incomplete fix for CVE-2012-0949

Bug #1004503 reported by Marc Deslauriers on 2012-05-25
264
This bug affects 1 person
Affects Status Importance Assigned to Milestone
update-manager (Ubuntu)
High
Brian Murray
Natty
High
Marc Deslauriers
Oneiric
High
Marc Deslauriers
Precise
High
Marc Deslauriers
Quantal
High
Brian Murray

Bug Description

The following USN fixed CVE-2012-0949:

http://www.ubuntu.com/usn/usn-1443-1/

"Felix Geyer discovered that the Update Manager Apport hook incorrectly
uploaded certain system state archive files to Launchpad when reporting
bugs. This could possibly result in repository credentials being included
in public bug reports."

This was originally LP #954483

Unfortunately, the state archive files are still being uploaded. It seems there is code in DistUpgradeApport.py that attaches the contents of the /var/log/dist-upgrade directory and manually runs apport.

apport_crash() can be simply modified to exclude the archive files, but fixing apport_pkgfailure() is more complicated.

Marc Deslauriers (mdeslaur) wrote :

Michael,

Do you have an idea of the best way to fix this?

Thanks!

Changed in update-manager (Ubuntu Natty):
assignee: nobody → Marc Deslauriers (mdeslaur)
Changed in update-manager (Ubuntu Oneiric):
assignee: nobody → Marc Deslauriers (mdeslaur)
Changed in update-manager (Ubuntu Precise):
assignee: nobody → Marc Deslauriers (mdeslaur)
Changed in update-manager (Ubuntu Natty):
importance: Undecided → High
Changed in update-manager (Ubuntu Oneiric):
importance: Undecided → High
Changed in update-manager (Ubuntu Precise):
importance: Undecided → High
Changed in update-manager (Ubuntu Quantal):
importance: Undecided → High
assignee: nobody → Michael Vogt (mvo)
Changed in update-manager (Ubuntu Natty):
status: New → Confirmed
Changed in update-manager (Ubuntu Oneiric):
status: New → Confirmed
Changed in update-manager (Ubuntu Precise):
status: New → Confirmed
Changed in update-manager (Ubuntu Quantal):
status: New → Confirmed
Brian Murray (brian-murray) wrote :

In /usr/share/apport/package_hook, which is called by DistUpgradeApport.py, we see that we can individual log files.

optparser.add_option('-l', '--log',*
    help='Append given log file, or, if it is a directory, all files in it (can be specified multiple times)',
    action='append', type='string', dest='logs')

So having a white list of files seems like the best idea to me.

On Fri, May 25, 2012 at 01:47:26PM -0000, Marc Deslauriers wrote:
> Michael,
Hi,

> Do you have an idea of the best way to fix this?

Urgh, nasty! Here is a potential fix:
=== modified file 'DistUpgrade/DistUpgradeApport.py'
--- DistUpgrade/DistUpgradeApport.py 2011-08-29 17:11:26 +0000
+++ DistUpgrade/DistUpgradeApport.py 2012-05-25 14:13:17 +0000
@@ -27,6 +27,9 @@
             f = os.path.join("/var/log/dist-upgrade",fname)
             if not os.path.isfile(f) or os.path.getsize(f) == 0:
                 continue
+ # never include system-state data
+ if "system_state" in f:
+ continue
             report[f.replace(".","").replace("-","")] = (open(f), )
         report.add_to_existing('/var/crash/_usr_bin_update-manager.0.crash')
     return True

But let me actually sit down and write a test case before it gets
commited.

Cheers,
 Michael

> Thanks!
>
> ** Changed in: update-manager (Ubuntu Natty)
> Assignee: (unassigned) => Marc Deslauriers (mdeslaur)
>
> ** Changed in: update-manager (Ubuntu Oneiric)
> Assignee: (unassigned) => Marc Deslauriers (mdeslaur)
>
> ** Changed in: update-manager (Ubuntu Precise)
> Assignee: (unassigned) => Marc Deslauriers (mdeslaur)
>
> ** Changed in: update-manager (Ubuntu Natty)
> Importance: Undecided => High
>
> ** Changed in: update-manager (Ubuntu Oneiric)
> Importance: Undecided => High
>
> ** Changed in: update-manager (Ubuntu Precise)
> Importance: Undecided => High
>
> ** Changed in: update-manager (Ubuntu Quantal)
> Importance: Undecided => High
>
> ** Changed in: update-manager (Ubuntu Quantal)
> Assignee: (unassigned) => Michael Vogt (mvo)
>
> ** Changed in: update-manager (Ubuntu Natty)
> Status: New => Confirmed
>
> ** Changed in: update-manager (Ubuntu Oneiric)
> Status: New => Confirmed
>
> ** Changed in: update-manager (Ubuntu Precise)
> Status: New => Confirmed
>
> ** Changed in: update-manager (Ubuntu Quantal)
> Status: New => Confirmed
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1004503
>
> Title:
> Incomplete fix for CVE-2012-0949
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/ubuntu/+source/update-manager/+bug/1004503/+subscriptions

Brian Murray (brian-murray) wrote :

I think it makes more sense to white list what we actually want to prevent issues like this in the future. I mean if we add code to update-manager to write another log file that ends up containing sensitive information we could end up with another CVE like this.

Michael Vogt (mvo) wrote :

On Fri, May 25, 2012 at 02:27:19PM -0000, Brian Murray wrote:
> I think it makes more sense to white list what we actually want to
> prevent issues like this in the future. I mean if we add code to
> update-manager to write another log file that ends up containing
> sensitive information we could end up with another CVE like this.

Yes, agreed on a whitelist approach, that makes more sense actually.

Cheers,
 Michael

Michael Vogt (mvo) wrote :

Please have a (extra careful) look at his updated version that uses a whitelist based approach which is indeed much preferable.

Marc Deslauriers (mdeslaur) wrote :

Michael,

apport_pkgfailure() in the same script also seems to need fixing:

def apport_pkgfailure(pkg, errormsg):
{snip}
    LOGDIR="/var/log/dist-upgrade/"
    s = "/usr/share/apport/package_hook"
{snip}
            p = subprocess.Popen([s,"-p",pkg,"-l",LOGDIR], stdin=subprocess.PIPE)

Brian Murray (brian-murray) wrote :

This is different from mvo's last patch in that lspci.log should be lspci.txt and screenlog.0 is included.

Additionally, I took care of apport_pkgfailure.

Michael Vogt (mvo) wrote :

Hey Brian, thanks for the updated patch. Unfortunately I'm unable to view it, I got "Processing Failed" from LP once and then some ansi charackters in there that look really odd. Could you please add it again, maybe something went wrong with LP.

Brian Murray (brian-murray) wrote :

Here is a non-cdiff'ed version of the patch.

Brian Murray (brian-murray) wrote :

Here is an updated patch taking into account passing popen a list wouldn't work to well.

Michael Vogt (mvo) wrote :

Thanks Brian for the update!

I put a new version of my patch there that includes a test for both functions that we changed, I also
simplified the _apport_append_logfiles() a bit as there is no need for a os.listdir() anymore now that
we only care about the whtielist names.

Marc Deslauriers (mdeslaur) wrote :

This new fix is CVE-2012-0950

visibility: private → public
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package update-manager - 1:0.156.14.5

---------------
update-manager (1:0.156.14.5) precise-security; urgency=low

  * SECURITY UPDATE: Incomplete fix for CVE-2012-0949 (LP: #1004503)
    - DistUpgrade/DistUpgradeApport.py: use a whitelist of files so we
      don't upload system_state archives.
    - tests/test_apport_crash.py: add test.
    - CVE-2012-0950
 -- Marc Deslauriers <email address hidden> Thu, 31 May 2012 13:05:04 -0400

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package update-manager - 1:0.152.25.12

---------------
update-manager (1:0.152.25.12) oneiric-security; urgency=low

  * SECURITY UPDATE: Incomplete fix for CVE-2012-0949 (LP: #1004503)
    - DistUpgrade/DistUpgradeApport.py: use a whitelist of files so we
      don't upload system_state archives.
    - tests/test_apport_crash.py: add test.
    - CVE-2012-0950
 -- Marc Deslauriers <email address hidden> Thu, 31 May 2012 13:07:39 -0400

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package update-manager - 1:0.150.5.4

---------------
update-manager (1:0.150.5.4) natty-security; urgency=low

  * SECURITY UPDATE: Incomplete fix for CVE-2012-0949 (LP: #1004503)
    - DistUpgrade/DistUpgradeApport.py: use a whitelist of files so we
      don't upload system_state archives.
    - tests/test_apport_crash.py: add test.
    - CVE-2012-0950
 -- Marc Deslauriers <email address hidden> Thu, 31 May 2012 13:10:34 -0400

Changed in update-manager (Ubuntu Natty):
status: Confirmed → Fix Released
Changed in update-manager (Ubuntu Oneiric):
status: Confirmed → Fix Released
Changed in update-manager (Ubuntu Precise):
status: Confirmed → Fix Released
tags: added: patch
jorge (jorgemonclova) on 2012-06-05
Changed in update-manager (Ubuntu Oneiric):
assignee: Marc Deslauriers (mdeslaur) → jorge (jorgemonclova)
Changed in update-manager (Ubuntu Oneiric):
assignee: jorge (jorgemonclova) → Marc Deslauriers (mdeslaur)
Brian Murray (brian-murray) wrote :

The code in question was split out into ubuntu-release-upgrader and was fixed with the following upload:

ubuntu-release-upgrader (1:0.174) quantal-proposed; urgency=low

  * DistUpgrade/DistUpgradeApport.py: use a whitelist to ensure that only
    specified files are gathered when creating an apport crash report
    (LP: #1004503)

 -- Brian Murray <email address hidden> Wed, 25 Jul 2012 12:06:06 -0700

Changed in update-manager (Ubuntu Quantal):
assignee: Michael Vogt (mvo) → Brian Murray (brian-murray)
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public Security information  Edit
Everyone can see this security related information.

Other bug subscribers