check patch hashes in rred worker instead of in the handler

rred is responsible for unpacking and reading the patch files in one go,
but we currently only have hashes for the uncompressed patch files, so
the handler read the entire patch file before dispatching it to the
worker which would read it again – both with an implicit uncompress.
Worse, while the workers operate in parallel the handler is the central
orchestration unit, so having it busy with work means the workers do
(potentially) nothing.

This means rred is working with 'untrusted' data, which is bad. Yet,
having the unpack in the handler meant that the untrusted uncompress was
done as root which isn't better either. Now, we have it at least
contained in a binary which we can harden a bit better. In the long run,
we want hashes for the compressed patch files through to be safe.

apt-pkg/acquire-item.cc

@@ -95,6 +95,19 @@ static std::string GetCompressedFileName(std::string const &URI, std::string con
    return Name;
 }
 									/*}}}*/
+static std::string GetMergeDiffsPatchFileName(std::string const &Final, std::string const &Patch)/*{{{*/
+{
+   // rred expects the patch as $FinalFile.ed.$patchname.gz
+   return Final + ".ed." + Patch + ".gz";
+}
+									/*}}}*/
+static std::string GetDiffsPatchFileName(std::string const &Final)	/*{{{*/
+{
+   // rred expects the patch as $FinalFile.ed
+   return Final + ".ed";
+}
+									/*}}}*/
+
 static bool AllowInsecureRepositories(indexRecords const * const MetaIndexParser, pkgAcqMetaBase * const TransactionManager, pkgAcquire::Item * const I) /*{{{*/
 {
    if(MetaIndexParser->IsAlwaysTrusted() || _config->FindB("Acquire::AllowInsecureRepositories") == true)
@@ -1860,6 +1873,9 @@ void pkgAcqIndexDiffs::Failed(string const &Message,pkgAcquire::MethodConfig con
 		<< "Falling back to normal index file acquire" << std::endl;
    DestFile = GetPartialFileNameFromURI(Target->URI);
    RenameOnError(PDiffError);
+   std::string const patchname = GetDiffsPatchFileName(DestFile);
+   if (RealFileExists(patchname))
+      rename(patchname.c_str(), std::string(patchname + ".FAILED").c_str());
    new pkgAcqIndex(Owner, TransactionManager, Target);
    Finish();
 }
@@ -1968,28 +1984,13 @@ void pkgAcqIndexDiffs::Done(string const &Message, HashStringList const &Hashes,
 
    Item::Done(Message, Hashes, Cnf);
 
-   // FIXME: verify this download too before feeding it to rred
    std::string const FinalFile = GetPartialFileNameFromURI(Target->URI);
+   std::string const PatchFile = GetDiffsPatchFileName(FinalFile);
 
    // success in downloading a diff, enter ApplyDiff state
    if(State == StateFetchDiff)
    {
-      FileFd fd(DestFile, FileFd::ReadOnly, FileFd::Gzip);
-      class Hashes LocalHashesCalc;
-      LocalHashesCalc.AddFD(fd);
-      HashStringList const LocalHashes = LocalHashesCalc.GetHashStringList();
-
-      if (fd.Size() != available_patches[0].patch_size ||
-	    available_patches[0].patch_hashes != LocalHashes)
-      {
-	 // patchfiles are dated, so bad indicates a bad download, so kill it
-	 unlink(DestFile.c_str());
-	 Failed("Patch has Size/Hashsum mismatch", NULL);
-	 return;
-      }
-
-      // rred excepts the patch as $FinalFile.ed
-      Rename(DestFile,FinalFile+".ed");
+      Rename(DestFile, PatchFile);
 
       if(Debug)
 	 std::clog << "Sending to rred method: " << FinalFile << std::endl;
@@ -2000,18 +2001,17 @@ void pkgAcqIndexDiffs::Done(string const &Message, HashStringList const &Hashes,
       QueueURI(Desc);
       SetActiveSubprocess("rred");
       return;
-   } 
-
+   }
 
    // success in download/apply a diff, queue next (if needed)
    if(State == StateApplyDiff)
    {
       // remove the just applied patch
       available_patches.erase(available_patches.begin());
-      unlink((FinalFile + ".ed").c_str());
+      unlink(PatchFile.c_str());
 
       // move into place
-      if(Debug) 
+      if(Debug)
       {
 	 std::clog << "Moving patched file in place: " << std::endl
 		   << DestFile << " -> " << FinalFile << std::endl;
@@ -2031,6 +2031,18 @@ void pkgAcqIndexDiffs::Done(string const &Message, HashStringList const &Hashes,
    }
 }
 									/*}}}*/
+std::string pkgAcqIndexDiffs::Custom600Headers() const			/*{{{*/
+{
+   if(State != StateApplyDiff)
+      return pkgAcqBaseIndex::Custom600Headers();
+   std::ostringstream patchhashes;
+   HashStringList const ExpectedHashes = available_patches[0].patch_hashes;
+   for (HashStringList::const_iterator hs = ExpectedHashes.begin(); hs != ExpectedHashes.end(); ++hs)
+      patchhashes <<  "\nPatch-0-" << hs->HashType() << "-Hash: " << hs->HashValue();
+   patchhashes << pkgAcqBaseIndex::Custom600Headers();
+   return patchhashes.str();
+}
+									/*}}}*/
 
 // AcqIndexMergeDiffs::AcqIndexMergeDiffs - Constructor			/*{{{*/
 pkgAcqIndexMergeDiffs::pkgAcqIndexMergeDiffs(pkgAcquire * const Owner,
@@ -2079,6 +2091,9 @@ void pkgAcqIndexMergeDiffs::Failed(string const &Message,pkgAcquire::MethodConfi
       std::clog << "Falling back to normal index file acquire" << std::endl;
    DestFile = GetPartialFileNameFromURI(Target->URI);
    RenameOnError(PDiffError);
+   std::string const patchname = GetMergeDiffsPatchFileName(DestFile, patch.file);
+   if (RealFileExists(patchname))
+      rename(patchname.c_str(), std::string(patchname + ".FAILED").c_str());
    new pkgAcqIndex(Owner, TransactionManager, Target);
 }
 									/*}}}*/
@@ -2090,26 +2105,10 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
 
    Item::Done(Message, Hashes, Cnf);
 
-   // FIXME: verify download before feeding it to rred
    string const FinalFile = GetPartialFileNameFromURI(Target->URI);
-
    if (State == StateFetchDiff)
    {
-      FileFd fd(DestFile, FileFd::ReadOnly, FileFd::Gzip);
-      class Hashes LocalHashesCalc;
-      LocalHashesCalc.AddFD(fd);
-      HashStringList const LocalHashes = LocalHashesCalc.GetHashStringList();
-
-      if (fd.Size() != patch.patch_size || patch.patch_hashes != LocalHashes)
-      {
-	 // patchfiles are dated, so bad indicates a bad download, so kill it
-	 unlink(DestFile.c_str());
-	 Failed("Patch has Size/Hashsum mismatch", NULL);
-	 return;
-      }
-
-      // rred expects the patch as $FinalFile.ed.$patchname.gz
-      Rename(DestFile, FinalFile + ".ed." + patch.file + ".gz");
+      Rename(DestFile, GetMergeDiffsPatchFileName(FinalFile, patch.file));
 
       // check if this is the last completed diff
       State = StateDoneDiff;
@@ -2158,7 +2157,7 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
 	    I != allPatches->end(); ++I)
       {
 	 std::string const PartialFile = GetPartialFileNameFromURI(Target->URI);
-	 std::string patch = PartialFile + ".ed." + (*I)->patch.file + ".gz";
+	 std::string const patch = GetMergeDiffsPatchFileName(PartialFile, (*I)->patch.file);
 	 unlink(patch.c_str());
       }
       unlink(FinalFile.c_str());
@@ -2170,6 +2169,24 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
    }
 }
 									/*}}}*/
+std::string pkgAcqIndexMergeDiffs::Custom600Headers() const		/*{{{*/
+{
+   if(State != StateApplyDiff)
+      return pkgAcqBaseIndex::Custom600Headers();
+   std::ostringstream patchhashes;
+   unsigned int seen_patches = 0;
+   for (std::vector<pkgAcqIndexMergeDiffs *>::const_iterator I = allPatches->begin();
+	 I != allPatches->end(); ++I)
+   {
+      HashStringList const ExpectedHashes = (*I)->patch.patch_hashes;
+      for (HashStringList::const_iterator hs = ExpectedHashes.begin(); hs != ExpectedHashes.end(); ++hs)
+	 patchhashes <<  "\nPatch-" << seen_patches << "-" << hs->HashType() << "-Hash: " << hs->HashValue();
+      ++seen_patches;
+   }
+   patchhashes << pkgAcqBaseIndex::Custom600Headers();
+   return patchhashes.str();
+}
+									/*}}}*/
 
 // AcqIndex::AcqIndex - Constructor					/*{{{*/
 pkgAcqIndex::pkgAcqIndex(pkgAcquire * const Owner,

apt-pkg/acquire-item.h

@@ -774,6 +774,7 @@ class APT_HIDDEN pkgAcqIndexMergeDiffs : public pkgAcqBaseIndex
    virtual void Failed(std::string const &Message,pkgAcquire::MethodConfig const * const Cnf);
    virtual void Done(std::string const &Message, HashStringList const &Hashes,
 	 pkgAcquire::MethodConfig const * const Cnf);
+   virtual std::string Custom600Headers() const;
    virtual std::string DescURI() const {return Target->URI + "Index";};
    virtual HashStringList GetExpectedHashes() const;
    virtual bool HashesRequired() const;
@@ -886,6 +887,7 @@ class APT_HIDDEN pkgAcqIndexDiffs : public pkgAcqBaseIndex
 
    virtual void Done(std::string const &Message, HashStringList const &Hashes,
 		     pkgAcquire::MethodConfig const * const Cnf);
+   virtual std::string Custom600Headers() const;
    virtual std::string DescURI() const {return Target->URI + "IndexDiffs";};
    virtual HashStringList GetExpectedHashes() const;
    virtual bool HashesRequired() const;

apt-pkg/acquire-method.cc

@@ -388,14 +388,14 @@ int pkgAcqMethod::Run(bool Single)
 	    *I = Tmp;
 	    if (QueueBack == 0)
 	       QueueBack = Tmp;
-	    
+
 	    // Notify that this item is to be fetched.
-	    if (Fetch(Tmp) == false)
+	    if (URIAcquire(Message, Tmp) == false)
 	       Fail();
-	    
-	    break;					     
-	 }   
-      }      
+
+	    break;
+	 }
+      }
    }
 
    Exit();
