if file is inaccessible for _apt, disable privilege drop in acquire

We had a very similar method previously for our own private usage, but
with some generalisation we can move this check into the acquire system
proper so that all frontends profit from this compatibility change.

As we are disabling a security feature here a warning is issued and
frontends are advised to consider reworking their download logic if
possible.

Note that this is implemented as an all or nothing situation: We can't
just (not) drop privileges for a subset of the files in a fetcher, so in
case you have to download some files with and some without you need to
use two fetchers.

diff --git a/apt-pkg/acquire.cc b/apt-pkg/acquire.cc
index f8b0773675b4cc9d85dffbf00a4985a80c95696e..cb32e8f2ba2e6dda4623d1e3c0133517f34db94a 100644 (file)
--- a/apt-pkg/acquire.cc
+++ b/apt-pkg/acquire.cc
@@ -34,6 +34,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <fcntl.h>
 #include <pwd.h>
 #include <grp.h>
 #include <dirent.h>
@@ -446,8 +447,58 @@ void pkgAcquire::RunFds(fd_set *RSet,fd_set *WSet)
 /* This runs the queues. It manages a select loop for all of the
    Worker tasks. The workers interact with the queues and items to
    manage the actual fetch. */
+static void CheckDropPrivsMustBeDisabled(pkgAcquire const &Fetcher)
+{
+   if(getuid() != 0)
+      return;
+
+   std::string SandboxUser = _config->Find("APT::Sandbox::User");
+   if (SandboxUser.empty())
+      return;
+
+   struct passwd const * const pw = getpwnam(SandboxUser.c_str());
+   if (pw == NULL)
+      return;
+
+   if (setegid(pw->pw_gid) != 0)
+      _error->Errno("setegid", "setegid %u failed", pw->pw_gid);
+   if (seteuid(pw->pw_uid) != 0)
+      _error->Errno("seteuid", "seteuid %u failed", pw->pw_uid);
+
+   bool dropPrivs = true;
+   for (pkgAcquire::ItemCIterator I = Fetcher.ItemsBegin();
+       I != Fetcher.ItemsEnd() && dropPrivs == true; ++I)
+   {
+      if ((*I)->DestFile.empty())
+        continue;
+
+      // we check directory instead of file as the file might or might not
+      // exist already as a link or not which complicates everything…
+      std::string dirname = flNotFile((*I)->DestFile);
+      if (DirectoryExists(dirname))
+        ;
+      else
+        continue; // assume it is created correctly by the acquire system
+
+      if (faccessat(AT_FDCWD, dirname.c_str(), R_OK | W_OK | X_OK, AT_EACCESS | AT_SYMLINK_NOFOLLOW) != 0)
+      {
+        dropPrivs = false;
+        _error->WarningE("pkgAcquire::Run", _("Can't drop privileges for downloading as file '%s' couldn't be accessed by user '%s'."),
+              (*I)->DestFile.c_str(), SandboxUser.c_str());
+        _config->Set("APT::Sandbox::User", "");
+        break;
+      }
+   }
+
+   if (seteuid(0) != 0)
+      _error->Errno("seteuid", "seteuid %u failed", 0);
+   if (setegid(0) != 0)
+      _error->Errno("setegid", "setegid %u failed", 0);
+}
 pkgAcquire::RunResult pkgAcquire::Run(int PulseIntervall)
 {
+   CheckDropPrivsMustBeDisabled(*this);
+
    Running = true;
    
    for (Queue *I = Queues; I != 0; I = I->Next)

diff --git a/apt-pkg/acquire.h b/apt-pkg/acquire.h
index 10025a6ef8c504e70ffb5f9b51aed553cfa40c3c..3e5ca41cdb88eb1aed67e17b04a8cd0424300a92 100644 (file)
--- a/apt-pkg/acquire.h
+++ b/apt-pkg/acquire.h
@@ -303,9 +303,11 @@ class pkgAcquire
 
    /** \brief Get the head of the list of items. */
    inline ItemIterator ItemsBegin() {return Items.begin();};
+   inline ItemCIterator ItemsBegin() const {return Items.begin();};
 
    /** \brief Get the end iterator of the list of items. */
    inline ItemIterator ItemsEnd() {return Items.end();};
+   inline ItemCIterator ItemsEnd() const {return Items.end();};
    
    // Iterate over queued Item URIs
    class UriIterator;

diff --git a/apt-private/private-download.cc b/apt-private/private-download.cc
index 18a9b1fbce23429b63bf8f956c868d6e8d7c3cfe..8a57ccc869d2d21470d75c5d657f6d6fc9b56f05 100644 (file)
--- a/apt-private/private-download.cc
+++ b/apt-private/private-download.cc
@@ -26,55 +26,6 @@
 #include <apti18n.h>
                                                                        /*}}}*/
 
-bool CheckDropPrivsMustBeDisabled(pkgAcquire &Fetcher)                 /*{{{*/
-{
-   // no need/possibility to drop privs
-   if(getuid() != 0)
-      return true;
-
-   // the user does not want to drop privs
-   std::string SandboxUser = _config->Find("APT::Sandbox::User");
-   if (SandboxUser.empty())
-      return true;
-
-   struct passwd const * const pw = getpwnam(SandboxUser.c_str());
-   if (pw == NULL)
-      return true;
-
-   if (seteuid(pw->pw_uid) != 0)
-      return _error->Errno("seteuid", "seteuid %u failed", pw->pw_uid);
-
-   bool res = true;
-   // check if we can write to destfile
-   for (pkgAcquire::ItemIterator I = Fetcher.ItemsBegin();
-       I != Fetcher.ItemsEnd() && res == true; ++I)
-   {
-      if ((*I)->DestFile.empty())
-        continue;
-      // we assume that an existing (partial) file means that we have sufficient rights
-      if (RealFileExists((*I)->DestFile))
-        continue;
-      int fd = open((*I)->DestFile.c_str(), O_CREAT | O_EXCL | O_RDWR, 0600);
-      if (fd < 0)
-      {
-        res = false;
-        std::string msg;
-        strprintf(msg, _("Can't drop privileges for downloading as file '%s' couldn't be accessed by user '%s'."),
-              (*I)->DestFile.c_str(), SandboxUser.c_str());
-        std::cerr << "W: " << msg << std::endl;
-        _config->Set("APT::Sandbox::User", "");
-        break;
-      }
-      unlink((*I)->DestFile.c_str());
-      close(fd);
-   }
-
-   if (seteuid(0) != 0)
-      return _error->Errno("seteuid", "seteuid %u failed", 0);
-
-   return res;
-}
-                                                                       /*}}}*/
 // CheckAuth - check if each download comes form a trusted source      /*{{{*/
 bool CheckAuth(pkgAcquire& Fetcher, bool const PromptUser)
 {

diff --git a/apt-private/private-download.h b/apt-private/private-download.h
index 0f3db5e7a0f67451f07a0a55a5cc537c3a963abf..80643e0f2e846522f943cf65b0458fca1e021d0a 100644 (file)
--- a/apt-private/private-download.h
+++ b/apt-private/private-download.h
@@ -8,8 +8,6 @@
 
 class pkgAcquire;
 
-APT_PUBLIC bool CheckDropPrivsMustBeDisabled(pkgAcquire &Fetcher);
-
 // Check if all files in the fetcher are authenticated
 APT_PUBLIC bool CheckAuth(pkgAcquire& Fetcher, bool const PromptUser);
 

diff --git a/cmdline/apt-get.cc b/cmdline/apt-get.cc
index d3b3da240b7b00c786d7c1fd4e7d95dd9c619361..ebc8c94c27dbc1f4378327ce86eea2262e300b1a 100644 (file)
--- a/cmdline/apt-get.cc
+++ b/cmdline/apt-get.cc
@@ -629,9 +629,6 @@ static bool DoDownload(CommandLine &CmdL)
       return true;
    }
 
-   // Disable drop-privs if "_apt" can not write to the target dir
-   CheckDropPrivsMustBeDisabled(Fetcher);
-
    if (_error->PendingError() == true || CheckAuth(Fetcher, false) == false)
       return false;
 
@@ -850,9 +847,6 @@ static bool DoSource(CommandLine &CmdL)
       return true;
    }
 
