From 29658a3a74af49e2a24e17bdebb20e1612aac3ec Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 18 Aug 2018 17:32:04 +0200 Subject: clear alternative URIs for mirror:// between steps (CVE-2018-0501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit APT in 1.6 saw me rewriting the mirror:// transport method, which works comparable to the decommissioned httpredir.d.o "just" that apt requests a mirror list and performs all the redirections internally with all the bells like parallel download and automatic fallback (more details in the apt-transport-mirror manpage included in the 1.6 release). The automatic fallback is the problem here: The intend is that if a file fails to be downloaded (e.g. because the mirror is offline, broken, out-of-sync, …) instead of erroring out the next mirror in the list is contacted for a retry of the download. Internally the acquire process of an InRelease file (works with the Release/Release.gpg pair, too) happens in steps: 1) download file and 2) verify file, both handled as URL requests passed around. Due to an oversight the fallbacks for the first step are still active for the second step, so that the successful download from another mirror stands in for the failed verification… *facepalm* Note that the attacker can not judge by the request arriving for the InRelease file if the user is using the mirror method or not. If entire traffic is observed Eve might be able to observe the request for a mirror list, but that might or might not be telling if following requests for InRelease files will be based on that list or for another sources.list entry not using mirror (Users have also the option to have the mirror list locally (via e.g. mirror+file://) instead of on a remote host). If the user isn't using mirror:// for this InRelease file apt will fail very visibly as intended. (The mirror list needs to include at least two mirrors and to work reliably the attacker needs to be able to MITM all mirrors in the list. For remotely accessed mirror lists this is no limitation as the attacker is in full control of the file in that case) Fixed by clearing the alternatives after a step completes (and moving a pimpl class further to the top to make that valid compilable code). mirror:// is at the moment the only method using this code infrastructure (for all others this set is already empty) and the only method-independent user so far is the download of deb files, but those are downloaded and verified in a single step; so there shouldn't be much opportunity for regression here even through a central code area is changed. Upgrade instructions: Given all apt-based frontends are affected, even additional restrictions like signed-by are bypassed and the attack in progress is hardly visible in the progress reporting of an update operation (the InRelease file is marked "Ign", but no fallback to "Release/Release.gpg" is happening) and leaves no trace (expect files downloaded from the attackers repository of course) the best course of action might be to change the sources.list to not use the mirror family of transports ({tor+,…}mirror{,+{http{,s},file,…}}) until a fixed version of the src:apt packages are installed. Regression-Of: 355e1aceac1dd05c4c7daf3420b09bd860fd169d, 57fa854e4cdb060e87ca265abd5a83364f9fa681 LP: #1787752 --- apt-pkg/acquire-item.cc | 45 ++++++++++++---------- .../test-cve-2018-0501-mirror-alternatives | 31 +++++++++++++++ test/integration/test-method-mirror | 12 ++++++ 3 files changed, 67 insertions(+), 21 deletions(-) create mode 100755 test/integration/test-cve-2018-0501-mirror-alternatives diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index b40c67ec1..83c793093 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -277,6 +277,27 @@ static HashStringList GetExpectedHashesFromFor(metaIndex * const Parser, std::st } /*}}}*/ +class pkgAcquire::Item::Private /*{{{*/ +{ +public: + struct AlternateURI + { + std::string URI; + std::unordered_map changefields; + AlternateURI(std::string &&u, decltype(changefields) &&cf) : URI(u), changefields(cf) {} + }; + std::list AlternativeURIs; + std::vector BadAlternativeSites; + std::vector PastRedirections; + std::unordered_map CustomFields; + unsigned int Retries; + + Private() : Retries(_config->FindI("Acquire::Retries", 0)) + { + } +}; + /*}}}*/ + // all ::HashesRequired and ::GetExpectedHashes implementations /*{{{*/ /* ::GetExpectedHashes is abstract and has to be implemented by all subclasses. It is best to implement it as broadly as possible, while ::HashesRequired defaults @@ -748,25 +769,6 @@ class APT_HIDDEN CleanupItem : public pkgAcqTransactionItem /*{{{*/ /*}}}*/ // Acquire::Item::Item - Constructor /*{{{*/ -class pkgAcquire::Item::Private -{ -public: - struct AlternateURI - { - std::string URI; - std::unordered_map changefields; - AlternateURI(std::string &&u, decltype(changefields) &&cf) : URI(u), changefields(cf) {} - }; - std::list AlternativeURIs; - std::vector BadAlternativeSites; - std::vector PastRedirections; - std::unordered_map CustomFields; - unsigned int Retries; - - Private() : Retries(_config->FindI("Acquire::Retries", 0)) - { - } -}; APT_IGNORE_DEPRECATED_PUSH pkgAcquire::Item::Item(pkgAcquire * const owner) : FileSize(0), PartialSize(0), Mode(0), ID(0), Complete(false), Local(false), @@ -1045,7 +1047,7 @@ void pkgAcquire::Item::Done(string const &/*Message*/, HashStringList const &Has } Status = StatDone; ErrorText.clear(); - Owner->Dequeue(this); + Dequeue(); } /*}}}*/ // Acquire::Item::Rename - Rename a file /*{{{*/ @@ -1070,6 +1072,7 @@ bool pkgAcquire::Item::Rename(string const &From,string const &To) /*}}}*/ void pkgAcquire::Item::Dequeue() /*{{{*/ { + d->AlternativeURIs.clear(); Owner->Dequeue(this); } /*}}}*/ @@ -1272,7 +1275,7 @@ void pkgAcqMetaBase::AbortTransaction() { (*I)->ExpectedAdditionalItems = 0; if ((*I)->Status != pkgAcquire::Item::StatFetching) - Owner->Dequeue(*I); + (*I)->Dequeue(); (*I)->TransactionState(TransactionAbort); } Transaction.clear(); diff --git a/test/integration/test-cve-2018-0501-mirror-alternatives b/test/integration/test-cve-2018-0501-mirror-alternatives new file mode 100755 index 000000000..f15454765 --- /dev/null +++ b/test/integration/test-cve-2018-0501-mirror-alternatives @@ -0,0 +1,31 @@ +#!/bin/sh +set -e +TESTDIR="$(readlink -f "$(dirname "$0")")" +. "$TESTDIR/framework" + +setupenvironment +configarchitecture "i386" + +buildsimplenativepackage 'foo' 'all' '1' 'stable' +setupaptarchive --no-update +changetohttpswebserver + +# User has mirror method configured in apt >= 1.6~alpha6 & +# Eve has enough MITM control over the network to +# a) have the mirror file include at least two mirrors and +# b) can send her bad InRelease files for both mirrors +sed -i -e 's# https:# mirror+https:#' -e 's#/ stable#/mirror.txt stable#' rootdir/etc/apt/sources.list.d/*-stable-* +echo "http://localhost:${APTHTTPPORT} +https://localhost:${APTHTTPSPORT}" > aptarchive/mirror.txt + +# real Eve would do something worse… +sed -i "/^Date: / a\ +Evil: yes" $(find ./aptarchive -name 'Release' -o -name 'InRelease') + +# progress display shows that the InRelease file was bad, +# but it is used anyhow as the bad file causes a fallback to +# a request to the second mirror which completes successful +# causing apt to believe the verify completed successfully… +testfailure apt update +testfailure grep '^Evil:' rootdir/var/lib/apt/lists/*Release +testfailure apt show foo diff --git a/test/integration/test-method-mirror b/test/integration/test-method-mirror index 38d6be9a9..56c9a10a0 100755 --- a/test/integration/test-method-mirror +++ b/test/integration/test-method-mirror @@ -196,6 +196,7 @@ msgmsg 'The prefix for the mirrorlist is' 'passed on' echo 'Dir::Bin::Methods::foo+mirror+file "mirror"; Dir::Bin::Methods::foo+mirror+http "mirror"; Dir::Bin::Methods::foo+http "http"; +Dir::Bin::Methods::foo+https "https"; ' > rootdir/etc/apt/apt.conf.d/99add-foo-method echo "http://localhost:${APTHTTPPORT}/redirectme " > aptarchive/mirror.txt @@ -241,3 +242,14 @@ Building dependency tree... Reading state information... All packages are up to date." apt update testrundownload 'foo=2' + +echo "https://localhost:${APTHTTPSPORT}/ +http://localhost:${APTHTTPPORT}/redirectme" > aptarchive/mirror.txt +rm -rf rootdir/var/lib/apt/lists +sed -i -e "s# foo+# [signed-by=$(readlink -f ./keys/joesixpack.pub)] foo+#g" rootdir/etc/apt/sources.list.d/apt-test-unstable-deb* +testsuccess apt update +testrundownload 'foo=2' + +rm -rf rootdir/var/lib/apt/lists +sed -i -e "s# \[signed-by=[^]]\+\] foo+# [signed-by=$(readlink -f ./keys/marvinparanoid.pub)] foo+#g" rootdir/etc/apt/sources.list.d/apt-test-unstable-deb* +testfailure apt update -- cgit v1.2.3