rred: truncate result file before writing to it

If another file in the transaction fails and hence dooms the transaction
we can end in a situation in which a -patched file (= rred writes the
result of the patching to it) remains in the partial/ directory.

The next apt call will perform the rred patching again and write its
result again to the -patched file, but instead of starting with an empty
file as intended it will override the content previously in the file
which has the same result if the new content happens to be longer than
the old content, but if it isn't parts of the old content remain in the
file which will pass verification as the new content written to it
matches the hashes and if the entire transaction passes the file will be
moved the lists/ directory where it might or might not trigger errors
depending on if the old content which remained forms a valid file
together with the new content.

This has no real security implications as no untrusted data is involved:
The old content consists of a base file which passed verification and a
bunch of patches which all passed multiple verifications as well, so the
old content isn't controllable by an attacker and the new one isn't
either (as the new content alone passes verification). So the best an
attacker can do is letting the user run into the same issue as in the
report.

Closes: #831762

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 1363933591236559477d9e624fd162c5ea64d7e7..3f1a4e8630c8e4f1dd4b7820e97b36cfbcad6f95 100644 (file)
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -1978,6 +1978,18 @@ void pkgAcqDiffIndex::QueueOnIMSHit() const                              /*{{{*/
    new pkgAcqIndexDiffs(Owner, TransactionManager, Target);
 }
                                                                        /*}}}*/
+static bool RemoveFileForBootstrapLinking(bool const Debug, std::string const &For, std::string const &Boot)/*{{{*/
+{
+   if (FileExists(Boot) && RemoveFile("Bootstrap-linking", Boot) == false)
+   {
+      if (Debug)
+        std::clog << "Bootstrap-linking for patching " << For
+           << " by removing stale " << Boot << " failed!" << std::endl;
+      return false;
+   }
+   return true;
+}
+                                                                       /*}}}*/
 bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)      /*{{{*/
 {
    ExpectedAdditionalItems = 0;
@@ -2318,23 +2330,15 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile)       /*{{{*/
       if (unlikely(Final.empty())) // because we wouldn't be called in such a case
         return false;
       std::string const PartialFile = GetPartialFileNameFromURI(Target.URI);
-      if (FileExists(PartialFile) && RemoveFile("Bootstrap-linking", PartialFile) == false)
-      {
-        if (Debug)
-           std::clog << "Bootstrap-linking for patching " << CurrentPackagesFile
-              << " by removing stale " << PartialFile << " failed!" << std::endl;
+      std::string const PatchedFile = GetKeepCompressedFileName(PartialFile + "-patched", Target);
+      if (RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PartialFile) == false ||
+           RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PatchedFile) == false)
         return false;
-      }
       for (auto const &ext : APT::Configuration::getCompressorExtensions())
       {
-        std::string const Partial = PartialFile + ext;
-        if (FileExists(Partial) && RemoveFile("Bootstrap-linking", Partial) == false)
-        {
-           if (Debug)
-              std::clog << "Bootstrap-linking for patching " << CurrentPackagesFile
-                 << " by removing stale " << Partial << " failed!" << std::endl;
+        if (RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PartialFile + ext) == false ||
+              RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PatchedFile + ext) == false)
            return false;
-        }
       }
       std::string const Ext = Final.substr(CurrentPackagesFile.length());
       std::string const Partial = PartialFile + Ext;

diff --git a/methods/rred.cc b/methods/rred.cc
index 678509844ddb8fad162c609d71714f01265dd2d4..2d2333f9125179b23c2e01366e7f562e1fe97f28 100644 (file)
--- a/methods/rred.cc
+++ b/methods/rred.cc
@@ -664,7 +664,7 @@ class RredMethod : public aptMethod {
            std::cerr << "FAILED to open inp " << Path << std::endl;
            return _error->Error("Failed to open inp %s", Path.c_str());
         }
-        if (out.Open(Itm->DestFile, FileFd::WriteOnly | FileFd::Create | FileFd::BufferedWrite, FileFd::Extension) == false)
+        if (out.Open(Itm->DestFile, FileFd::WriteOnly | FileFd::Create | FileFd::Empty | FileFd::BufferedWrite, FileFd::Extension) == false)
         {
            std::cerr << "FAILED to open out " << Itm->DestFile << std::endl;
            return _error->Error("Failed to open out %s", Itm->DestFile.c_str());
@@ -762,7 +762,7 @@ int main(int argc, char **argv)
       FileFd out, inp;
       std::cerr << "Patching " << argv[2] << " into " << argv[3] << "\n";
       inp.Open(argv[2], FileFd::ReadOnly,FileFd::Extension);
-      out.Open(argv[3], FileFd::WriteOnly | FileFd::Create | FileFd::BufferedWrite, FileFd::Extension);
+      out.Open(argv[3], FileFd::WriteOnly | FileFd::Create | FileFd::Empty | FileFd::BufferedWrite, FileFd::Extension);
       patch.apply_against_file(out, inp);
       out.Close();
    } else if (just_diff) {

diff --git a/test/integration/test-method-rred b/test/integration/test-method-rred
index 721aa5cdc632e2f73107f1e1c7a34a3fac97ee4a..5a885e9d2751a76e28b5e40464e4834d57c306f5 100755 (executable)
--- a/test/integration/test-method-rred
+++ b/test/integration/test-method-rred
@@ -38,6 +38,8 @@ testrred() {
                cat Packages | runapt "${METHODSDIR}/rred" "$@"
        }
        testsuccessequal "$4" --nomsg rred -f Packages.ed
+       testsuccess runapt "${METHODSDIR}/rred" -t Packages Packages-patched Packages.ed
+       testfileequal Packages-patched "$4"
 }
 
 testrred 'Remove' 'first line' '1d' "$(tail -n +2 ./Packages)"