-   // Disable drop-privs if "_apt" can not write to the target dir
-   CheckDropPrivsMustBeDisabled(Fetcher);
-
    // check authentication status of the source as well
    if (UntrustedList.empty() == false && AuthPrompt(UntrustedList, false) == false)
       return false;
@@ -1403,8 +1397,6 @@ static bool DoChangelog(CommandLine &CmdL)
 
    if (printOnly == false)
    {
-      // Note: CheckDropPrivsMustBeDisabled isn't needed here as the download happens in a dedicated tempdir
-
       bool Failed = false;
       if (AcquireRun(Fetcher, 0, &Failed, NULL) == false || Failed == true)
         return false;

diff --git a/cmdline/apt-helper.cc b/cmdline/apt-helper.cc
index d235ac315f069e14c1d6c16e6edcadaad181a003..3c49bf1492fe9e8a8c5a8275d2285c2ab4f2d479 100644 (file)
--- a/cmdline/apt-helper.cc
+++ b/cmdline/apt-helper.cc
@@ -68,9 +68,6 @@ static bool DoDownloadFile(CommandLine &CmdL)
       fileind += 3;
    }
 
-   // Disable drop-privs if "_apt" can not write to the target dir
-   CheckDropPrivsMustBeDisabled(Fetcher);
-
    bool Failed = false;
    if (AcquireRun(Fetcher, 0, &Failed, NULL) == false || Failed == true)
       return _error->Error(_("Download Failed"));

