From 15124923b4cdb19af6bf642f396dab7c3e6bd074 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 29 Apr 2022 16:58:23 +0200 Subject: Allow HashSum mismatches to fallback to other mirrors Perhaps a common error in the past nowadays thanks to by-hash we rarely get mismatches due to our calling behaviour and more because the mirror is genuinely bad especially if this mirror didn't work with by-hash. Using our "normal" error handling allows falling back to other mirrors which hopefully do support by-hash and/or are not broken on a filesystem level making us more resistent to the occasional bad apple in a wild mirror forest. --- apt-pkg/acquire-item.cc | 4 +- apt-pkg/acquire-worker.cc | 108 ++++++++++++--------- .../test-apt-update-incomplete-file-mirror | 30 ++++++ 3 files changed, 94 insertions(+), 48 deletions(-) diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index c50f46909..2c3f45076 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -950,7 +950,7 @@ void pkgAcquire::Item::FailMessage(string const &Message) failreason = WEAK_HASHSUMS; else if (FailReason == "RedirectionLoop") failreason = REDIRECTION_LOOP; - else if (Status == StatAuthError) + else if (Status == StatAuthError || FailReason == "HashSumMismatch") failreason = HASHSUM_MISMATCH; if(ErrorText.empty()) @@ -975,7 +975,7 @@ void pkgAcquire::Item::FailMessage(string const &Message) break; } - if (Status == StatAuthError) + if (Status == StatAuthError || failreason == HASHSUM_MISMATCH) { auto const ExpectedHashes = GetExpectedHashes(); if (ExpectedHashes.empty() == false) diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc index 4b9d33505..55897945c 100644 --- a/apt-pkg/acquire-worker.cc +++ b/apt-pkg/acquire-worker.cc @@ -486,9 +486,21 @@ bool pkgAcquire::Worker::RunMessages() bool const isIMSHit = StringToBool(LookupTag(Message,"IMS-Hit"),false) || StringToBool(LookupTag(Message,"Alt-IMS-Hit"),false); auto const forcedHash = _config->Find("Acquire::ForceHash"); + + bool consideredOkay = true; + HashStringList ExpectedHashes; + bool const DebugAuth = _config->FindB("Debug::pkgAcquire::Auth", false); + if (DebugAuth) + { + std::clog << "201 URI Done: " << URI << endl + << "ReceivedHash:" << endl; + for (auto const &hs : ReceivedHashes) + std::clog << "\t- " << hs.toStr() << std::endl; + std::clog << "ExpectedHash:" << endl; + } for (auto const Owner: ItmOwners) { - HashStringList const ExpectedHashes = [&]() { + HashStringList const OwnerExpectedHashes = [&]() { if (AltFile) { auto const * const transOwner = dynamic_cast(Owner); @@ -501,46 +513,51 @@ bool pkgAcquire::Worker::RunMessages() } return Owner->GetExpectedHashes(); }(); - if(_config->FindB("Debug::pkgAcquire::Auth", false) == true) + for (auto const &h : OwnerExpectedHashes) { - std::clog << "201 URI Done: " << Owner->DescURI() << endl - << "ReceivedHash:" << endl; - for (HashStringList::const_iterator hs = ReceivedHashes.begin(); hs != ReceivedHashes.end(); ++hs) - std::clog << "\t- " << hs->toStr() << std::endl; - std::clog << "ExpectedHash:" << endl; - for (HashStringList::const_iterator hs = ExpectedHashes.begin(); hs != ExpectedHashes.end(); ++hs) - std::clog << "\t- " << hs->toStr() << std::endl; - std::clog << endl; - } - - // decide if what we got is what we expected - bool consideredOkay = false; - if ((forcedHash.empty() && ExpectedHashes.empty() == false) || - (forcedHash.empty() == false && ExpectedHashes.usable())) - { - if (ReceivedHashes.empty()) + if (not ExpectedHashes.push_back(h)) { - /* IMS-Hits can't be checked here as we will have uncompressed file, - but the hashes for the compressed file. What we have was good through - so all we have to ensure later is that we are not stalled. */ - consideredOkay = isIMSHit; - } - else if (ReceivedHashes == ExpectedHashes) - consideredOkay = true; - else consideredOkay = false; - + std::clog << "\t- " << h.toStr() << " [conflict]" << std::endl; + } + else if (DebugAuth) + std::clog << "\t- " << h.toStr() << std::endl; + } + } + if (DebugAuth) + std::clog << endl; + + // decide if what we got is what we expected + if (not consideredOkay) + ; + else if ((forcedHash.empty() && not ExpectedHashes.empty()) || + (not forcedHash.empty() && ExpectedHashes.usable())) + { + if (ReceivedHashes.empty()) + { + /* IMS-Hits can't be checked here as we will have uncompressed file, + but the hashes for the compressed file. What we have was good through + so all we have to ensure later is that we are not stalled. */ + consideredOkay = isIMSHit; } + else if (ReceivedHashes == ExpectedHashes) + consideredOkay = true; else - consideredOkay = !Owner->HashesRequired(); - - if (consideredOkay == true) - consideredOkay = Owner->VerifyDone(Message, Config); - else // hashsum mismatch - Owner->Status = pkgAcquire::Item::StatAuthError; + consideredOkay = false; + } + else + consideredOkay = std::none_of(ItmOwners.begin(), ItmOwners.end(), [](auto const * const O) { return O->HashesRequired(); }); + bool otherReasons = false; + if (consideredOkay && not std::all_of(ItmOwners.begin(), ItmOwners.end(), [&](auto * const O) { return O->VerifyDone(Message, Config); })) + { + consideredOkay = false; + otherReasons = true; + } - if (consideredOkay == true) + if (consideredOkay) + { + for (auto const Owner : ItmOwners) { if (isDoomedItem(Owner) == false) Owner->Done(Message, ReceivedHashes, Config); @@ -552,22 +569,21 @@ bool pkgAcquire::Worker::RunMessages() Log->Done(Owner->GetItemDesc()); } } + } + else + { + if (otherReasons) + HandleFailure(ItmOwners, Config, Log, Message, false, false); else { - auto SavedDesc = Owner->GetItemDesc(); - if (isDoomedItem(Owner) == false) + if (Message.find("\nFailReason:") == std::string::npos) { - if (Message.find("\nFailReason:") == std::string::npos) - { - if (ReceivedHashes != ExpectedHashes) - Message.append("\nFailReason: HashSumMismatch"); - else - Message.append("\nFailReason: WeakHashSums"); - } - Owner->Failed(Message,Config); + if (ReceivedHashes != ExpectedHashes) + Message.append("\nFailReason: HashSumMismatch"); + else + Message.append("\nFailReason: WeakHashSums"); } - if (Log != nullptr) - Log->Fail(SavedDesc); + HandleFailure(ItmOwners, Config, Log, Message, false, true); } } ItemDone(); diff --git a/test/integration/test-apt-update-incomplete-file-mirror b/test/integration/test-apt-update-incomplete-file-mirror index 2d2125582..959ddc1a6 100755 --- a/test/integration/test-apt-update-incomplete-file-mirror +++ b/test/integration/test-apt-update-incomplete-file-mirror @@ -25,9 +25,39 @@ testsuccess apt update rm -rf rootdir/var/lib/apt/lists + +msgmsg 'File mirror was hacked' +mkdir aptarchive2 +cp -a aptarchive/dists aptarchive2/ +rm -rf rootdir/var/lib/apt/lists find aptarchive/dists -name 'Packages' | while read FILE; do echo 'hacked' > $FILE done testfailure apt update -o Debug::pkgAcquire::Worker=1 testsuccessequal '4' grep -c -- '- Filesize:' rootdir/tmp/testfailure.output testsuccessequal '2' grep -c '%0aAlt-Checksum-FileSize-Hash:%20' rootdir/tmp/testfailure.output + + +msgmsg 'Fallback over hashsum errors' +rm -f rootdir/etc/apt/sources.list rootdir/etc/apt/sources.list.d/* +echo "deb mirror+file:${TMPWORKINGDIRECTORY}/mirror.list unstable main" > rootdir/etc/apt/sources.list +rm -rf rootdir/var/lib/apt/lists +cat > mirror.list < mirror.list < mirror.list <