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

[Christoph Trassl]

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)]

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

[Aurélien Gâteau (agateau)]

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)]

Fix has been released in 0.2.6

[Aurélien Gâteau (agateau)]

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)]

Attached a debdiff backporting r91 from sni-qt trunk.

[Martin Pitt (pitti)]

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

[Chris Halse Rogers (raof)]

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!

[Aurélien Gâteau (agateau)]

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

[Martin Pitt (pitti)]

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

[Launchpad Janitor (janitor)]

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

[Aurélien Gâteau (agateau)]

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.