diff options
| -rw-r--r-- | apt-pkg/acquire-item.cc | 79 | ||||
| -rw-r--r-- | apt-pkg/acquire-item.h | 148 | ||||
| -rwxr-xr-x | test/integration/test-apt-update-nofallback | 207 |
3 files changed, 348 insertions, 86 deletions
diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 4e843ecaf..7fad5605d 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -193,6 +193,14 @@ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const Status = StatError; // do not report as usually its not the mirrors fault, but Portal/Proxy break; + case SignatureError: + ErrorText = _("Signature error"); + Status = StatError; + break; + case NotClearsigned: + ErrorText = _("Does not start with a cleartext signature"); + Status = StatError; + break; } return false; } @@ -1307,6 +1315,8 @@ void pkgAcqMetaBase::AbortTransaction() if(_config->FindB("Debug::Acquire::Transaction", false) == true) std::clog << "AbortTransaction: " << TransactionManager << std::endl; + // ensure the toplevel is in error state too + Status = pkgAcquire::Item::StatError; for (std::vector<Item*>::iterator I = Transaction.begin(); I != Transaction.end(); ++I) { @@ -1407,6 +1417,7 @@ pkgAcqMetaSig::pkgAcqMetaSig(pkgAcquire *Owner, /*{{{*/ indexRecords* MetaIndexParser) : pkgAcqMetaBase(Owner, HashStringList(), TransactionManager), RealURI(URI), MetaIndexParser(MetaIndexParser), MetaIndexFile(MetaIndexFile), + URIDesc(URIDesc), ShortDesc(ShortDesc), IndexTargets(IndexTargets), AuthPass(false), IMSHit(false) { DestFile = _config->FindDir("Dir::State::lists") + "partial/"; @@ -1505,13 +1516,35 @@ void pkgAcqMetaSig::Done(string Message,unsigned long long Size, HashStringList DestFile += URItoFileName(RealURI); } - Complete = true; + // we parse the MetaIndexFile here because at this point we can + // trust the data + if(AuthPass == true) + { + // load indexes and queue further downloads + MetaIndexParser->Load(MetaIndexFile); + ((pkgAcqMetaIndex*)TransactionManager)->QueueIndexes(true); + } + Complete = true; } /*}}}*/ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/ { string Final = _config->FindDir("Dir::State::lists") + URItoFileName(RealURI); + + // FIXME: meh, this is not really elegant + string InReleaseURI = RealURI.replace(RealURI.rfind("Release.gpg"), 12, + "InRelease"); + string FinalInRelease = _config->FindDir("Dir::State::lists") + URItoFileName(InReleaseURI); + + if(RealFileExists(Final) || RealFileExists(FinalInRelease)) + { + _error->Error("The repository '%s' is no longer signed.", + URIDesc.c_str()); + Rename(MetaIndexFile, MetaIndexFile+".FAILED"); + TransactionManager->AbortTransaction(); + return; + } // this ensures that any file in the lists/ dir is removed by the // transaction @@ -1527,6 +1560,19 @@ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/ return; } + // only allow going further if the users explicitely wants it + if(_config->FindB("APT::Get::AllowUnauthenticated", false)) + { + _error->Warning("Please use --allow-unauthenticated"); + } + else + { + // we parse the indexes here because at this point the user wanted + // a repository that may potentially harm him + MetaIndexParser->Load(MetaIndexFile); + ((pkgAcqMetaIndex*)TransactionManager)->QueueIndexes(true); + } + // FIXME: this is used often (e.g. in pkgAcqIndexTrans) so refactor if (Cnf->LocalOnly == true || StringToBool(LookupTag(Message,"Transient-Failure"),false) == false) @@ -1547,6 +1593,7 @@ pkgAcqMetaIndex::pkgAcqMetaIndex(pkgAcquire *Owner, /*{{{*/ const vector<IndexTarget*>* IndexTargets, indexRecords* MetaIndexParser) : pkgAcqMetaBase(Owner, HashStringList(), TransactionManager), RealURI(URI), IndexTargets(IndexTargets), + URIDesc(URIDesc), ShortDesc(ShortDesc), MetaIndexParser(MetaIndexParser), AuthPass(false), IMSHit(false), MetaIndexSigURI(MetaIndexSigURI), MetaIndexSigURIDesc(MetaIndexSigURIDesc), MetaIndexSigShortDesc(MetaIndexSigShortDesc) @@ -1619,13 +1666,7 @@ void pkgAcqMetaIndex::Done(string Message,unsigned long long Size,HashStringList // Still more retrieving to do return; - if (SigFile == "") - { - // load indexes, the signature will downloaded afterwards - MetaIndexParser->Load(DestFile); - QueueIndexes(true); - } - else + if (SigFile != "") { // There was a signature file, so pass it to gpgv for // verification @@ -1952,7 +1993,8 @@ void pkgAcqMetaIndex::Failed(string Message, /* Always move the meta index, even if gpgv failed. This ensures * that PackageFile objects are correctly filled in */ - if (FileExists(DestFile)) { + if (FileExists(DestFile)) + { string FinalFile = _config->FindDir("Dir::State::lists"); FinalFile += URItoFileName(RealURI); /* InRelease files become Release files, otherwise @@ -1970,13 +2012,19 @@ void pkgAcqMetaIndex::Failed(string Message, DestFile = FinalFile; } - // warn if the repository is unsinged - _error->Warning(_("The data from '%s' is not signed. Packages " - "from that repository can not be authenticated."), - URIDesc.c_str()); // No Release file was present, or verification failed, so fall // back to queueing Packages files without verification - QueueIndexes(false); + // only allow going further if the users explicitely wants it + if(_config->FindB("APT::Get::AllowUnauthenticated", false)) + { + // warn if the repository is unsinged + _error->Warning(_("The data from '%s' is not signed. Packages " + "from that repository can not be authenticated."), + URIDesc.c_str()); + _error->Warning("Please use --allow-unauthenticated"); + } else { + QueueIndexes(false); + } } /*}}}*/ @@ -2060,7 +2108,8 @@ void pkgAcqMetaClearSig::Done(std::string Message,unsigned long long Size, if (FileExists(DestFile) && !StartsWithGPGClearTextSignature(DestFile)) { pkgAcquire::Item::Failed(Message, Cnf); - ErrorText = _("Does not start with a cleartext signature"); + RenameOnError(NotClearsigned); + TransactionManager->AbortTransaction(); return; } pkgAcqMetaIndex::Done(Message, Size, Hashes, Cnf); diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 15b566069..cc156cf17 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -312,7 +312,9 @@ class pkgAcquire::Item : public WeakPointable enum RenameOnErrorState { HashSumMismatch, SizeMismatch, - InvalidFormat + InvalidFormat, + SignatureError, + NotClearsigned, }; /** \brief Rename failed file and set error @@ -367,6 +369,67 @@ class pkgAcqMetaBase : public pkgAcquire::Item : Item(Owner, ExpectedHashes, TransactionManager) {}; }; +/** \brief An acquire item that downloads the detached signature {{{ + * of a meta-index (Release) file, then queues up the release + * file itself. + * + * \todo Why protected members? + * + * \sa pkgAcqMetaIndex + */ +class pkgAcqMetaSig : public pkgAcqMetaBase +{ + void *d; + + protected: + + /** \brief The URI of the signature file. Unlike Desc.URI, this is + * never modified; it is used to determine the file that is being + * downloaded. + */ + std::string RealURI; + + std::string URIDesc; + std::string ShortDesc; + + /** \brief A package-system-specific parser for the meta-index file. */ + indexRecords* MetaIndexParser; + + /** \brief The file we need to verify */ + std::string MetaIndexFile; + + /** \brief The index files which should be looked up in the meta-index + * and then downloaded. + * + * \todo Why a list of pointers instead of a list of structs? + */ + const std::vector<IndexTarget*>* IndexTargets; + + /** \brief If we are in fetching or download state */ + bool AuthPass; + + /** \brief Was this file already on disk */ + bool IMSHit; + + public: + + // Specialized action members + virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf); + virtual void Done(std::string Message,unsigned long long Size, HashStringList const &Hashes, + pkgAcquire::MethodConfig *Cnf); + virtual std::string Custom600Headers() const; + virtual std::string DescURI() const {return RealURI; }; + + /** \brief Create a new pkgAcqMetaSig. */ + pkgAcqMetaSig(pkgAcquire *Owner, + pkgAcqMetaBase *TransactionManager, + std::string URI,std::string URIDesc, std::string ShortDesc, + std::string MetaIndexFile, + const std::vector<IndexTarget*>* IndexTargets, + indexRecords* MetaIndexParser); + virtual ~pkgAcqMetaSig(); +}; + /*}}}*/ /** \brief An item that is responsible for downloading the meta-index {{{ * file (i.e., Release) itself and verifying its signature. @@ -436,15 +499,8 @@ class pkgAcqMetaIndex : public pkgAcqMetaBase */ void AuthDone(std::string Message); - /** \brief Starts downloading the individual index files. - * - * \param verify If \b true, only indices whose expected hashsum - * can be determined from the meta-index will be downloaded, and - * the hashsums of indices will be checked (reporting - * #StatAuthError if there is a mismatch). If verify is \b false, - * no hashsum checking will be performed. - */ - void QueueIndexes(bool verify); + std::string URIDesc; + std::string ShortDesc; /** \brief The URI of the meta-index file for the detached signature */ std::string MetaIndexSigURI; @@ -459,7 +515,17 @@ class pkgAcqMetaIndex : public pkgAcqMetaBase void Init(std::string URIDesc, std::string ShortDesc); public: - + + /** \brief Starts downloading the individual index files. + * + * \param verify If \b true, only indices whose expected hashsum + * can be determined from the meta-index will be downloaded, and + * the hashsums of indices will be checked (reporting + * #StatAuthError if there is a mismatch). If verify is \b false, + * no hashsum checking will be performed. + */ + void QueueIndexes(bool verify); + // Specialized action members virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf); virtual void Done(std::string Message,unsigned long long Size, HashStringList const &Hashes, @@ -999,66 +1065,6 @@ class OptionalIndexTarget : public IndexTarget }; /*}}}*/ -/** \brief An acquire item that downloads the detached signature {{{ - * of a meta-index (Release) file, then queues up the release - * file itself. - * - * \todo Why protected members? - * - * \sa pkgAcqMetaIndex - */ -class pkgAcqMetaSig : public pkgAcqMetaBase -{ - void *d; - - protected: - /** \brief The last good signature file */ - std::string LastGoodSig; - - /** \brief The URI of the signature file. Unlike Desc.URI, this is - * never modified; it is used to determine the file that is being - * downloaded. - */ - std::string RealURI; - - /** \brief A package-system-specific parser for the meta-index file. */ - indexRecords* MetaIndexParser; - - /** \brief The file we need to verify */ - std::string MetaIndexFile; - - /** \brief The index files which should be looked up in the meta-index - * and then downloaded. - * - * \todo Why a list of pointers instead of a list of structs? - */ - const std::vector<IndexTarget*>* IndexTargets; - - /** \brief If we are in fetching or download state */ - bool AuthPass; - - /** \brief Was this file already on disk */ - bool IMSHit; - - public: - - // Specialized action members - virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf); - virtual void Done(std::string Message,unsigned long long Size, HashStringList const &Hashes, - pkgAcquire::MethodConfig *Cnf); - virtual std::string Custom600Headers() const; - virtual std::string DescURI() const {return RealURI; }; - - /** \brief Create a new pkgAcqMetaSig. */ - pkgAcqMetaSig(pkgAcquire *Owner, - pkgAcqMetaBase *TransactionManager, - std::string URI,std::string URIDesc, std::string ShortDesc, - std::string MetaIndexFile, - const std::vector<IndexTarget*>* IndexTargets, - indexRecords* MetaIndexParser); - virtual ~pkgAcqMetaSig(); -}; - /*}}}*/ /** \brief An item that is responsible for fetching a package file. {{{ * * If the package file already exists in the cache, nothing will be diff --git a/test/integration/test-apt-update-nofallback b/test/integration/test-apt-update-nofallback new file mode 100755 index 000000000..4e8ea9916 --- /dev/null +++ b/test/integration/test-apt-update-nofallback @@ -0,0 +1,207 @@ +#!/bin/sh +# +# ensure we never fallback from a signed to a unsigned repo +# +# hash checks are done in +# +set -e + +simulate_mitm_and_inject_evil_package() +{ + rm -f $APTARCHIVE/dists/unstable/InRelease + rm -f $APTARCHIVE/dists/unstable/Release.gpg + inject_evil_package +} + +inject_evil_package() +{ + cat > $APTARCHIVE/dists/unstable/main/binary-i386/Packages <<EOF +Package: evil +Installed-Size: 29 +Maintainer: Joe Sixpack <joe@example.org> +Architecture: all +Version: 1.0 +Filename: pool/evil_1.0_all.deb +Size: 1270 +Description: an autogenerated evil package +EOF + # avoid ims hit + touch -d '+1hour' aptarchive/dists/unstable/main/binary-i386/Packages +} + +assert_update_is_refused_and_last_good_state_used() +{ + testequal "E: The repository 'file: unstable Release.gpg' is no longer signed." aptget update -qq + + assert_repo_is_intact +} + +assert_repo_is_intact() +{ + testequal "foo/unstable 2.0 all" apt list -q + testsuccess "" aptget install -y -s foo + testfailure "" aptget install -y evil + + LISTDIR=rootdir/var/lib/apt/lists + if ! ( ls $LISTDIR/*InRelease >/dev/null 2>&1 || + ls $LISTDIR/*Release.gpg >/dev/null 2>&1 ); then + echo "Can not find InRelease/Release.gpg in $(ls $LISTDIR)" + msgfail + fi +} + +setupaptarchive_with_lists_clean() +{ + setupaptarchive --no-update + rm -f rootdir/var/lib/apt/lists/_* + #rm -rf rootdir/var/lib/apt/lists +} + +test_from_inrelease_to_unsigned() +{ + # setup archive with InRelease file + setupaptarchive_with_lists_clean + testsuccess aptget update + + simulate_mitm_and_inject_evil_package + assert_update_is_refused_and_last_good_state_used +} + +test_from_release_gpg_to_unsigned() +{ + # setup archive with Release/Release.gpg (but no InRelease) + setupaptarchive_with_lists_clean + rm $APTARCHIVE/dists/unstable/InRelease + testsuccess aptget update + + simulate_mitm_and_inject_evil_package + assert_update_is_refused_and_last_good_state_used +} + +test_cve_2012_0214() +{ + # see https://bugs.launchpad.net/ubuntu/+source/apt/+bug/947108 + # + # it was possible to MITM the download so that InRelease/Release.gpg + # are not delivered (404) and a altered Release file was send + # + # apt left the old InRelease file in /var/lib/apt/lists and downloaded + # the unauthenticated Release file too giving the false impression that + # Release was authenticated + # + # Note that this is pretty much impossible nowdays because: + # a) InRelease is left as is, not split to InRelease/Release as it was + # in the old days + # b) we refuse to go from signed->unsigned + # + # Still worth having a regression test the simulates the condition + + # setup archive with InRelease + setupaptarchive_with_lists_clean + testsuccess aptget update + + # do what CVE-2012-0214 did + rm $APTARCHIVE/dists/unstable/InRelease + rm $APTARCHIVE/dists/unstable/Release.gpg + inject_evil_package + # build valid Release file + aptftparchive -qq release ./aptarchive > aptarchive/dists/unstable/Release + + assert_update_is_refused_and_last_good_state_used + + # ensure there is no _Release file downloaded + testfailure ls rootdir/var/lib/apt/lists/*_Release +} + +test_subvert_inrelease() +{ + # setup archive with InRelease + setupaptarchive_with_lists_clean + testsuccess aptget update + + # replace InRelease with something else + mv $APTARCHIVE/dists/unstable/Release $APTARCHIVE/dists/unstable/InRelease + + testequal "W: Failed to fetch file:${APTARCHIVE}/dists/unstable/InRelease Does not start with a cleartext signature + +E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -qq + + # ensure we keep the repo + assert_repo_is_intact +} + +test_inrelease_to_invalid_inrelease() +{ + # setup archive with InRelease + setupaptarchive_with_lists_clean + testsuccess aptget update + + # now remove InRelease and subvert Release do no longer verify + sed -i 's/Codename.*/Codename: evil!'/ $APTARCHIVE/dists/unstable/InRelease + inject_evil_package + + testequal "W: An error occurred during the signature verification. The repository is not updated and the previous index files will be used. GPG error: file: unstable InRelease: The following signatures were invalid: BADSIG 5A90D141DBAC8DAE Joe Sixpack (APT Testcases Dummy) <joe@example.org> + +W: Failed to fetch file:${APTARCHIVE}/dists/unstable/InRelease + +W: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -qq + + # ensure we keep the repo + assert_repo_is_intact + testfailure grep "evil" rootdir/var/lib/apt/lists/*InRelease +} + +test_release_gpg_to_invalid_release_release_gpg() +{ + # setup archive with InRelease + setupaptarchive_with_lists_clean + rm $APTARCHIVE/dists/unstable/InRelease + testsuccess aptget update + + # now subvert Release do no longer verify + echo "Some evil data" >> $APTARCHIVE/dists/unstable/Release + inject_evil_package + + testequal "E: The repository 'file: unstable Release.gpg' is no longer signed." aptget update -qq + + assert_repo_is_intact + testfailure grep "evil" rootdir/var/lib/apt/lists/*Release +} + + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework + +setupenvironment +configarchitecture "i386" + +# a "normal" package with source and binary +buildsimplenativepackage 'foo' 'all' '2.0' + +# setup the archive and ensure we have a single package that installs fine +setupaptarchive +APTARCHIVE=$(readlink -f ./aptarchive) +assert_repo_is_intact + +# test the various cases where a repo may go from signed->unsigned +msgmsg "test_from_inrelease_to_unsigned" +test_from_inrelease_to_unsigned + +msgmsg "test_from_release_gpg_to_unsigned" +test_from_release_gpg_to_unsigned + +# ensure we do not regress on CVE-2012-0214 +msgmsg "test_cve_2012_0214" +test_cve_2012_0214 + +# ensure InRelase can not be subverted +msgmsg "test_subvert_inrelease" +test_subvert_inrelease + +# ensure we revert to last good state if InRelease does not verify +msgmsg "test_inrelease_to_invalid_inrelease" +test_inrelease_to_invalid_inrelease + +# ensure we revert to last good state if Release/Release.gpg does not verify +msgmsg "test_release_gpg_to_invalid_release_release_gpg" +test_release_gpg_to_invalid_release_release_gpg |