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.

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index ec6ec6e84daf40349a5e383a695d20e158475c12..7b69ee9937ffe72e4f7652d5abb1a7b3135f4717 100644 (file)
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -95,6 +95,19 @@ static std::string GetCompressedFileName(std::string const &URI, std::string con
    return Name;
 }
                                                                        /*}}}*/
    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)
 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);
                << "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();
 }
    new pkgAcqIndex(Owner, TransactionManager, Target);
    Finish();
 }


@@ -1968,28 +1984,13 @@ void pkgAcqIndexDiffs::Done(string const &Message, HashStringList const &Hashes,
 
    Item::Done(Message, Hashes, Cnf);
 
 
    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 FinalFile = GetPartialFileNameFromURI(Target->URI);

+   std::string const PatchFile = GetDiffsPatchFileName(FinalFile);

 
    // success in downloading a diff, enter ApplyDiff state
    if(State == StateFetchDiff)
    {
 
    // 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;
 
       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;
       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());
 
    // 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
 
       // move into place

-      if(Debug) 
+      if(Debug)

       {
         std::clog << "Moving patched file in place: " << std::endl
                   << DestFile << " -> " << FinalFile << std::endl;
       {
         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,
 
 // 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::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);
 }
                                                                        /*}}}*/
    new pkgAcqIndex(Owner, TransactionManager, Target);
 }
                                                                        /*}}}*/


@@ -2090,26 +2105,10 @@ void pkgAcqIndexMergeDiffs::Done(string const &Message, HashStringList const &Ha
 
    Item::Done(Message, Hashes, Cnf);
 
 
    Item::Done(Message, Hashes, Cnf);
 

-   // FIXME: verify download before feeding it to rred

    string const FinalFile = GetPartialFileNameFromURI(Target->URI);
    string const FinalFile = GetPartialFileNameFromURI(Target->URI);

-

    if (State == StateFetchDiff)
    {
    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;
 
       // 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);
            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());
         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,
 
 // AcqIndex::AcqIndex - Constructor                                    /*{{{*/
 pkgAcqIndex::pkgAcqIndex(pkgAcquire * const Owner,

diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 97d5ea1dd941bd03fe8ef4ea0261c9e8b8cd4d49..f24af1aeca81520f116e1959a1ae8e19524b6a73 100644 (file)
--- a/apt-pkg/acquire-item.h
+++ b/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 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;
    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 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;
    virtual std::string DescURI() const {return Target->URI + "IndexDiffs";};
    virtual HashStringList GetExpectedHashes() const;
    virtual bool HashesRequired() const;

diff --git a/apt-pkg/acquire-method.cc b/apt-pkg/acquire-method.cc
index b77096efd652b319676dbfec109a5e64f3e1752f..a8fc75f8eccec894946d040f69ba70298f6a6972 100644 (file)
--- a/apt-pkg/acquire-method.cc
+++ b/apt-pkg/acquire-method.cc
@@ -388,14 +388,14 @@ int pkgAcqMethod::Run(bool Single)
            *I = Tmp;
            if (QueueBack == 0)
               QueueBack = Tmp;
            *I = Tmp;
            if (QueueBack == 0)
               QueueBack = Tmp;

-           
+

            // Notify that this item is to be fetched.
            // Notify that this item is to be fetched.

-           if (Fetch(Tmp) == false)
+           if (URIAcquire(Message, Tmp) == false)

               Fail();
               Fail();

-           
-           break;                                           
-        }   
-      }      
+
+           break;
+        }
+      }

    }
 
    Exit();
    }
 
    Exit();


@@ -403,8 +403,6 @@ int pkgAcqMethod::Run(bool Single)
 }
                                                                        /*}}}*/
 // AcqMethod::PrintStatus - privately really send a log/status message /*{{{*/
 }
                                                                        /*}}}*/
 // AcqMethod::PrintStatus - privately really send a log/status message /*{{{*/