@@ -403,8 +403,6 @@ int pkgAcqMethod::Run(bool Single)
 }
 									/*}}}*/
 // AcqMethod::PrintStatus - privately really send a log/status message	/*{{{*/
-// ---------------------------------------------------------------------
-/* */
 void pkgAcqMethod::PrintStatus(char const * const header, const char* Format,
 			       va_list &args) const
 {

apt-pkg/acquire-method.h

@@ -76,11 +76,12 @@ class pkgAcqMethod
    std::string FailReason;
    std::string UsedMirror;
    std::string IP;
-   
+
    // Handlers for messages
    virtual bool Configuration(std::string Message);
    virtual bool Fetch(FetchItem * /*Item*/) {return true;};
-   
+   virtual bool URIAcquire(std::string const &/*Message*/, FetchItem *Itm) { return Fetch(Itm); };
+
    // Outgoing messages
    void Fail(bool Transient = false);
    inline void Fail(const char *Why, bool Transient = false) {Fail(std::string(Why),Transient);};

methods/rred.cc

@@ -388,7 +388,7 @@ class Patch {
 
    public:
 
-   void read_diff(FileFd &f)
+   void read_diff(FileFd &f, Hashes * const h)
    {
       char buffer[BLOCK_SIZE];
       bool cmdwanted = true;
@@ -396,6 +396,8 @@ class Patch {
       Change ch(0);
       while(f.ReadLine(buffer, sizeof(buffer)))
       {
+	 if (h != NULL)
+	    h->Add(buffer);
 	 if (cmdwanted) {
 	    char *m, *c;
 	    size_t s, e;
@@ -519,8 +521,29 @@ class RredMethod : public pkgAcqMethod {
    private:
       bool Debug;
 
+      struct PDiffFile {
+	 std::string FileName;
+	 HashStringList ExpectedHashes;
+	 PDiffFile(std::string const &FileName, HashStringList const &ExpectedHashes) :
+	    FileName(FileName), ExpectedHashes(ExpectedHashes) {}
+      };
+
+      HashStringList ReadExpectedHashesForPatch(unsigned int const patch, std::string const &Message)
+      {
+	 HashStringList ExpectedHashes;
+	 for (char const * const * type = HashString::SupportedHashes(); *type != NULL; ++type)
+	 {
+	    std::string tagname;
+	    strprintf(tagname, "Patch-%d-%s-Hash", patch, *type);
+	    std::string const hashsum = LookupTag(Message, tagname.c_str());
+	    if (hashsum.empty() == false)
+	       ExpectedHashes.push_back(HashString(*type, hashsum));
+	 }
+	 return ExpectedHashes;
+      }
+
    protected:
-      virtual bool Fetch(FetchItem *Itm) {
+      virtual bool URIAcquire(std::string const &Message, FetchItem *Itm) {
 	 Debug = _config->FindB("Debug::pkgAcquire::RRed", false);
 	 URI Get = Itm->Uri;
 	 std::string Path = Get.Host + Get.Path; // rred:/path - no host
@@ -534,11 +557,17 @@ class RredMethod : public pkgAcqMethod {
 	 } else
 	    URIStart(Res);
 
-	 std::vector<std::string> patchpaths;
+	 std::vector<PDiffFile> patchfiles;
 	 Patch patch;
 
 	 if (FileExists(Path + ".ed") == true)
-	    patchpaths.push_back(Path + ".ed");
+	 {
+	    HashStringList const ExpectedHashes = ReadExpectedHashesForPatch(0, Message);
+	    std::string const FileName = Path + ".ed";
+	    if (ExpectedHashes.usable() == false)
+	       return _error->Error("No hashes found for uncompressed patch: %s", FileName.c_str());
+	    patchfiles.push_back(PDiffFile(FileName, ExpectedHashes));
+	 }
 	 else
 	 {
 	    _error->PushToStack();
@@ -546,18 +575,27 @@ class RredMethod : public pkgAcqMethod {
 	    _error->RevertToStack();
 
 	    std::string const baseName = Path + ".ed.";
+	    unsigned int seen_patches = 0;
 	    for (std::vector<std::string>::const_iterator p = patches.begin();
 		  p != patches.end(); ++p)
+	    {
 	       if (p->compare(0, baseName.length(), baseName) == 0)
-		  patchpaths.push_back(*p);
+	       {
+		  HashStringList const ExpectedHashes = ReadExpectedHashesForPatch(seen_patches, Message);
+		  if (ExpectedHashes.usable() == false)
+		     return _error->Error("No hashes found for uncompressed patch %d: %s", seen_patches, p->c_str());
+		  patchfiles.push_back(PDiffFile(*p, ExpectedHashes));
+		  ++seen_patches;
+	       }
+	    }
 	 }
 
 	 std::string patch_name;
-	 for (std::vector<std::string>::iterator I = patchpaths.begin();
-	       I != patchpaths.end();
+	 for (std::vector<PDiffFile>::iterator I = patchfiles.begin();
+	       I != patchfiles.end();
 	       ++I)
 	 {
-	    patch_name = *I;
+	    patch_name = I->FileName;
 	    if (Debug == true)
 	       std::clog << "Patching " << Path << " with " << patch_name
 		  << std::endl;
@@ -569,8 +607,12 @@ class RredMethod : public pkgAcqMethod {
 	       _error->DumpErrors(std::cerr);
 	       abort();
 	    }
-	    patch.read_diff(p);
+	    Hashes patch_hash(I->ExpectedHashes);
+	    patch.read_diff(p, &patch_hash);
 	    p.Close();
+	    HashStringList const hsl = patch_hash.GetHashStringList();
+	    if (hsl != I->ExpectedHashes)
+	       return _error->Error("Patch %s doesn't have the expected hashsum", patch_name.c_str());
 	 }
 
 	 if (Debug == true)
@@ -643,7 +685,7 @@ int main(int argc, char **argv)
 	 _error->DumpErrors(std::cerr);
 	 exit(1);
       }
-      patch.read_diff(p);
+      patch.read_diff(p, NULL);
    }
 
    if (just_diff) {

test/integration/test-pdiff-usage

@@ -170,6 +170,8 @@ SHA256-Patches:
 	generatereleasefiles '+1hour'
 	signreleasefiles
 	testsuccess aptget update "$@"
+	cp -f rootdir/tmp/testsuccess.output rootdir/tmp/aptgetupdate.output
+	testsuccess grep 'have the expected hashsum' rootdir/tmp/aptgetupdate.output
 	testnopackage oldstuff
 	testsuccessequal "$(cat ${PKGFILE}-new)
 " aptcache show apt newstuff