Merge lp:~mandel/ubuntu-download-manager/typos-logs-unlock-not-locked into lp:ubuntu-download-manager

[Muhamad Hafizie Bin Ruzaimi]

=== modified file 'ubuntu-download-manager-priv/downloads/file_download.cpp'
--- ubuntu-download-manager-priv/downloads/file_download.cpp 2014-03-01 15:46:48 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.cpp 2014-03-05 09:11:58 +0000
@@ -30,7 +30,7 @@
 #include "system/logger.h"
 #include "system/network_reply.h"

-#define DOWN_LOG(LEVEL) LOG(LEVEL) << ((parent() != nullptr)?"GroupDownload{" + parent()->objectName() + "} ":"") << "Download ID{" << objectName() << "}"
+#define DOWN_LOG(LEVEL) LOG(LEVEL) << ((parent() != nullptr)?"GroupDownload {" + parent()->objectName() + " } ":"") << "Download ID{" << objectName() << " } "

 namespace {
@@ -117,7 +117,7 @@
     cleanUpCurrentData();
     // unlock the file name so that other downloads can use it it if is
     // no used in the file system
- _fileNameMutex->unlockFileName(_filePath);
+ unlockFilePath();
     _downloading = false;
     emit canceled(true);
 }
@@ -623,19 +623,34 @@
 FileDownload::emitFinished() {
     auto fileMan = FileManager::instance();

- LOG(INFO) << "EMIT finished" << filePath();
- setState(Download::FINISH);
-
     if (fileMan->exists(_tempFilePath)) {
- LOG(INFO) << "Rename '" << _tempFilePath << "' to '"
+ DOWN_LOG(INFO) << "Rename '" << _tempFilePath << "' to '"
             << _filePath << "'";
- fileMan->rename(_tempFilePath, _filePath);
+ QFile tempFile(_tempFilePath);
+ auto r = tempFile.rename(_filePath);
+ if (!r) {
+ DOWN_LOG(WARNING) << "Could not rename '" << _tempFilePath << "' to '"
+ << _filePath << "' due to " << tempFile.errorString();
+ }
     }
- _fileNameMutex->unlockFileName(_filePath);
+
+ setState(Download::FINISH);
+ unlockFilePath();
+
+ DOWN_LOG(INFO) << "EMIT finished" << filePath();
     emit finished(_filePath);
 }

 void
+FileDownload::unlockFilePath() {
+ if (!isConfined() && metadata().contains(Metadata::LOCAL_PATH_KEY)) {
+ _fileNameMutex->unlockFileName(_tempFilePath);
+ } else {
+ _fileNameMutex->unlockFileName(_filePath);
+ }
+}
+
+void
 FileDownload::cleanUpCurrentData() {
     bool success = true;
     QFile::FileError error = QFile::NoError;
@@ -685,7 +700,7 @@
     _reply = nullptr;
     cleanUpCurrentData();
     // let other downloads use the same file name
- _fileNameMutex->unlockFileName(_filePath);
+ unlockFilePath();
     Download::emitError(error);
 }

=== modified file 'ubuntu-download-manager-priv/downloads/file_download.h'
--- ubuntu-download-manager-priv/downloads/file_download.h 2014-02-22 10:41:59 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.h 2014-03-05 09:11:58 +0000
@@ -122,6 +122,7 @@
     void onOnlineStateChanged(bool);
     void initFileNames();
     void emitFinished();
+ void unlockFilePath();

  private:
     bool _downloading = false;

=== modified file 'ubuntu-download-manager-priv/downloads/group_download.cpp'
--- ubuntu-download-manager-priv/downloads/group_download.cpp 2014-02-20 20:13:28 +0000
+++ ubuntu-download-manager-priv/downloads/group_download.cpp 2014-03-05 09:11:58 +0000
@@ -16,6 +16,8 @@
  * boston, ma 02110-1301, usa.
  */

+#include <QDir>
+#include <QFileInfo>
 #include <ubuntu/download_manager/metadata.h>
 #include <ubuntu/download_manager/system/hash_algorithm.h>
 #include "downloads/download_adaptor.h"
@@ -24,7 +26,7 @@
 #include "system/logger.h"
 #include "system/uuid_factory.h"

