Unsafe m_iconCacheDir handling may result in deletion of some files in home directory

Bug #874447 reported by Christoph Trassl on 2011-10-14
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
sni-qt
Fix Released
Critical
Aurélien Gâteau
sni-qt (Ubuntu)
Critical
Unassigned
Oneiric
Critical
Unassigned

Bug Description

Version: sni-qt_0.2.5-0ubuntu1_amd64

---- SRU info ----

[Impact]
Bug can cause removal of several files in the home directory of users if they run an application which uses sni-qt on a system with a non-writable temporary dir (/tmp or the dir pointed to by $TMPDIR).

See comment https://bugs.launchpad.net/ubuntu/+source/sni-qt/+bug/874447/comments/4 for more details on which files may be removed.

sni-qt creates a temp dir for its iconcache at startup. This temp dir is removed when the application quits as part of sni-qt cleanup code. This can cause some files in the home dir to be removed if the temp dir creation failed.

[Development Fix]
Bug has been fixed by:
- Checking the temporary dir was created before calling the FsUtils::recursiveRm() function.
- To avoid undefined behavior, sni-qt will also disable itself if it can't create its temp dir for the iconcache.

The fix is available here: http://bazaar.launchpad.net/~agateau/sni-qt/trunk/revision/91

[Test Case]
- Create a test user.
- List files from the home folder: find > /tmp/before
- Start an application using sni-qt, for example Clementine, like this: TMPDIR=/ clementine
- Quit the application with Ctrl+Q.
- List files again: find > /tmp/after
- Compare the two lists, some files are likely gone

[Regression Potential]
No regression expected.

---- Original report ----

The m_iconCacheDir variable is used unchecked throughout sni-qt. This may result in deletion of home directory.

This was found by an apparmor "DENIED" while running a sandboxed application (apparmor error data: operation="unlink" name="/home/user/.bash_history").

Lets assume we /tmp writes blocked and we have HOME/.* writes blocked.

1. statusnotifieritemfactory.cpp: FsUtils::generateTempDir is used to generate a temporary IconCache location.

