From 5684f71fa0f6c1b765aa53e22ca3b024c578b9c9 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 6 Oct 2014 14:29:53 +0200 Subject: [PATCH] use _apt:root only for partial directories Using a different user for calling methods is intended to protect us from methods running amok (via remotely exploited bugs) by limiting what can be done by them. By using root:root for the final directories and just have the files in partial writeable by the methods we enhance this in sofar as a method can't modify already verified data in its parent directory anymore. As a side effect, this also clears most of the problems you could have if the final directories are shared without user-sharing or if these directories disappear as they are now again root owned and only the partial directories contain _apt owned files (usually none if apt isn't running) and the directory itself is autocreated with the right permissions. --- apt-pkg/acquire-item.cc | 133 ++++++++++++++---------- apt-pkg/acquire-item.h | 7 +- apt-pkg/acquire-worker.cc | 3 +- debian/apt.postinst | 15 +-- test/integration/framework | 22 +++- test/integration/test-apt-get-download | 16 +++ test/integration/test-apt-update-unauth | 20 ++-- 7 files changed, 137 insertions(+), 79 deletions(-) diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index ad9198cba..6bfd14992 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -44,6 +44,9 @@ #include #include #include +#include +#include +#include #include /*}}}*/ @@ -62,6 +65,30 @@ static void printHashSumComparision(std::string const &URI, HashStringList const std::cerr << "\t- " << hs->toStr() << std::endl; } /*}}}*/ +static void changeOwnerAndPermissionOfFile(char const * const requester, char const * const file, char const * const user, char const * const group, mode_t const mode) +{ + // ensure the file is owned by root and has good permissions + struct passwd const * const pw = getpwnam(user); + struct group const * const gr = getgrnam(group); + if (getuid() == 0) // if we aren't root, we can't chown, so don't try it + { + if (pw != NULL && gr != NULL && chown(file, pw->pw_uid, gr->gr_gid) != 0) + _error->WarningE(requester, "chown to %s:%s of file %s failed", user, group, file); + } + if (chmod(file, mode) != 0) + _error->WarningE(requester, "chmod 0%o of file %s failed", mode, file); +} +static std::string preparePartialFile(std::string const &file) +{ + std::string DestFile = _config->FindDir("Dir::State::lists") + "partial/"; + DestFile += file; + return DestFile; +} +static std::string preparePartialFileFromURI(std::string const &uri) +{ + return preparePartialFile(URItoFileName(uri)); +} + // Acquire::Item::Item - Constructor /*{{{*/ #if __GNUC__ >= 4 @@ -178,6 +205,21 @@ bool pkgAcquire::Item::Rename(string From,string To) return true; } /*}}}*/ + +void pkgAcquire::Item::QueueURI(ItemDesc &Item) +{ + if (access(DestFile.c_str(), R_OK) == 0) + { + changeOwnerAndPermissionOfFile("preparePartialFile", DestFile.c_str(), "_apt", "root", 0600); + std::cerr << "QUEUE ITEM: " << DestFile << std::endl; + } + Owner->Enqueue(Item); +} +void pkgAcquire::Item::Dequeue() +{ + Owner->Dequeue(this); +} + bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const error)/*{{{*/ { if(FileExists(DestFile)) @@ -293,8 +335,7 @@ pkgAcqDiffIndex::pkgAcqDiffIndex(pkgAcquire *Owner, Desc.ShortDesc = Target->ShortDesc; Desc.URI = Target->URI + ".diff/Index"; - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(Desc.URI); + DestFile = preparePartialFileFromURI(Desc.URI); if(Debug) std::clog << "pkgAcqDiffIndex: " << Desc.URI << std::endl; @@ -454,8 +495,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string IndexDiffFile) /*{{{*/ { // FIXME: make this use the method PackagesFileReadyInPartial = true; - std::string Partial = _config->FindDir("Dir::State::lists"); - Partial += "partial/" + URItoFileName(RealURI); + std::string const Partial = preparePartialFileFromURI(RealURI); FileFd From(CurrentPackagesFile, FileFd::ReadOnly); FileFd To(Partial, FileFd::WriteEmpty); @@ -585,9 +625,7 @@ pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *Owner, : pkgAcqBaseIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser), available_patches(diffs), ServerSha1(ServerSha1) { - - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(Target->URI); + DestFile = preparePartialFileFromURI(Target->URI); Debug = _config->FindB("Debug::pkgAcquire::Diffs",false); @@ -640,7 +678,7 @@ void pkgAcqIndexDiffs::Finish(bool allDone) } // queue for copy - PartialFile = _config->FindDir("Dir::State::lists")+"partial/"+URItoFileName(RealURI); + PartialFile = preparePartialFileFromURI(RealURI); DestFile = _config->FindDir("Dir::State::lists"); DestFile += URItoFileName(RealURI); @@ -671,8 +709,7 @@ void pkgAcqIndexDiffs::Finish(bool allDone) bool pkgAcqIndexDiffs::QueueNextDiff() /*{{{*/ { // calc sha1 of the just patched file - string FinalFile = _config->FindDir("Dir::State::lists"); - FinalFile += "partial/" + URItoFileName(RealURI); + std::string const FinalFile = preparePartialFileFromURI(RealURI); if(!FileExists(FinalFile)) { @@ -717,8 +754,7 @@ bool pkgAcqIndexDiffs::QueueNextDiff() /*{{{*/ // queue the right diff Desc.URI = RealURI + ".diff/" + available_patches[0].file + ".gz"; Desc.Description = Description + " " + available_patches[0].file + string(".pdiff"); - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(RealURI + ".diff/" + available_patches[0].file); + DestFile = preparePartialFileFromURI(RealURI + ".diff/" + available_patches[0].file); if(Debug) std::clog << "pkgAcqIndexDiffs::QueueNextDiff(): " << Desc.URI << std::endl; @@ -737,9 +773,7 @@ void pkgAcqIndexDiffs::Done(string Message,unsigned long long Size, HashStringLi Item::Done(Message, Size, Hashes, Cnf); // FIXME: verify this download too before feeding it to rred - - string FinalFile; - FinalFile = _config->FindDir("Dir::State::lists")+"partial/"+URItoFileName(RealURI); + std::string const FinalFile = preparePartialFileFromURI(RealURI); // success in downloading a diff, enter ApplyDiff state if(State == StateFetchDiff) @@ -800,10 +834,6 @@ pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire *Owner, : pkgAcqBaseIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser), patch(patch), allPatches(allPatches), State(StateFetchDiff) { - - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(Target->URI); - Debug = _config->FindB("Debug::pkgAcquire::Diffs",false); RealURI = Target->URI; @@ -813,8 +843,8 @@ pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire *Owner, Desc.URI = RealURI + ".diff/" + patch.file + ".gz"; Desc.Description = Description + " " + patch.file + string(".pdiff"); - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(RealURI + ".diff/" + patch.file); + + DestFile = preparePartialFileFromURI(RealURI + ".diff/" + patch.file); if(Debug) std::clog << "pkgAcqIndexMergeDiffs: " << Desc.URI << std::endl; @@ -852,8 +882,7 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri Item::Done(Message,Size,Hashes,Cnf); // FIXME: verify download before feeding it to rred - - string const FinalFile = _config->FindDir("Dir::State::lists") + "partial/" + URItoFileName(RealURI); + string const FinalFile = preparePartialFileFromURI(RealURI); if (State == StateFetchDiff) { @@ -909,8 +938,7 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri for (std::vector::const_iterator I = allPatches->begin(); I != allPatches->end(); ++I) { - std::string PartialFile = _config->FindDir("Dir::State::lists"); - PartialFile += "partial/" + URItoFileName(RealURI); + std::string const PartialFile = preparePartialFileFromURI(RealURI); std::string patch = PartialFile + ".ed." + (*I)->patch.file + ".gz"; std::cerr << patch << std::endl; unlink(patch.c_str()); @@ -1008,8 +1036,7 @@ void pkgAcqIndex::Init(string const &URI, string const &URIDesc, { Stage = STAGE_DOWNLOAD; - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(URI); + DestFile = preparePartialFileFromURI(URI); CurrentCompressionExtension = CompressionExtensions.substr(0, CompressionExtensions.find(' ')); if (CurrentCompressionExtension == "uncompressed") @@ -1126,8 +1153,7 @@ void pkgAcqIndex::ReverifyAfterIMS() { // update destfile to *not* include the compression extension when doing // a reverify (as its uncompressed on disk already) - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(RealURI); + DestFile = preparePartialFileFromURI(RealURI); // adjust DestFile if its compressed on disk if (_config->FindB("Acquire::GzipIndexes",false) == true) @@ -1253,8 +1279,7 @@ void pkgAcqIndex::StageDownloadDone(string Message, // If we have compressed indexes enabled, queue for hash verification if (_config->FindB("Acquire::GzipIndexes",false)) { - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(RealURI) + '.' + CurrentCompressionExtension; + DestFile = preparePartialFileFromURI(RealURI + '.' + CurrentCompressionExtension); EraseFileName = ""; Stage = STAGE_DECOMPRESS_AND_VERIFY; Desc.URI = "copy:" + FileName; @@ -1395,9 +1420,7 @@ void pkgAcqMetaBase::AbortTransaction() (*I)->Status = pkgAcquire::Item::StatDone; // kill files in partial - string PartialFile = _config->FindDir("Dir::State::lists"); - PartialFile += "partial/"; - PartialFile += flNotDir((*I)->DestFile); + std::string const PartialFile = preparePartialFile(flNotDir((*I)->DestFile)); if(FileExists(PartialFile)) Rename(PartialFile, PartialFile + ".FAILED"); } @@ -1428,19 +1451,18 @@ void pkgAcqMetaBase::CommitTransaction() { if((*I)->PartialFile != "") { - if(_config->FindB("Debug::Acquire::Transaction", false) == true) - std::clog << "mv " - << (*I)->PartialFile << " -> " - << (*I)->DestFile << " " - << (*I)->DescURI() - << std::endl; - Rename((*I)->PartialFile, (*I)->DestFile); - chmod((*I)->DestFile.c_str(),0644); + if(_config->FindB("Debug::Acquire::Transaction", false) == true) + std::clog << "mv " << (*I)->PartialFile << " -> "<< (*I)->DestFile << " " + << (*I)->DescURI() << std::endl; + + Rename((*I)->PartialFile, (*I)->DestFile); + changeOwnerAndPermissionOfFile("CommitTransaction", (*I)->DestFile.c_str(), "root", "root", 0644); + } else { if(_config->FindB("Debug::Acquire::Transaction", false) == true) - std::clog << "rm " + std::clog << "rm " << (*I)->DestFile - << " " + << " " << (*I)->DescURI() << std::endl; unlink((*I)->DestFile.c_str()); @@ -1625,8 +1647,7 @@ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/ // this ensures that any file in the lists/ dir is removed by the // transaction - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(RealURI); + DestFile = preparePartialFileFromURI(RealURI); TransactionManager->TransactionStageRemoval(this, DestFile); // only allow going further if the users explicitely wants it @@ -1684,8 +1705,7 @@ pkgAcqMetaIndex::pkgAcqMetaIndex(pkgAcquire *Owner, /*{{{*/ // pkgAcqMetaIndex::Init - Delayed constructor /*{{{*/ void pkgAcqMetaIndex::Init(std::string URIDesc, std::string ShortDesc) { - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; - DestFile += URItoFileName(RealURI); + DestFile = preparePartialFileFromURI(RealURI); // Create the item Desc.Description = URIDesc; @@ -1770,7 +1790,7 @@ bool pkgAcqMetaBase::CheckAuthDone(string Message, const string &RealURI) /*{{{* return true; } /*}}}*/ - /*{{{*/ +// pkgAcqMetaBase::QueueForSignatureVerify /*{{{*/ void pkgAcqMetaBase::QueueForSignatureVerify(const std::string &MetaIndexFile, const std::string &MetaIndexFileSignature) { @@ -1781,7 +1801,7 @@ void pkgAcqMetaBase::QueueForSignatureVerify(const std::string &MetaIndexFile, SetActiveSubprocess("gpgv"); } /*}}}*/ - /*{{{*/ +// pkgAcqMetaBase::CheckDownloadDone /*{{{*/ bool pkgAcqMetaBase::CheckDownloadDone(const std::string &Message, const std::string &RealURI) { @@ -2332,7 +2352,10 @@ bool pkgAcqArchive::QueueNext() if ((unsigned long long)Buf.st_size > Version->Size) unlink(DestFile.c_str()); else + { PartialSize = Buf.st_size; + changeOwnerAndPermissionOfFile("pkgAcqArchive::QueueNext", FinalFile.c_str(), "_apt", "root", 0600); + } } // Disables download of archives - useful if no real installation follows, @@ -2388,21 +2411,20 @@ void pkgAcqArchive::Done(string Message,unsigned long long Size, HashStringList return; } - Complete = true; - // Reference filename if (FileName != DestFile) { StoreFilename = DestFile = FileName; Local = true; + Complete = true; return; } - + // Done, move it into position string FinalFile = _config->FindDir("Dir::Cache::Archives"); FinalFile += flNotDir(StoreFilename); Rename(DestFile,FinalFile); - + changeOwnerAndPermissionOfFile("pkgAcqArchive::Done", FinalFile.c_str(), "root", "root", 0644); StoreFilename = DestFile = FinalFile; Complete = true; } @@ -2498,7 +2520,10 @@ pkgAcqFile::pkgAcqFile(pkgAcquire *Owner,string URI, HashStringList const &Hashe if ((Size > 0) && (unsigned long long)Buf.st_size > Size) unlink(DestFile.c_str()); else + { PartialSize = Buf.st_size; + changeOwnerAndPermissionOfFile("pkgAcqFile", DestFile.c_str(), "_apt", "root", 0600); + } } QueueURI(Desc); diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 02b8c13e8..a3388ca3e 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -75,12 +75,11 @@ class pkgAcquire::Item : public WeakPointable * \param Item Metadata about this item (its URI and * description). */ - inline void QueueURI(ItemDesc &Item) - {Owner->Enqueue(Item);}; + void QueueURI(ItemDesc &Item); /** \brief Remove this item from its owner's queue. */ - inline void Dequeue() {Owner->Dequeue(this);}; - + void Dequeue(); + /** \brief Rename a file without modifying its timestamp. * * Many item methods call this as their final action. diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc index 54be8e99f..4a357bdab 100644 --- a/apt-pkg/acquire-worker.cc +++ b/apt-pkg/acquire-worker.cc @@ -371,7 +371,8 @@ bool pkgAcquire::Worker::RunMessages() { if (Itm == 0) { - _error->Error("Method gave invalid 400 URI Failure message"); + std::string const msg = LookupTag(Message,"Message"); + _error->Error("Method gave invalid 400 URI Failure message: %s", msg.c_str()); break; } diff --git a/debian/apt.postinst b/debian/apt.postinst index 01f78a1dd..b8f3edbe5 100755 --- a/debian/apt.postinst +++ b/debian/apt.postinst @@ -35,12 +35,15 @@ case "$1" in fi fi - # add unprivileged user for the apt methods - adduser --force-badname --system -home /var/empty \ - --no-create-home --quiet _apt || true - chown -R _apt:root \ - /var/lib/apt/lists \ - /var/cache/apt/archives + # add unprivileged user for the apt methods + adduser --force-badname --system -home /var/empty \ + --no-create-home --quiet _apt || true + + # deal with upgrades from experimental + if dpkg --compare-versions "$2" 'eq' '1.1~exp3'; then + # libapt will setup partial/ at runtime + chown -R root:root /var/lib/apt/lists /var/cache/apt/archives || true + fi # ensure tighter permissons on the logs, see LP: #975199 if dpkg --compare-versions "$2" lt-nl 0.9.7.7; then diff --git a/test/integration/framework b/test/integration/framework index e83606fae..688a1abf2 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -164,9 +164,10 @@ addtrap() { setupenvironment() { TMPWORKINGDIRECTORY=$(mktemp -d) - TESTDIRECTORY=$(readlink -f $(dirname $0)) + addtrap "cd /; rm -rf $TMPWORKINGDIRECTORY;" msgninfo "Preparing environment for ${CCMD}$(basename $0)${CINFO} in ${TMPWORKINGDIRECTORY}… " + TESTDIRECTORY=$(readlink -f $(dirname $0)) # allow overriding the default BUILDDIR location BUILDDIRECTORY=${APT_INTEGRATION_TESTS_BUILD_DIR:-"${TESTDIRECTORY}/../../build/bin"} LIBRARYPATH=${APT_INTEGRATION_TESTS_LIBRARY_PATH:-"${BUILDDIRECTORY}"} @@ -177,7 +178,6 @@ setupenvironment() { test -x "${BUILDDIRECTORY}/apt-get" || msgdie "You need to build tree first" # ----- - addtrap "cd /; rm -rf $TMPWORKINGDIRECTORY;" cd $TMPWORKINGDIRECTORY mkdir rootdir aptarchive keys cd rootdir @@ -210,6 +210,7 @@ setupenvironment() { cp "${TESTDIRECTORY}/${SOURCESSFILE}" aptarchive/Sources fi cp $(find $TESTDIRECTORY -name '*.pub' -o -name '*.sec') keys/ + chmod 644 $(find keys -name '*.pub' -o -name '*.sec') ln -s ${TMPWORKINGDIRECTORY}/keys/joesixpack.pub rootdir/etc/apt/trusted.gpg.d/joesixpack.gpg echo "Dir \"${TMPWORKINGDIRECTORY}/rootdir\";" > aptconfig.conf echo "Dir::state::status \"${TMPWORKINGDIRECTORY}/rootdir/var/lib/dpkg/status\";" >> aptconfig.conf @@ -837,9 +838,7 @@ setupaptarchive() { fi signreleasefiles if [ "$1" != '--no-update' ]; then - msgninfo "\tSync APT's cache with the archive… " - aptget update -qq - msgdone "info" + testsuccess aptget update -o Debug::pkgAcquire::Worker=true -o Debug::Acquire::gpgv=true fi } @@ -1175,6 +1174,19 @@ testfailure() { fi } +testaccessrights() { + msgtest "Test that file $1 has access rights set to" "$2" + if [ "$2" = "$(stat --format '%a' "$1")" ]; then + msgpass + else + echo >&2 + ls -l >&2 "$1" + echo -n >&2 "stat(1) reports access rights: " + stat --format '%a' "$1" + msgfail + fi +} + testwebserverlaststatuscode() { local DOWNLOG='rootdir/tmp/webserverstatus-testfile.log' local STATUS='rootdir/tmp/webserverstatus-statusfile.log' diff --git a/test/integration/test-apt-get-download b/test/integration/test-apt-get-download index 58ed44f8f..0514542b3 100755 --- a/test/integration/test-apt-get-download +++ b/test/integration/test-apt-get-download @@ -11,8 +11,23 @@ buildsimplenativepackage 'apt' 'all' '1.0' 'stable' buildsimplenativepackage 'apt' 'all' '2.0' 'unstable' insertinstalledpackage 'vrms' 'all' '1.0' +umask 0027 + setupaptarchive +# apt-ftparchive knows how to chmod files +find aptarchive/dists -name '*Packages*' -type f | while read file; do + testaccessrights "$file" '644' +done +# created by the framework without special care +find aptarchive/dists -name '*Release*' -type f | while read file; do + testaccessrights "$file" '640' +done +# all copied files are properly chmodded +find rootdir/var/lib/apt/lists -type f | while read file; do + testaccessrights "$file" '644' +done + testdownload() { local APT="$2" if [ -n "$3" ]; then @@ -65,6 +80,7 @@ testsuccess aptget update # test with already stored deb testsuccess aptget install -d apt testsuccess test -s rootdir/var/cache/apt/archives/apt_2.0_all.deb +testaccessrights 'aptarchive/pool/apt_2.0_all.deb' '644' mv aptarchive/pool/apt_2.0_all.deb aptarchive/pool/apt_2.0_all.deb.gone testdownload apt_2.0_all.deb apt mv aptarchive/pool/apt_2.0_all.deb.gone aptarchive/pool/apt_2.0_all.deb diff --git a/test/integration/test-apt-update-unauth b/test/integration/test-apt-update-unauth index cf5195024..b7ccd6cf3 100755 --- a/test/integration/test-apt-update-unauth +++ b/test/integration/test-apt-update-unauth @@ -27,7 +27,7 @@ runtest() { find rootdir/var/lib/apt/lists/ -type f | xargs rm -f rm -f aptarchive/dists/unstable/*Release* - aptget update -qq --allow-insecure-repositories + testsuccess aptget update -qq --allow-insecure-repositories # FIXME: this really shouldn't be needed rm -f rootdir/var/lib/apt/lists/partial/* @@ -41,7 +41,6 @@ runtest() { aptarchive/dists/unstable/main/binary-i386/Packages.uncompressed # and ensure we re-check the downloaded data - msgtest "Check rollback on going from unauth -> auth" # change the local packages file PKGS=$(ls rootdir/var/lib/apt/lists/*Packages*) @@ -49,18 +48,22 @@ runtest() { ls rootdir/var/lib/apt/lists/ > lists.before # update and ensure all is reverted on the hashsum failure - aptget update -o Debug::Acquire::Transaction=0 -o Debug::pkgAcquire::Auth=1 -o Debug::pkgAcquire::worker=0 -o Debug::acquire::http=0 > output.log 2>&1 || true + testfailure aptget update -o Debug::Acquire::Transaction=0 -o Debug::pkgAcquire::Auth=1 -o Debug::pkgAcquire::worker=0 -o Debug::acquire::http=0 # ensure we have before what we have after + msgtest 'Check rollback on going from' 'unauth -> auth' ls rootdir/var/lib/apt/lists/ > lists.after - if diff -u lists.before lists.after; then + if cmp lists.before lists.after; then msgpass else - cat output.log - msgfail + echo >&2 '### Output of previous apt-get update ###' + cat >&2 rootdir/tmp/testfailure.output + echo >&2 '### Changes in the lists-directory: ###' + diff -u >&2 lists.before lists.after + msgfail fi - # move uncompressed back for release file + # move uncompressed back for release file mv aptarchive/dists/unstable/main/binary-i386/Packages.uncompressed \ aptarchive/dists/unstable/main/binary-i386/Packages } @@ -72,6 +75,5 @@ for COMPRESSEDINDEXES in 'false' 'true'; do else msgmsg 'Run tests with GzipIndexes disabled' fi - - runtest + runtest done -- 2.45.2