-#define GROUP_LOG(LEVEL) LOG(LEVEL) << "Group Download{" << objectName() << "}"
+#define GROUP_LOG(LEVEL) LOG(LEVEL) << "Group Download {" << objectName() << " } "

 namespace Ubuntu {

@@ -167,6 +169,7 @@
 GroupDownload::cancelDownload() {
     TRACE;
     cancelAllDownloads();
+ GROUP_LOG(INFO) << "EMIT canceled";
     emit canceled(true);
 }

@@ -181,6 +184,7 @@
             download->pauseDownload();
         }
     }
+ GROUP_LOG(INFO) << "EMIT paused";
     emit paused(true);
 }

@@ -193,6 +197,7 @@
             download->resumeDownload();
         }
     }
+ GROUP_LOG(INFO) << "EMIT resumed";
     emit resumed(true);
 }

@@ -206,9 +211,12 @@
                 download->startDownload();
             }
         }
+ GROUP_LOG(INFO) << "EMIT started";
         emit started(true);
     } else {
+ GROUP_LOG(INFO) << "EMIT started";
         emit started(true);
+ GROUP_LOG(INFO) << "EMIT finished " << _finishedDownloads.join(";");
         emit finished(_finishedDownloads);
     }
 }
@@ -257,6 +265,8 @@
     // files that we managed to download
     cancelAllDownloads();
     QString errorMsg = down->url().toString() + ":" + error;
+
+ GROUP_LOG(INFO) << "EMIT error " << errorMsg;
     emitError(errorMsg);
 }

@@ -351,6 +361,17 @@
     // that downloads we are done :)
     if (_downloads.count() == _finishedDownloads.count()) {
         setState(Download::FINISH);
+#ifndef NDEBUG
+ foreach(const QString& file, _finishedDownloads) {
+ auto parentDir = QFileInfo(file).dir();
+ DLOG(INFO) << "Files for path dir '" << parentDir.absolutePath() << "' :";
+ auto children = parentDir.entryList();
+ foreach(const QString& child, children) {
+ DLOG(INFO) << "\t" << child;
+ }
+ }
+#endif
+ GROUP_LOG(INFO) << "EMIT finished " << _finishedDownloads.join(";");
         emit finished(_finishedDownloads);
     }
 }