-// ---------------------------------------------------------------------
-/* */

 void pkgAcqMethod::PrintStatus(char const * const header, const char* Format,
                               va_list &args) const
 {
 void pkgAcqMethod::PrintStatus(char const * const header, const char* Format,
                               va_list &args) const
 {

diff --git a/apt-pkg/acquire-method.h b/apt-pkg/acquire-method.h
index 399454892997cc72ca401224b48a1e26e4569a43..6480eb4b57f4ea3dad497bba344019a7a84fc9fa 100644 (file)
--- a/apt-pkg/acquire-method.h
+++ b/apt-pkg/acquire-method.h
@@ -76,11 +76,12 @@ class pkgAcqMethod
    std::string FailReason;
    std::string UsedMirror;
    std::string IP;
    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;};
    // 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);};
    // Outgoing messages
    void Fail(bool Transient = false);
    inline void Fail(const char *Why, bool Transient = false) {Fail(std::string(Why),Transient);};

diff --git a/methods/rred.cc b/methods/rred.cc
index 554ac99b4e3aed7ee22528a52917646957f8b35e..3da33c1267ba6414cc0e1dfafdfb8018c7d7f932 100644 (file)
--- a/methods/rred.cc
+++ b/methods/rred.cc
@@ -388,7 +388,7 @@ class Patch {
 
    public:
 
 
    public:
 

-   void read_diff(FileFd &f)
+   void read_diff(FileFd &f, Hashes * const h)

    {
       char buffer[BLOCK_SIZE];
       bool cmdwanted = true;
    {
       char buffer[BLOCK_SIZE];
       bool cmdwanted = true;


@@ -396,6 +396,8 @@ class Patch {
       Change ch(0);
       while(f.ReadLine(buffer, sizeof(buffer)))
       {
       Change ch(0);
       while(f.ReadLine(buffer, sizeof(buffer)))
       {

+        if (h != NULL)
+           h->Add(buffer);

         if (cmdwanted) {
            char *m, *c;
            size_t s, e;
         if (cmdwanted) {
            char *m, *c;
            size_t s, e;


@@ -519,8 +521,29 @@ class RredMethod : public pkgAcqMethod {
    private:
       bool Debug;
 
    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:
    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
         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);
 
         } else
            URIStart(Res);
 

-        std::vector<std::string> patchpaths;
+        std::vector<PDiffFile> patchfiles;

         Patch patch;
 
         if (FileExists(Path + ".ed") == true)
         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();
         else
         {
            _error->PushToStack();


@@ -546,18 +575,27 @@ class RredMethod : public pkgAcqMethod {
            _error->RevertToStack();
 
            std::string const baseName = Path + ".ed.";
            _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)
            for (std::vector<std::string>::const_iterator p = patches.begin();
                  p != patches.end(); ++p)

+           {

               if (p->compare(0, baseName.length(), baseName) == 0)
               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;
         }
 
         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)
         {
               ++I)
         {

-           patch_name = *I;
+           patch_name = I->FileName;

            if (Debug == true)
               std::clog << "Patching " << Path << " with " << patch_name
                  << std::endl;
            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();
            }
               _error->DumpErrors(std::cerr);
               abort();
            }

-           patch.read_diff(p);
+           Hashes patch_hash(I->ExpectedHashes);
+           patch.read_diff(p, &patch_hash);

            p.Close();
            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)
         }
 
         if (Debug == true)


@@ -643,7 +685,7 @@ int main(int argc, char **argv)
         _error->DumpErrors(std::cerr);
         exit(1);
       }
         _error->DumpErrors(std::cerr);
         exit(1);
       }

-      patch.read_diff(p);
+      patch.read_diff(p, NULL);

    }
 
    if (just_diff) {
    }
 
    if (just_diff) {

diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage
index 7d72a6944d2534deb33b5e5dd7416e40f65e5cf6..73df61895d5a802a01a8024f708aa14d8183fd5e 100755 (executable)
--- a/test/integration/test-pdiff-usage
+++ b/test/integration/test-pdiff-usage
@@ -170,6 +170,8 @@ SHA256-Patches:
        generatereleasefiles '+1hour'
        signreleasefiles
        testsuccess aptget update "$@"
        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
        testnopackage oldstuff
        testsuccessequal "$(cat ${PKGFILE}-new)
 " aptcache show apt newstuff