@@ -152,6 +154,7 @@ failrred() {
                cat Packages | runapt "${METHODSDIR}/rred" "$@"
        }
        testfailure --nomsg rred -f Packages.ed
+       testfailure runapt "${METHODSDIR}/rred" -t Packages Packages-patched Packages.ed
 }
 
 failrred 'Bogus content' '<html>

diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage
index 9c7946083d3788b7d9616cee65ebb7d0923489f0..91528389b967666a3a386c3b53e136a7af2d5027 100755 (executable)
--- a/test/integration/test-pdiff-usage
+++ b/test/integration/test-pdiff-usage
@@ -30,6 +30,10 @@ echo 'hacked' > aptarchive/hacked-i386
 compressfile aptarchive/hacked-i386
 
 wasmergeused() {
+       if echo "$*" | grep -q -- '-o test::cannot-use-pdiff=1'; then
+               find rootdir/var/lib/apt/lists/partial -name '*-patched*' -delete
+       fi
+
        testsuccess apt update "$@"
 
        msgtest 'No intermediate patch files' 'still exist'
@@ -302,8 +306,7 @@ SHA256-Download:
  $(sha256sum "${PATCHFILE}.gz" | cut -d' ' -f 1) $(stat -c%s "${PATCHFILE}.gz")000 $(basename "${PATCHFILE}.gz")" > "$PATCHINDEX"
        generatereleasefiles '+1hour'
        signreleasefiles
-       testsuccess apt update "$@"
-       cp -f rootdir/tmp/testsuccess.output rootdir/tmp/aptupdate.output
+       wasmergeused "$@" -o test::cannot-use-pdiff=1
        testsuccess grep 'bytes (Limit is' rootdir/tmp/aptupdate.output
        testnopackage oldstuff
        testsuccessequal "$(cat "${PKGFILE}-new")
@@ -322,15 +325,22 @@ testcase() {
        testrun -o Acquire::PDiffs::Merge=0 -o APT::Get::List-Cleanup=0 "$@"
        testrun -o Acquire::PDiffs::Merge=1 -o APT::Get::List-Cleanup=0 "$@"
 }
-partialleftovers() { touch "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_Packages"; }
+generatepartialleftovers() {
+       for f in "$@"; do
+               cat "${PKGFILE}" "${PKGFILE}" > "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_${f}"
+               printf '\n\nInvalid\nStanza: yes\n\n' >> "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_${f}"
+       done
+}
+
+partialleftovers() { generatepartialleftovers 'Packages' 'Packages-patched'; }
 aptautotest_apt_update() { aptautotest_aptget_update "$@"; testsuccess test -e "rootdir/var/lib/apt/lists/localhost:${APTHTTPPORT}_Packages"; }
 testcase -o Acquire::IndexTargets::deb::Packages::KeepCompressed=false
-partialleftovers() { touch "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_Packages.$LOWCOSTEXT"; }
+partialleftovers() { generatepartialleftovers "Packages.${LOWCOSTEXT}" "Packages-patched.${LOWCOSTEXT}"; }
 aptautotest_apt_update() { aptautotest_aptget_update "$@"; testsuccess test -e "rootdir/var/lib/apt/lists/localhost:${APTHTTPPORT}_Packages.$LOWCOSTEXT"; }
 testcase -o Acquire::IndexTargets::deb::Packages::KeepCompressed=true
 
 
-partialleftovers() { touch "rootdir/var/lib/apt/lists-bak/partial/localhost:${APTHTTPPORT}_redirectme_Packages.$LOWCOSTEXT"; }
+partialleftovers() { generatepartialleftovers "redirectme_Packages.${LOWCOSTEXT}" "redirectme_Packages-patched.${LOWCOSTEXT}"; }
 webserverconfig 'aptwebserver::redirect::replace::/redirectme/' "http://0.0.0.0:${APTHTTPPORT}/"
 rewritesourceslist "http://localhost:${APTHTTPPORT}/redirectme"
 aptautotest_apt_update() {