summaryrefslogtreecommitdiff
path: root/apt-pkg
diff options
context:
space:
mode:
authorDavid Kalnischkies <david@kalnischkies.de>2018-08-18 17:32:04 +0200
committerJulian Andres Klode <julian.klode@canonical.com>2018-08-20 09:42:31 +0200
commitaebd4278bacc728ab00ebe31556983e140f60e47 (patch)
tree5868282247206ed8562c1fafe240cf9b5d94b932 /apt-pkg
parentb4e19f92719b20ff3630d776badb8b244d1d20d2 (diff)
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
Diffstat (limited to 'apt-pkg')
-rw-r--r--apt-pkg/acquire-item.cc43
1 files changed, 23 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();