=== modified file 'ubuntu-download-manager-priv/system/filename_mutex.cpp'
--- ubuntu-download-manager-priv/system/filename_mutex.cpp 2014-02-22 10:41:59 +0000
+++ ubuntu-download-manager-priv/system/filename_mutex.cpp 2014-03-05 09:11:58 +0000
@@ -82,7 +82,7 @@
     _mutex.lock();
     auto removed = _paths.remove(filename);
     if (!removed) {
- LOG(WARNING) << "Tired to remove filename '" << filename
+ LOG(WARNING) << "Tried to remove filename '" << filename
             << "' when it was not owned by any object.";
     } else {
         LOG(INFO) << "Released path '" << filename << "'";

=== modified file 'ubuntu-download-manager-tests/downloads/test_download.cpp'
--- ubuntu-download-manager-tests/downloads/test_download.cpp 2014-03-01 15:46:48 +0000
+++ ubuntu-download-manager-tests/downloads/test_download.cpp 2014-03-05 09:11:58 +0000
@@ -2285,8 +2285,14 @@
     QCOMPARE(2, calledMethods.count());
     auto methodName = calledMethods[0].methodName();
     QCOMPARE(QString("lockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
     methodName = calledMethods[1].methodName();
     QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
 }

 void
@@ -2307,8 +2313,14 @@
     QCOMPARE(2, calledMethods.count());
     auto methodName = calledMethods[0].methodName();
     QCOMPARE(QString("lockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
     methodName = calledMethods[1].methodName();
     QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
 }

 void
@@ -2336,8 +2348,14 @@
     QCOMPARE(2, calledMethods.count());
     auto methodName = calledMethods[0].methodName();
     QCOMPARE(QString("lockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
     methodName = calledMethods[1].methodName();
     QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
 }

 void
@@ -2378,8 +2396,14 @@
     QCOMPARE(2, calledMethods.count());
     auto methodName = calledMethods[0].methodName();
     QCOMPARE(QString("lockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
     methodName = calledMethods[1].methodName();
     QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
 }

 void
@@ -2412,6 +2436,177 @@
     QCOMPARE(2, calledMethods.count());
     auto methodName = calledMethods[0].methodName();
     QCOMPARE(QString("lockFileName"), methodName);
- methodName = calledMethods[1].methodName();
- QCOMPARE(QString("unlockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
+ methodName = calledMethods[1].methodName();
+ QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath(), path);
+}
+
+void
+TestDownload::testCancelUnlocksPathFromMetadata() {
+ QVariantMap metadata;
+ QString localPath = "/home/my/local/path";
+ metadata["local-path"] = localPath;
+
+ _fileNameMutex->record();
+ QScopedPointer<FileDownload> download(new FileDownload(_id, _path,
+ _isConfined, _rootPath, _url, metadata, _headers));
+ QSignalSpy spy(download.data(),
+ SIGNAL(canceled(bool))); // NOLINT(readability/function)
+
+ download->start(); // change state
+ download->startDownload();
+ download->cancel(); // change state
+ download->cancelDownload(); // method under test
+
+ // assert that the filename was correctly managed
+ auto calledMethods = _fileNameMutex->calledMethods();
+ QCOMPARE(2, calledMethods.count());
+ auto methodName = calledMethods[0].methodName();
+ QCOMPARE(QString("lockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath() + ".tmp", path);
+ methodName = calledMethods[1].methodName();
+ QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath() + ".tmp", path);
+}
+
+void
+TestDownload::testFinishUnlocksPathFromMetadata() {
+ QVariantMap metadata;
+ QString localPath = "/home/my/local/path";
+ metadata["local-path"] = localPath;
+
+ _fileNameMutex->record();
+ _reqFactory->record();
+ QScopedPointer<FileDownload> download(new FileDownload(_id, _path,
+ _isConfined, _rootPath, _url, metadata, _headers));
+ QSignalSpy spy(download.data(), SIGNAL(finished(QString)));
+
+ download->start(); // change state
+ download->startDownload();
+
+ QList<MethodData> calledMethods = _reqFactory->calledMethods();
+ QCOMPARE(1, calledMethods.count());
+ FakeNetworkReply* reply = reinterpret_cast<FakeNetworkReply*>(
+ calledMethods[0].params().outParams()[0]);
+
+ // emit the finish signal and expect it to be raised
+ emit reply->finished();
+ QCOMPARE(spy.count(), 1);
+ QCOMPARE(download->state(), Download::FINISH);
+
+ calledMethods = _fileNameMutex->calledMethods();
+ QCOMPARE(2, calledMethods.count());
+ auto methodName = calledMethods[0].methodName();
+ QCOMPARE(QString("lockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath() + ".tmp", path);
+ methodName = calledMethods[1].methodName();
+ QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath() + ".tmp", path);
+}
+
+void
+TestDownload::testProcessFinishUnlocksPathFromMetadata() {
+ _fileNameMutex->record();
+ _processFactory->record();
+ _reqFactory->record();
+
+ QString command = "cd";
+ QVariantMap metadata;
+ metadata["post-download-command"] = command;
+ QString localPath = "/home/my/local/path";
+ metadata["local-path"] = localPath;
+
+ QScopedPointer<FileDownload> download(new FileDownload(_id, _path,
+ _isConfined, _rootPath, _url, metadata, _headers));
+ QSignalSpy spy(download.data(), SIGNAL(finished(QString)));
+
+ download->start(); // change state
+ download->startDownload();
+
+ // we need to set the data before we pause!!!
+ QList<MethodData> calledMethods = _reqFactory->calledMethods();
+ QCOMPARE(1, calledMethods.count());
+ FakeNetworkReply* reply = reinterpret_cast<FakeNetworkReply*>(
+ calledMethods[0].params().outParams()[0]);
+
+ // makes the process to be executed
+ reply->emitFinished();
+
+ calledMethods = _processFactory->calledMethods();
+ QCOMPARE(1, calledMethods.count());
+ FakeProcess* process = reinterpret_cast<FakeProcess*>(
+ calledMethods[0].params().outParams()[0]);
+
+ process->emitFinished(0, QProcess::NormalExit);
+ QTRY_COMPARE(spy.count(), 1);
+
+ calledMethods = _fileNameMutex->calledMethods();
+ QCOMPARE(2, calledMethods.count());
+ auto methodName = calledMethods[0].methodName();
+ QCOMPARE(QString("lockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath() + ".tmp", path);
+ methodName = calledMethods[1].methodName();
+ QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath() + ".tmp" , path);
+}
+
+void
+TestDownload::testErrorUnlocksPathFromMetadata() {
+ QVariantMap metadata;
+ QString localPath = "/home/my/local/path";
+ metadata["local-path"] = localPath;
+
+ // fake an error and make sure that unlock is called
+ _fileNameMutex->record();
+ _reqFactory->record();
+ QScopedPointer<FileDownload> download(new FileDownload(_id, _path,
+ _isConfined, _rootPath, _url, metadata, _headers));
+ QSignalSpy errorSpy(download.data(), SIGNAL(error(QString)));
+
+ download->start(); // change state
+ download->startDownload();
+
+ // we need to set the data before we pause!!!
+ QList<MethodData> calledMethods = _reqFactory->calledMethods();
+ QCOMPARE(1, calledMethods.count());
+ FakeNetworkReply* reply = reinterpret_cast<FakeNetworkReply*>(
+ calledMethods[0].params().outParams()[0]);
+
+ // set the attrs in the reply so that we do raise two signals
+ reply->setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 404);
+ reply->setAttribute(QNetworkRequest::HttpReasonPhraseAttribute, "");
+
+ // emit the error and esure that the signals are raised
+ reply->emitHttpError(QNetworkReply::ContentAccessDenied);
+ QCOMPARE(errorSpy.count(), 1);
+
+ calledMethods = _fileNameMutex->calledMethods();
+ QCOMPARE(2, calledMethods.count());
+ auto methodName = calledMethods[0].methodName();
+ QCOMPARE(QString("lockFileName"), methodName);
+ auto path = qobject_cast<StringWrapper*>(
+ calledMethods[0].params().inParams()[0])->value();
+ QCOMPARE(download->filePath() + ".tmp", path);
+ methodName = calledMethods[1].methodName();
+ QCOMPARE(QString("unlockFileName"), methodName);
+ path = qobject_cast<StringWrapper*>(
+ calledMethods[1].params().inParams()[0])->value();
+ QCOMPARE(download->filePath() + ".tmp", path);
 }

=== modified file 'ubuntu-download-manager-tests/downloads/test_download.h'
--- ubuntu-download-manager-tests/downloads/test_download.h 2014-02-19 19:01:37 +0000
+++ ubuntu-download-manager-tests/downloads/test_download.h 2014-03-05 09:11:58 +0000
@@ -169,6 +169,10 @@
     void testFinishUnlocksPath();
     void testProcessFinishUnlocksPath();
     void testErrorUnlocksPath();
+ void testCancelUnlocksPathFromMetadata();
+ void testFinishUnlocksPathFromMetadata();
+ void testProcessFinishUnlocksPathFromMetadata();
+ void testErrorUnlocksPathFromMetadata();

  private:
     QString _id;

diff -Nru ubuntu-download-manager-1.2+16.04.20160322/debian/changelog ubuntu-download-manager-1.2+16.04.20160408/debian/changelog
--- ubuntu-download-manager-1.2+16.04.20160322/debian/changelog 2016-04-08 17:52:14.000000000 +0000
+++ ubuntu-download-manager-1.2+16.04.20160408/debian/changelog 2016-04-08 17:52:15.000000000 +0000
@@ -1,3 +1,10 @@
+ubuntu-download-manager (1.2+16.04.20160408-0ubuntu1) xenial; urgency=medium
+
+ * Disallow post-processing commands from unconfined apps (Fixes CVE-
+ 2016-1579)
+
+ -- Michael Sheldon <email address hidden> Fri, 08 Apr 2016 17:50:24 +0000
+
 ubuntu-download-manager (1.2+16.04.20160322-0ubuntu1) xenial; urgency=medium

   * Fix error reporting in download manager for network errors (LP:
diff -Nru ubuntu-download-manager-1.2+16.04.20160322/src/downloads/priv/ubuntu/downloads/file_download.cpp ubuntu-download-manager-1.2+16.04.20160408/src/downloads/priv/ubuntu/downloads/file_download.cpp
--- ubuntu-download-manager-1.2+16.04.20160322/src/downloads/priv/ubuntu/downloads/file_download.cpp 2016-03-22 16:24:29.000000000 +0000
+++ ubuntu-download-manager-1.2+16.04.20160408/src/downloads/priv/ubuntu/downloads/file_download.cpp 2016-04-08 17:50:22.000000000 +0000
@@ -1023,6 +1023,11 @@
         postDownloadProcess->start(command, args);
         return;
     } else if (_metadata.contains(Metadata::COMMAND_KEY)) {
+ if (isConfined()) {
+ DOWN_LOG(ERROR) << "Post processing commands are unavailable to confined applications";
+ emitError(COMMAND_ERROR);
+ return;
+ }
         // just emit processing if we DO NOT have a hash because else we
         // already emitted it.
         if (_hash.isEmpty()) {