clear alternative URIs for mirror:// between steps (CVE-2018-0501)

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

3 files changed, 66 insertions, 20 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 21d990fba..3e858175c 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -276,6 +276,26 @@ static HashStringList GetExpectedHashesFromFor(metaIndex * const Parser, std::st
 }
 									/*}}}*/
 
+class pkgAcquire::Item::Private /*{{{*/
+{
+public:
+   struct AlternateURI
+   {
+      std::string URI;
+      std::unordered_map<std::string, std::string> changefields;
+      AlternateURI(std::string &&u, decltype(changefields) &&cf) : URI(u), changefields(cf) {}
+   };
+   std::list<AlternateURI> AlternativeURIs;
+   std::vector<std::string> PastRedirections;
+   std::unordered_map<std::string, std::string> 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
@@ -682,24 +702,6 @@ class APT_HIDDEN CleanupItem : public pkgAcqTransactionItem		/*{{{*/
 									/*}}}*/
 
 // Acquire::Item::Item - Constructor					/*{{{*/
-class pkgAcquire::Item::Private
-{
-public:
-   struct AlternateURI
-   {
-      std::string const URI;
-      std::unordered_map<std::string, std::string> changefields;
-      AlternateURI(std::string &&u, decltype(changefields) &&cf) : URI(u), changefields(cf) {}
-   };
-   std::list<AlternateURI> AlternativeURIs;
-   std::vector<std::string> PastRedirections;
-   std::unordered_map<std::string, std::string> 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),
@@ -960,7 +962,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				/*{{{*/
@@ -985,6 +987,7 @@ bool pkgAcquire::Item::Rename(string const &From,string const &To)
 									/*}}}*/
 void pkgAcquire::Item::Dequeue()					/*{{{*/
 {
+   d->AlternativeURIs.clear();
    Owner->Dequeue(this);
 }
 									/*}}}*/
@@ -1187,7 +1190,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