diff --git a/test/integration/test-acquire-same-file-multiple-times b/test/integration/test-acquire-same-file-multiple-times
index 5267655215c92dd7f0b3f77976a4b2bed3296a12..a850f897c769fd816fb86c19210b14cefcb75a57 100755 (executable)
--- a/test/integration/test-acquire-same-file-multiple-times
+++ b/test/integration/test-acquire-same-file-multiple-times
@@ -14,10 +14,9 @@ filedown() {
        msgtest 'Downloading the same URI twice over file' "$1"
        testsuccess --nomsg apthelper download-file file:///$APTARCHIVE/foo ./downloaded/foo1 '' file:///$APTARCHIVE/foo ./downloaded/foo2 '' -o Debug::pkgAcquire::Worker=1
        cp rootdir/tmp/testsuccess.output download.log
-       #cat download.log
        testsuccess cmp $TESTFILE ./downloaded/foo1
        testsuccess cmp ./downloaded/foo1 ./downloaded/foo2
-       #testequal '1' grep -c '200%20URI%20Start' ./download.log
+       testequal '1' grep -c '200%20URI%20Start' ./download.log
        testequal '1' grep -c '201%20URI%20Done' ./download.log
        rm -f ./downloaded/foo1 ./downloaded/foo2
 }

diff --git a/test/integration/test-apt-get-changelog b/test/integration/test-apt-get-changelog
index 6ca05d0fa8ef4cdfcb752032dcbca9b02ebaa6a3..502920617a7cac0a803f6f523b7258841b39dba7 100755 (executable)
--- a/test/integration/test-apt-get-changelog
+++ b/test/integration/test-apt-get-changelog
@@ -47,8 +47,12 @@ testsuccessequal "'http://localhost:8080/main/f/foo/foo_1.0.changelog' foo.chang
 'http://localhost:8080/main/libb/libbar/libbar_1.0.changelog' libbar.changelog" aptget changelog foo libbar --print-uris -o Acquire::Changelogs::URI::Override::Label::Debian='http://localhost:8080/CHANGEPATH.changelog'
 
 releasechanger 'Changelogs' 'no'
-testequal 'E: Failed to fetch changelog:/foo.changelog  Changelog unavailable for foo=1.0
-' aptget changelog foo -qq -d
+if [ "$(id -u)" = '0' ]; then
+       testfailuremsg "W: Can't drop privileges for downloading as file 'foo.changelog' couldn't be accessed by user '_apt'. - pkgAcquire::Run (13: Permission denied)
+E: Failed to fetch changelog:/foo.changelog  Changelog unavailable for foo=1.0" aptget changelog foo -d
+else
+       testfailuremsg 'E: Failed to fetch changelog:/foo.changelog  Changelog unavailable for foo=1.0' aptget changelog foo -d
+fi
 
 sed -i '/^Changelogs: / d' $(find rootdir/var/lib/apt/lists -name '*Release')
 releasechanger 'Label' 'Testcases'