vulnerability: rewrite arbitrary user file

[Vasily Kulikov]

Binary package hint: conky

Hi, I've just discovered that conky is vulnerable to rewriting any user file:

char *getSkillname(const char *file, int skillid)
...
 if (!file_exists(file)) {
  skilltree = getXmlFromAPI(NULL, NULL, NULL, EVEURL_SKILLTREE);
  writeSkilltree(skilltree, file);
  free(skilltree);
 }

getXmlFromAPI() can be executed for a long time (e.g. bad connection), so between file_exists() and write_file() attacker can create link to any user file named "/tmp/.cesf". Attacker can choose the time when to create the link by watching for network connections.

Thanks.

[Kees Cook (kees)]

Thanks for taking the time to report this bug and helping to make Ubuntu better.

The latest release of Ubuntu is not vulnerable to symlink race attacks, but earlier releases will need fixing. https://wiki.ubuntu.com/Security/Features#symlink

Since the package referred to in this bug is in universe or multiverse, it is community maintained. If you are able, I suggest posting a debdiff for this issue. When a debdiff is available, members of the security team will review it and publish the package. See the following link for more information: https://wiki.ubuntu.com/SecurityTeam/UpdateProcedures

[garo (nikolas)]

I (as conky dev) added a fix for this in conky's git master branch (http://git.omp.am/?p=conky.git;a=commitdiff;h=ac4a3682aecb9d6466fea4aebb183b5f8f632905 ).
If you happen to find other bugs in conky in the future, could you also add them to our bugtracker ( http://sourceforge.net/tracker/?group_id=143975&atid=757308 ) ?
Thanks !

[Vasily Kulikov (segooon)]

Garo, as SF seems unresponsible for me I'll comment here. The patch is not full, it is still racy. There is still a period between file_exists() and fopen(). If conky is Linux only (I don't know) then the fix is a call to fopen() with "x" flag (glibc extension, not portable). Otherwise use open(2) with O_EXCL flag and either fdopen(3) + fwrite(3) + fclose(3) or write(2)+close(2).

[garo (nikolas)]

There are parts of conky that are linux-only but that are the parts that do things like examine hardware, the rest should work on everything POSIX-compatible.
There is idd still a period between file_exists() and fopen() but the only thing that happens in that time is a stat().
I am by no means a security expert so i could be wrong, but i don't see how a attacker could make that stat() hang long enough to create a symlink

[Vasily Kulikov (segooon)]

The thing is that an attacker shouldn't succeed every time, but he wants to succeed only once (defender's task is never ever allow anybody to do it). He may naively try to create & delete symlink in a loop or do some nontrivial steps to make the system scheduler to stop conky process exactly (ideally for an attacker) between stat and open calls. He may know scheduler heuristics (when it might change the running task) or even try to exploit another vulnerability, e.g. if he gains CAP_SYS_NICE then he may controll scheduler's behavior rather well. It's better not to think about the cases, but just safely use temp files (or not even use them at all - it's even better :-)).

Also please look at my patch at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=612033, I tried to make it small.

Thanks,
Vasiliy.

[garo (nikolas)]

Why are you setting the umask ----rw-rw- instead of -rw------- ?
It also seems that the original code (i have no idea who wrote the code for the eve-part of conky) overwrites the tempfile each time but since you are creating unique files this will cause the hard drive to be filled with junk, i think a "unlink(file);" after "doc = xmlReadFile(file, NULL, 0);" should fix this.

[garo (nikolas)]

(never mind the umask-part in my previous comment, i forgot something)

[garo (nikolas)]

I already pushed my version (with the unlink) on the master branch before i saw your version on the debian bugtracker, but it does exactly the same, the only difference is that i'm removing the file inside getSkillname() right after the reading is over and you let getSkillname() finish the parsing of the data it has read.

[Launchpad Janitor (janitor)]

This bug was fixed in the package conky - 1.8.1-2

---------------
conky (1.8.1-2) unstable; urgency=low

  * Add build dependency on libxnvctrl-dev | nvidia-settings to keep Conky
    in sync with Ubuntu.
  * Add debian/patches/fix-curl-ftbfs.patch to fix FTBFS with curl 7.21.7.
    (Closes: #636367)
  * Add debian/patches/fix-kfreebsd-ftbfs.patch to fix FTBFS on kfreebsd.

conky (1.8.1-1) unstable; urgency=low

  * Adopt package. (Closes: #632655)
  * Acknowledge NMU patch for #612033, which fixes a known vulnerability in
    Conky. Thanks to Luca Falavigna! (LP: #607309)
  * New upstream release. (Closes: #604921)
    - Fix bug in $if_existing. (Closes: #612904)
    - Fix various memory leaks. (Closes: #628527)
    - Fix battery_bar not parsing arguments correctly. (LP: #569195)
  * Fix ncurses being enabled for all binary packages (upstream default).
  * Rename "70b6f35a.patch" to "fix-race-condition.patch", and add a full
    DEP-3 formatted header.
  * Add debian/patches/fix-acpitemp.patch to let $acpitemp use /sys instead
    of /proc. (Closes: #609745, #628519; LP: #810667)
  * Change section of source package from "contrib/utils" to "utils".
    (Closes: #579102)
    - Change section of binary package conky-all to "contrib/utils".
    - Remove build-dependency on nvidia-settings; add build-dependency on
      libxnvctrl-dev instead to conky-all, due to the move of the
      NVCtrl/NVCtrl.h header file to libxnvctrl-dev.
    - Update debian/NEWS and debian/README.source to reflect the changes to
      Conky packaging in Debian.
  * Change dependency of transitional conky package to
    "conky-all | conky-std".
  * debian/copyright: Remove reference to deprecated BSD license file
    (/usr/share/common-licenses/BSD).
  * Change Vcs-* entries in debian/control to point to collab-maint.
  * Update Standards version from 3.8.4 to 3.9.2, no changes required.
  * Update debhelper compatiblity level from 5 to 8.

conky (1.8.0-1.1) unstable; urgency=medium

  * Non-maintainer upload.
  * debian/patches/70b6f35a.patch:
    - Cherrypick a patch from upstream to avoid rewriting an arbitrary
      user file (Closes: #612033).
 -- <email address hidden> (Tarun K. Mall) Wed, 10 Aug 2011 11:43:59 +0000