Incomplete fix for CVE-2012-0949

Michael,

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

Thanks!

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

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.

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

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

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)

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.

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.

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

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

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.

This new fix is CVE-2012-0950

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

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

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

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