summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Kalnischkies <david@kalnischkies.de>2016-07-27 15:52:22 +0200
committerJulian Andres Klode <jak@debian.org>2016-08-31 14:16:10 +0200
commit6a6bcc4b3365a4455539271f87be1fa55ea237f1 (patch)
tree3045a95bba5afb085bb3d68a0dfae40404fc4cbb
parenta8c30aa78bf30586ffa47aa9b9a3ff97f763690a (diff)
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 (cherry picked from commit 0e071dfe205ad21d8b929b4bb8164b008dc7c474)
-rw-r--r--apt-pkg/acquire-item.cc30
-rw-r--r--methods/rred.cc4
-rwxr-xr-xtest/integration/test-method-rred3
-rwxr-xr-xtest/integration/test-pdiff-usage20
4 files changed, 37 insertions, 20 deletions
diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 1780df6e7..353ac942f 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -1804,6 +1804,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) /*{{{*/
{
// failing here is fine: our caller will take care of trying to
@@ -2138,23 +2150,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 8573c3922..2002da743 100644
--- a/methods/rred.cc
+++ b/methods/rred.cc
@@ -665,7 +665,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());
@@ -764,7 +764,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 721aa5cdc..5a885e9d2 100755
--- 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 4e2d1f182..dda9e3284 100755
--- 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'
@@ -292,8 +296,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 -o Debug::pkgAcquire::Diffs=1 "$@"
- 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")
@@ -309,15 +312,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() {