Ubuntu
xdiagnose package

xdiagnose has a symlink attack due to improperly named file in /tmp

Bug #1036211 reported by Alec Warner
264
This bug affects 1 person
Affects Status Importance Assigned to Milestone
xdiagnose (Ubuntu)
Fix Released
Low
Bryce Harrington
Precise
Fix Released
Low
Jamie Strandboge
Quantal
Fix Released
Low
Bryce Harrington

Bug Description

The code already says it, mktemp should be used to direct the output of the commands to a secure location instead of /tmp .

/usr/lib/python2.7/dist-packages/xdiagnose/welcome.py

   def on_make_archive(self, widget):
       xorg_backup_name = "failsafeX-backup-${timestamp}"
       xorg_backup_dir = "/tmp" # TODO: $(mktemp -d -t ${xorg_backup_name}.XXX)
       xorg_backup_file = "/var/log/%s.tar" %(xorg_backup_name)

       shutils.copy("/etc/X11/xorg.conf", xorg_backup_dir)
       shutils.copy("/var/log/Xorg.0.log", xorg_backup_dir)
       shutils.copy("/var/log/Xorg.0.log.old", xorg_backup_dir)
       shutils.copytree("/var/log/gdm", xorg_backup_dir)
       shutils.copytree("/var/log/lightdm", xorg_backup_dir)
       execute("lspci -vvnn > %s/lspci-vvnn.txt" %(xorg_backup_dir))
       execute("xrandr --verbose > %s/xrandr-verbose.txt" %(xorg_backup_dir))
       execute("tar -cf %s %s" %(xorg_backup_file, xorg_backup_dir))
       shutils.rmtree(xorg_backup_dir)

Tags: patch
Revision history for this message
Alec Warner (antarus) wrote :
Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Thank you for using Ubuntu and reporting a bug. Ubuntu's kernel hardening should prevent attacks against xdiagnose. So I'll mark this as Low for now. Subscribing Bryce.

Changed in xdiagnose (Ubuntu):
status: New → Triaged
importance: Undecided → Low
assignee: nobody → Bryce Harrington (bryce)
Revision history for this message
Thomas Bushnell, BSG (tbushnell) wrote :

Please raise the priority! It's not clear at all why we should assume that kernel hardening will guarantee that no path writable by the process in a symlink attack will be harmful. And I don't believe there is any apparmor rules for this in any case.

Because it's a security issue, it should be fixed, and not just postponed until some other security task is done.

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

Bryce can you comment on the patch? As written, it won't clean up after itself and there is a TODO in the code to implement this in the way Alec suggested. I'm not sure why it wasn't implemented like this to begin with if there wasn't some cleanup or other reason that needed to also be done.

Revision history for this message
Bryce Harrington (bryce) wrote :

Yeah, sheer laziness on my part.

This bit of code is actually exceedingly unimportant. The tarball that is generated is purely for the user's use and is not uploaded anywhere or made use of in any form by xdiagnose. It was a community contribution from back in the days before we had a decent X apport hook. Now days we'd just have them file a bug via ubuntu-bug xorg. I suppose users might not have apport turned on or something, so this might conceivably still be of some use, however I see the function's begun to bitrot: it's looking for gdm rather than lightdm.

So, I think the right thing to do here is rather than fix the temp file, to just excise the functionality and focus on apport as the way to gather troubleshooting info.

Revision history for this message
Bryce Harrington (bryce) wrote :

Note that the original patch is against welcome.py, which isn't used by anything. The code is a partial rewrite of Failsafe-X into python, but it's still not quite done so I haven't switched over to it. And like I mentioned, that particular functionality is obsolete by apport now so should just go.

I've posted three commits to xdiagnose trunk:

http://bazaar.launchpad.net/~bryce/xdiagnose/trunk/revision/307
 - This one just gets rid of the code from welcome.py. This is probably adequate to eliminate the security issue. Since like I said, this is not user facing at all, it causes no functional loss and thus should be no risk to backport to precise.

http://bazaar.launchpad.net/~bryce/xdiagnose/trunk/revision/306
 - This drops the same functionality from the actual failsafe-X code that is user facing. This particular chunk of code doesn't have the security flaw (it calls mktemp properly). So not really a need to backport this. But you're welcome to if you'd like. I've tested that failsafe-x still works properly with this removed.

http://bazaar.launchpad.net/~bryce/xdiagnose/trunk/revision/308
 - Changelog updates for above two changes.

jdstrand, go ahead and pick what you want from the above for past releases.
I'll roll the above out to quantal either today or tomorrow.

Changed in xdiagnose (Ubuntu):
status: Triaged → Fix Committed
Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Code is not present in 11.10 and earlier.

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

As the commit was made publicly, marking the bug public. I will push this out to 12.04 LTS tomorrow. Bryce, feel free to upload to 12.10.

visibility: private → public
Changed in xdiagnose (Ubuntu Precise):
status: New → In Progress
importance: Undecided → Low
assignee: nobody → Jamie Strandboge (jdstrand)
Jamie Strandboge (jdstrand)
Changed in xdiagnose (Ubuntu Precise):
status: In Progress → Fix Committed
Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "Use tempfile.mkdtemp to make a secure temp directory" of this bug report has been identified as being a patch. The ubuntu-reviewers team has been subscribed to the bug report so that they can review the patch. In the event that this is in fact not a patch you can resolve this situation by removing the tag 'patch' from the bug report and editing the attachment so that it is not flagged as a patch. Additionally, if you are member of the ubuntu-reviewers team please also unsubscribe the team from this bug report.

[This is an automated message performed by a Launchpad user owned by Brian Murray. Please contact him regarding any issues with the action taken in this bug report.]

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

This bug was fixed in the package xdiagnose - 3.2

---------------
xdiagnose (3.2) quantal; urgency=low

  * bin/*: When run within trunk, use trunk modules
  * failsafe-x: Remove the (obsolete) archive generation functionality.
    Users should use `ubuntu-bug xorg` instead in this case.
    (LP: #1036211)
  * control: Depend on python3-apport since apport hooks have been
    converted to python3.
    (LP: #1056837)
  * apport/source_xorg.py: Don't prompt with support questions for
    auto-collected crash bugs.
    (LP: #1036114)
 -- Bryce Harrington <email address hidden> Tue, 25 Sep 2012 11:16:30 -0700

Changed in xdiagnose (Ubuntu Quantal):
status: Fix Committed → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package xdiagnose - 2.5.2ubuntu0.1

---------------
xdiagnose (2.5.2ubuntu0.1) precise-security; urgency=low

  * SECURITY UPDATE: fix insecure temporary file creation
    - xdiagnose/welcome.py: remove 'Archive' option and on_make_archive()
      as people should be using 'ubuntu-bug xorg' anyway. Patch thanks
      to Bryce Harrington.
    - CVE-2012-XXXX
    - LP: #1036211
 -- Jamie Strandboge <email address hidden> Mon, 01 Oct 2012 17:04:28 -0500

Changed in xdiagnose (Ubuntu Precise):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.
Subscribing...

Other bug subscribers

Remote bug watches

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