From 3679515479136179e0d95325a6559fcc6d0af7f8 Mon Sep 17 00:00:00 2001
From: David Kalnischkies <david@kalnischkies.de>
Date: Sat, 6 Jun 2015 19:16:45 +0200
Subject: check patch hashes in rred worker instead of in the handler
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 methods/rred.cc | 62 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 10 deletions(-)

(limited to 'methods/rred.cc')

diff --git a/methods/rred.cc b/methods/rred.cc
index 554ac99b4..3da33c126 100644
--- a/methods/rred.cc
+++ b/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) {
-- 
cgit v1.2.3