From ca7fd76c2f30c100dcf1c12e717ce397cccd690b Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Tue, 16 Sep 2014 20:23:43 +0200 Subject: SECURITY UPDATE for CVE-2014-{0488,0487,0489} incorrect invalidating of unauthenticated data (CVE-2014-0488) incorect verification of 304 reply (CVE-2014-0487) incorrect verification of Acquire::Gzip indexes (CVE-2014-0489) --- apt-pkg/acquire-item.cc | 97 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 75 insertions(+), 22 deletions(-) (limited to 'apt-pkg/acquire-item.cc') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 6cb9b012a..058b8bf74 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -1021,6 +1021,31 @@ void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf) /*{{{*/ Item::Failed(Message,Cnf); } /*}}}*/ +// pkgAcqIndex::GetFinalFilename - Return the full final file path /*{{{*/ +std::string pkgAcqIndex::GetFinalFilename(std::string const &URI, + std::string const &compExt) +{ + std::string FinalFile = _config->FindDir("Dir::State::lists"); + FinalFile += URItoFileName(URI); + if (_config->FindB("Acquire::GzipIndexes",false) && compExt == "gz") + FinalFile += ".gz"; + return FinalFile; +} + /*}}}*/ +// AcqIndex::ReverifyAfterIMS - Reverify index after an ims-hit /*{{{*/ +void pkgAcqIndex::ReverifyAfterIMS(std::string const &FileName) +{ + std::string const compExt = CompressionExtension.substr(0, CompressionExtension.find(' ')); + if (_config->FindB("Acquire::GzipIndexes",false) && compExt == "gz") + DestFile += ".gz"; + + string FinalFile = GetFinalFilename(RealURI, compExt); + Rename(FinalFile, FileName); + Decompression = true; + Desc.URI = "copy:" + FileName; + QueueURI(Desc); +} + /*}}}*/ // AcqIndex::Done - Finished a fetch /*{{{*/ // --------------------------------------------------------------------- /* This goes through a number of states.. On the initial fetch the @@ -1032,6 +1057,7 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, pkgAcquire::MethodConfig *Cfg) { Item::Done(Message,Size,Hash,Cfg); + std::string const compExt = CompressionExtension.substr(0, CompressionExtension.find(' ')); if (Decompression == true) { @@ -1043,6 +1069,7 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, if (!ExpectedHash.empty() && ExpectedHash.toStr() != Hash) { + Desc.URI = RealURI; RenameOnError(HashSumMismatch); return; } @@ -1053,9 +1080,9 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, /* Always verify the index file for correctness (all indexes must * have a Package field) (LP: #346386) (Closes: #627642) */ - FileFd fd(DestFile, FileFd::ReadOnly); + FileFd fd(DestFile, FileFd::ReadOnlyGzip); // Only test for correctness if the file is not empty (empty is ok) - if (fd.FileSize() > 0) + if (fd.Size() > 0) { pkgTagSection sec; pkgTagFile tag(&fd); @@ -1069,8 +1096,7 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, } // Done, move it into position - string FinalFile = _config->FindDir("Dir::State::lists"); - FinalFile += URItoFileName(RealURI); + string FinalFile = GetFinalFilename(RealURI, compExt); Rename(DestFile,FinalFile); chmod(FinalFile.c_str(),0644); @@ -1078,7 +1104,9 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, will work OK */ DestFile = _config->FindDir("Dir::State::lists") + "partial/"; DestFile += URItoFileName(RealURI); - + if (_config->FindB("Acquire::GzipIndexes",false) && compExt == "gz") + DestFile += ".gz"; + // Remove the compressed version. if (Erase == true) unlink(DestFile.c_str()); @@ -1094,7 +1122,10 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, { // The files timestamp matches if (StringToBool(LookupTag(Message,"Alt-IMS-Hit"),false) == true) - return; + { + ReverifyAfterIMS(FileName); + return; + } Decompression = true; Local = true; DestFile += ".decomp"; @@ -1111,15 +1142,12 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, ErrorText = "Method gave a blank filename"; } - std::string const compExt = CompressionExtension.substr(0, CompressionExtension.find(' ')); - // The files timestamp matches - if (StringToBool(LookupTag(Message,"IMS-Hit"),false) == true) { - if (_config->FindB("Acquire::GzipIndexes",false) && compExt == "gz") - // Update DestFile for .gz suffix so that the clean operation keeps it - DestFile += ".gz"; + if (StringToBool(LookupTag(Message,"IMS-Hit"),false) == true) + { + ReverifyAfterIMS(FileName); return; - } + } if (FileName == DestFile) Erase = true; @@ -1128,16 +1156,16 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, string decompProg; - // If we enable compressed indexes and already have gzip, keep it - if (_config->FindB("Acquire::GzipIndexes",false) && compExt == "gz" && !Local) { - string FinalFile = _config->FindDir("Dir::State::lists"); - FinalFile += URItoFileName(RealURI) + ".gz"; - Rename(DestFile,FinalFile); - chmod(FinalFile.c_str(),0644); - - // Update DestFile for .gz suffix so that the clean operation keeps it - DestFile = _config->FindDir("Dir::State::lists") + "partial/"; + // If we enable compressed indexes, queue for hash verification + if (_config->FindB("Acquire::GzipIndexes",false) && compExt == "gz" && !Local) + { + DestFile = _config->FindDir("Dir::State::lists"); DestFile += URItoFileName(RealURI) + ".gz"; + + Decompression = true; + Desc.URI = "copy:" + FileName; + QueueURI(Desc); + return; } @@ -1181,6 +1209,9 @@ string pkgAcqIndexTrans::Custom600Headers() string Final = _config->FindDir("Dir::State::lists"); Final += URItoFileName(RealURI); + if (_config->FindB("Acquire::GzipIndexes",false)) + Final += ".gz"; + struct stat Buf; if (stat(Final.c_str(),&Buf) != 0) return "\nFail-Ignore: true\nIndex-File: true"; @@ -1510,6 +1541,28 @@ void pkgAcqMetaIndex::AuthDone(string Message) /*{{{*/ std::cerr << "Signature verification succeeded: " << DestFile << std::endl; + // do not trust any previously unverified content that we may have + string LastGoodSigFile = _config->FindDir("Dir::State::lists").append("partial/").append(URItoFileName(RealURI)); + if (DestFile != SigFile) + LastGoodSigFile.append(".gpg"); + LastGoodSigFile.append(".reverify"); + if(IMSHit == false && RealFileExists(LastGoodSigFile) == false) + { + for (vector ::const_iterator Target = IndexTargets->begin(); + Target != IndexTargets->end(); + ++Target) + { + // remove old indexes + std::string index = _config->FindDir("Dir::State::lists") + + URItoFileName((*Target)->URI); + unlink(index.c_str()); + // and also old gzipindexes + index += ".gz"; + unlink(index.c_str()); + } + } + + // Download further indexes with verification QueueIndexes(true); -- cgit v1.2.3-70-g09d2 From daff4aa356128310f022370f7825bdc369c66ba8 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 17 Sep 2014 14:57:05 +0200 Subject: Fix regression for file:/// uris from CVE-2014-0487 Do not run ReverifyAfterIMS() for local file URIs as this will causes apt to mess around in the file:/// uri space. This is wrong in itself, but it will also cause a incorrect verification failure when the archive and the lists directory are on different partitions as rename(). --- apt-pkg/acquire-item.cc | 18 ++++++------------ test/integration/test-apt-update-file | 27 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 12 deletions(-) create mode 100755 test/integration/test-apt-update-file (limited to 'apt-pkg/acquire-item.cc') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 058b8bf74..2ced65aa2 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -1120,12 +1120,6 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, string FileName = LookupTag(Message,"Alt-Filename"); if (FileName.empty() == false) { - // The files timestamp matches - if (StringToBool(LookupTag(Message,"Alt-IMS-Hit"),false) == true) - { - ReverifyAfterIMS(FileName); - return; - } Decompression = true; Local = true; DestFile += ".decomp"; @@ -1142,18 +1136,18 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, ErrorText = "Method gave a blank filename"; } + if (FileName == DestFile) + Erase = true; + else + Local = true; + // The files timestamp matches - if (StringToBool(LookupTag(Message,"IMS-Hit"),false) == true) + if (!Local && StringToBool(LookupTag(Message,"IMS-Hit"),false) == true) { ReverifyAfterIMS(FileName); return; } - if (FileName == DestFile) - Erase = true; - else - Local = true; - string decompProg; // If we enable compressed indexes, queue for hash verification diff --git a/test/integration/test-apt-update-file b/test/integration/test-apt-update-file new file mode 100755 index 000000000..069f8ba2f --- /dev/null +++ b/test/integration/test-apt-update-file @@ -0,0 +1,27 @@ +#!/bin/sh +# +# Ensure that we do not modify file:/// uris (regression test for +# CVE-2014-0487 +# +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework + +setupenvironment +configarchitecture "amd64" +configcompression 'bz2' 'gz' + +insertpackage 'unstable' 'foo' 'all' '1.0' + +umask 022 +setupaptarchive --no-update + +# ensure the archive is not writable +chmod 550 aptarchive/dists/unstable/main/binary-amd64 + +testsuccess aptget update -qq +testsuccess aptget update -qq + +# the cleanup should still work +chmod 750 aptarchive/dists/unstable/main/binary-amd64 -- cgit v1.2.3-70-g09d2 From 23d0a6fbee9e8880107481502e14411961c44a7b Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Wed, 17 Sep 2014 17:48:27 +0200 Subject: improve test for commit daff4a --- apt-pkg/acquire-item.cc | 5 +++-- test/integration/test-apt-update-file | 7 +++++++ 2 files changed, 10 insertions(+), 2 deletions(-) (limited to 'apt-pkg/acquire-item.cc') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 2ced65aa2..5df43726b 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -1141,13 +1141,14 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, else Local = true; - // The files timestamp matches + // The files timestamp matches, for non-local URLs reverify the local + // file, for local file, uncompress again to ensure the hashsum is still + // matching the Release file if (!Local && StringToBool(LookupTag(Message,"IMS-Hit"),false) == true) { ReverifyAfterIMS(FileName); return; } - string decompProg; // If we enable compressed indexes, queue for hash verification diff --git a/test/integration/test-apt-update-file b/test/integration/test-apt-update-file index 069f8ba2f..e267c71da 100755 --- a/test/integration/test-apt-update-file +++ b/test/integration/test-apt-update-file @@ -22,6 +22,13 @@ chmod 550 aptarchive/dists/unstable/main/binary-amd64 testsuccess aptget update -qq testsuccess aptget update -qq +aptget update -qq -o Debug::pkgAcquire::Auth=1 2> output.log + +# ensure that the hash of the uncompressed file was verified even on a local +# ims hit +canary="SHA512:$(bzcat aptarchive/dists/unstable/main/binary-amd64/Packages.bz2 | sha512sum |cut -f1 -d' ')" +grep -q "RecivedHash: $canary" output.log + # the cleanup should still work chmod 750 aptarchive/dists/unstable/main/binary-amd64 -- cgit v1.2.3-70-g09d2 From 801745284905e7962aa77a9f37a6b4e7fcdc19d0 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Sun, 21 Sep 2014 21:23:04 +0200 Subject: Fix regression for cdrom: sources from latest security update Skip a reverify for cdrom: sources. The reverify step is actually harmful here because the apt-cdrom add code uses the indexcopy.cc which will "normalize" the Packages file from the cdrom when it writes it to the local disk. This leads to changing the "MD5sum" field (notice the lower case "s") on the cdrom Packages file to a "MD5Sum" field on the local file in /var/lib/apt/lists. Which of course alters the hash and makes apt fail to reverify the file. --- apt-pkg/acquire-item.cc | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'apt-pkg/acquire-item.cc') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 5df43726b..36c0fa567 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -1141,6 +1141,12 @@ void pkgAcqIndex::Done(string Message,unsigned long long Size,string Hash, else Local = true; + // do not reverify cdrom sources as apt-cdrom may rewrite the Packages + // file when its doing the indexcopy + if (RealURI.substr(0,6) == "cdrom:" && + StringToBool(LookupTag(Message,"IMS-Hit"),false) == true) + return; + // The files timestamp matches, for non-local URLs reverify the local // file, for local file, uncompress again to ensure the hashsum is still // matching the Release file -- cgit v1.2.3-70-g09d2