[...]
StatusNotifierItemFactory::StatusNotifierItemFactory()
: m_isAvailable(false)
{
    QString tempSubDir = QString("sni-qt_%1_%2")
        .arg(QCoreApplication::applicationFilePath().section('/', -1))
        .arg(QCoreApplication::applicationPid());
    m_iconCacheDir = FsUtils::generateTempDir(tempSubDir);
    [...]

2. fsutils.cpp: Returns an empty QString if generation of file system objects failed.

QString generateTempDir(const QString& prefix)
{
    QDir dir = QDir::temp();
    if (!dir.mkpath(".")) {
        qCritical("Failed to generate temporary file for prefix %s: could not create %s",
            qPrintable(prefix), qPrintable(dir.path()));
        return QString();
    }

    QString tmpl = QString("%1/%2-XXXXXX")
        .arg(dir.path())
        .arg(prefix);
    QByteArray ba = QFile::encodeName(tmpl);
    const char* name = mkdtemp(ba.data());
    if (!name) {
        qCritical("Failed to generate temporary file for prefix %s: %s",
            qPrintable(prefix), strerror(errno));
        return QString();
    }
    return QFile::encodeName(name);
}

3. statusnotifieritemfactory.cpp: generateTempDir may have returned an empty QString (or another invalid location), which we are using as m_iconCacheDir

[...]
    m_iconCacheDir = FsUtils::generateTempDir(tempSubDir);
    SNI_VAR(m_iconCacheDir);

    m_iconCache = new IconCache(m_iconCacheDir, this);
[...]

4. iconcache.cpp: IconCache will be created with an empty QString (or another invalid location). If directory creation fails there is an error message printed, but the error stays unchecked.

IconCache::IconCache(const QString& baseDir, QObject* parent)
: QObject(parent)
, m_themePath(baseDir + "/icons")
{
    QDir dir(baseDir);
    bool ok = dir.mkdir("icons");
    if (!ok) {
        qCritical("Could not create '%s' dir for SNI icon cache", qPrintable(m_themePath));
        m_themePath = QString();
        return;
    }
}

5. statusnotifieritemfactory.cpp: So when closing an application using sni-qt m_iconCacheDir is wiped.

 StatusNotifierItemFactory::~StatusNotifierItemFactory()
{
    SNI_DEBUG;
    FsUtils::recursiveRm(m_iconCacheDir);
}

Christoph Trassl (chtrassl) wrote :

There was a mistake in the assumptions. Please lets assume we have :
* /tmp writes blocked
* HOME/icons/ writes blocked
* HOME/.* writes blocked

Changed in sni-qt:
status: New → In Progress
importance: Undecided → Critical
assignee: nobody → Aurélien Gâteau (agateau)
Aurélien Gâteau (agateau) wrote :

Thanks for your report. I uploaded a package (version 0.2.6~bzr91) to the sni-qt ppa [1] to fix this bug. The package should be available in a few hours, can you try it and confirm it fixes the bug?

[1]: https://launchpad.net/~agateau/+archive/sni-qt

Aurélien Gâteau (agateau) wrote :

Fix has been released in 0.2.6

Changed in sni-qt:
status: In Progress → Fix Released
Aurélien Gâteau (agateau) wrote :

I made some real world testing of that issue with a test user by starting clementine as:

  TMPDIR=/ clementine

It turns out it does not rm the whole home directory: in recursiveRm(), the line dir.rmdir(dir.path()) fails because it calls the C function rmdir() with twice the path: if dir.path() is "./foo/bar", rmdir() is called with "./foo/bar/./foo/bar" which fails (this is visible when running the application with strace). At this point the recursive removal is stopped.

It is still a serious issue as it will remove all files it finds until it tries to backtrack in the tree. Luckily the algorithm is depth-first, if the hierarchy looks like this:

* a1.file
* a2/
** b1.file
** b2.file
** b3/
*** c1.file
*** c2.file
*** c3.file
** b4/
*** c4.file
*** c5.file
*** c6.file
** b5.file
* a3/
* a4.file
* a5/

Assuming the files are returned in the listed order, recursiveRm() will:
- delete a1.file
- go into a2/
- delete a2/b1.file and a2/b2.file
- go into a2/b3/
- delete c1.file, c2.file and c3.file
- try to rmdir a2/b3 but will fail because it will call rmdir("a2/b3/a2/b3")
- stops there

I am preparing a debdiff for an SRU

Aurélien Gâteau (agateau) wrote :

Attached a debdiff backporting r91 from sni-qt trunk.

description: updated
security vulnerability: yes → no
visibility: private → public
summary: - Unsafe m_iconCacheDir handling may result in deletion of home directory
+ Unsafe m_iconCacheDir handling may result in deletion of some files in
+ home directory
Martin Pitt (pitti) wrote :

Please reupload. The uploaded debdiff only changes debian/changelog: http://launchpadlibrarian.net/82983924/sni-qt_0.2.5-0ubuntu1_0.2.5-0ubuntu2.diff.gz

Hello Christoph, or anyone else affected,

Accepted sni-qt into oneiric-proposed, the package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in sni-qt (Ubuntu Oneiric):
status: New → Fix Committed
tags: added: verification-needed
Aurélien Gâteau (agateau) wrote :

Thanks Chris. I confirm the bug is fixed with the package in oneiric-proposed.

Martin Pitt (pitti) wrote :

As this is a serious data-loss bug, waiving the usual 7 day maturing period.

tags: added: verification-done
removed: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package sni-qt - 0.2.5-0ubuntu2

---------------
sni-qt (0.2.5-0ubuntu2) oneiric-proposed; urgency=high

  * Backport r91 from upstream trunk: prevent deletion of cwd if tmp dir is not
    writable (LP: #874447)
 -- Aurelien Gateau <email address hidden> Sat, 15 Oct 2011 16:08:57 +0200

Changed in sni-qt (Ubuntu):
status: New → Fix Released
Changed in sni-qt (Ubuntu Oneiric):
status: Fix Committed → Fix Released
Martin Pitt (pitti) on 2011-10-19
Changed in sni-qt (Ubuntu Oneiric):
importance: Undecided → Critical
Changed in sni-qt (Ubuntu):
importance: Undecided → Critical

Le 19/10/2011 18:00, Martin Pitt a écrit :
> As this is a serious data-loss bug, waiving the usual 7 day maturing
> period.
>
> ** Tags removed: verification-needed
> ** Tags added: verification-done
>

Thanks for this, Martin.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers