From 149b23c2b9697bc262c0af1934c7a3f6114d903f Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 4 Jun 2021 13:06:34 +0200 Subject: URI encode Filename field of Packages files (again) Keeping URIs encoded in the acquire system depends on having them encoded in the first place. While many other places got the encoding 2 out of 3 ArchiveURI implementations were missed which are in practice responsible for nearly all of the URI building, just that index filename do not contain characters to escape and the Filename fields in Packages files usually aren't. Usually. Except if you happen to have e.g. an epoch featuring package with the colon encoded in the filename. On the upside, in most repositories the epoch isn't part of the filename. Reported-By: Johannes 'josch' Schauer on IRC References: e6c55283d235aa9404395d30f2db891f36995c49 --- apt-pkg/acquire-item.cc | 12 +++---- apt-pkg/indexfile.cc | 5 +-- test/integration/test-apt-get-install-deb | 11 +++--- .../test-bug-723705-tagfile-truncates-fields | 4 +-- test/integration/test-uri-encode-filename-field | 42 ++++++++++++++++++++++ 5 files changed, 59 insertions(+), 15 deletions(-) create mode 100755 test/integration/test-uri-encode-filename-field diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 225c4304a..d6ffaf34d 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -3853,16 +3853,16 @@ pkgAcqFile::pkgAcqFile(pkgAcquire *const Owner, string const &URI, HashStringLis const string &DestDir, const string &DestFilename, bool const IsIndexFile) : Item(Owner), d(NULL), IsIndexFile(IsIndexFile), ExpectedHashes(Hashes) { + ::URI url{URI}; + if (url.Path.find(' ') != std::string::npos || url.Path.find('%') == std::string::npos) + url.Path = pkgAcquire::URIEncode(url.Path); + if(!DestFilename.empty()) DestFile = DestFilename; else if(!DestDir.empty()) - DestFile = DestDir + "/" + flNotDir(URI); + DestFile = DestDir + "/" + DeQuoteString(flNotDir(url.Path)); else - DestFile = flNotDir(URI); - - ::URI url{URI}; - if (url.Path.find(' ') != std::string::npos || url.Path.find('%') == std::string::npos) - url.Path = pkgAcquire::URIEncode(url.Path); + DestFile = DeQuoteString(flNotDir(url.Path)); // Create the item Desc.URI = std::string(url); diff --git a/apt-pkg/indexfile.cc b/apt-pkg/indexfile.cc index ef3486ab8..f13b52940 100644 --- a/apt-pkg/indexfile.cc +++ b/apt-pkg/indexfile.cc @@ -9,6 +9,7 @@ // Include Files /*{{{*/ #include +#include #include #include #include @@ -174,7 +175,7 @@ pkgDebianIndexTargetFile::pkgDebianIndexTargetFile(IndexTarget const &Target, bo /*}}}*/ std::string pkgDebianIndexTargetFile::ArchiveURI(std::string const &File) const/*{{{*/ { - return Target.Option(IndexTarget::REPO_URI) + File; + return Target.Option(IndexTarget::REPO_URI) + pkgAcquire::URIEncode(File); } /*}}}*/ std::string pkgDebianIndexTargetFile::Describe(bool const Short) const /*{{{*/ @@ -281,7 +282,7 @@ std::string pkgDebianIndexRealFile::Describe(bool const /*Short*/) const/*{{{*/ /*}}}*/ std::string pkgDebianIndexRealFile::ArchiveURI(std::string const &/*File*/) const/*{{{*/ { - return "file:" + File; + return "file:" + pkgAcquire::URIEncode(File); } /*}}}*/ std::string pkgDebianIndexRealFile::IndexFileName() const /*{{{*/ diff --git a/test/integration/test-apt-get-install-deb b/test/integration/test-apt-get-install-deb index 6fde00708..3d0b9a80d 100755 --- a/test/integration/test-apt-get-install-deb +++ b/test/integration/test-apt-get-install-deb @@ -209,30 +209,31 @@ echo "Package: pkg-as-it-should-be Architecture: all Version: 0 Installed-Size: 2903 -Filename: incoming/pkg-as-it-should-be_0_all.deb +Filename: incoming/pkg-as-it-should-be_0%3a0+0_all.deb Size: $(stat -c %s incoming/pkg-as-it-should-be_0_all.deb) SHA256: $(sha256sum incoming/pkg-as-it-should-be_0_all.deb | cut -d' ' -f 1) " > Packages +ln -s pkg-as-it-should-be_0_all.deb incoming/pkg-as-it-should-be_0%3a0+0_all.deb testdpkgnotinstalled 'pkg-as-it-should-be' testnopackage pkg-as-it-should-be testsuccess apt install --with-source ./Packages pkg-as-it-should-be -s testsuccess apt install --with-source ./Packages pkg-as-it-should-be --print-uris testsuccess apt show --with-source ./Packages pkg-as-it-should-be testequal 'Package: pkg-as-it-should-be' head -n1 rootdir/tmp/testsuccess.output -testsuccess apt install -y --with-source ./Packages pkg-as-it-should-be +testsuccess apt install -y --with-source ./Packages pkg-as-it-should-be -o Debug::pkgAcquire::Worker=1 testdpkginstalled 'pkg-as-it-should-be' rm -f ./Packages echo 'dpkg::install::recursive "true"; dpkg::install::recursive::force "true"; dpkg::install::recursive::minimum "0";' > rootdir/etc/apt/apt.conf.d/lowerminimum.conf -mv ./incoming/pkg-as-it-should-be_0_all.deb ./incoming/pkg-as-it-should-be_0_all.ddeb -testsuccess aptget install -y ./incoming/pkg-as-it-should-be_0_all.ddeb --reinstall +mv ./incoming/pkg-as-it-should-be_0_all.deb ./incoming/pkg-as-it-should-be_0%3a0+0_all.ddeb +testsuccess aptget install -y ./incoming/pkg-as-it-should-be_0%3a0+0_all.ddeb --reinstall testfailure grep 'is already the newest version' rootdir/tmp/testsuccess.output testsuccess apt purge -y pkg-as-it-should-be testdpkgnotinstalled 'pkg-as-it-should-be' -mv ./incoming/pkg-as-it-should-be_0_all.ddeb ./incoming/pkg-as-it-should-be_0_all.foobar +mv ./incoming/pkg-as-it-should-be_0%3a0+0_all.ddeb ./incoming/pkg-as-it-should-be_0_all.foobar echo "Package: pkg-as-it-should-be Architecture: all Version: 0 diff --git a/test/integration/test-bug-723705-tagfile-truncates-fields b/test/integration/test-bug-723705-tagfile-truncates-fields index bfa3598ae..d167a3f65 100755 --- a/test/integration/test-bug-723705-tagfile-truncates-fields +++ b/test/integration/test-bug-723705-tagfile-truncates-fields @@ -26,8 +26,8 @@ After this operation, 19.8 MB of additional disk space will be used. 'file:///tmp/aptarchive/pool/main/c/cdebconf/cdebconf-gtk-udeb_0.185_amd64.udeb' cdebconf-gtk-udeb_0.185_amd64.udeb 27278 MD5Sum:a1bbbc1d4fb8e0615b5621abac021924 'file:///tmp/aptarchive/pool/main/c/cdebconf/cdebconf-newt-udeb_0.185_amd64.udeb' cdebconf-newt-udeb_0.185_amd64.udeb 19192 MD5Sum:de27807f56dae2f2403b3322d5fe6bd2 'file:///tmp/aptarchive/pool/main/g/glib2.0/libglib2.0-udeb_2.36.4-1_amd64.udeb' libglib2.0-udeb_2.36.4-1_amd64.udeb 1714604 MD5Sum:72da029f1bbb36057d874f1f82a5d00a -'file:///tmp/aptarchive/pool/main/e/eglibc/libc6-udeb_2.17-92+b1_amd64.udeb' libc6-udeb_2.17-92+b1_amd64.udeb 1056000 MD5Sum:7fd7032eeeecf7f76eff79a0543fbd72 -'file:///tmp/aptarchive/pool/main/g/gtk+2.0/libgtk2.0-0-udeb_2.24.20-1_amd64.udeb' libgtk2.0-0-udeb_2.24.20-1_amd64.udeb 1643046 MD5Sum:25513478eb2e02e5766c0eea0b411ca9 +'file:///tmp/aptarchive/pool/main/e/eglibc/libc6-udeb_2.17-92%2bb1_amd64.udeb' libc6-udeb_2.17-92+b1_amd64.udeb 1056000 MD5Sum:7fd7032eeeecf7f76eff79a0543fbd72 +'file:///tmp/aptarchive/pool/main/g/gtk%2b2.0/libgtk2.0-0-udeb_2.24.20-1_amd64.udeb' libgtk2.0-0-udeb_2.24.20-1_amd64.udeb 1643046 MD5Sum:25513478eb2e02e5766c0eea0b411ca9 'file:///tmp/aptarchive/pool/main/v/vte/libvte9-udeb_0.28.2-5_amd64.udeb' libvte9-udeb_1%3a0.28.2-5_amd64.udeb 216968 MD5Sum:7da7201effaf5ced19abd9d0b45aa2c6 'file:///tmp/aptarchive/pool/main/c/cdebconf-terminal/cdebconf-gtk-terminal_0.22_amd64.udeb' cdebconf-gtk-terminal_0.22_amd64.udeb 14734 MD5Sum:f9c3a7354560cb88e0396e2b7ba54363 'file:///tmp/aptarchive/pool/main/c/cdebconf-terminal/cdebconf-newt-terminal_0.22_amd64.udeb' cdebconf-newt-terminal_0.22_amd64.udeb 4538 MD5Sum:20db6152fce5081fcbf49c7c08f21246" diff --git a/test/integration/test-uri-encode-filename-field b/test/integration/test-uri-encode-filename-field new file mode 100755 index 000000000..136cce8d2 --- /dev/null +++ b/test/integration/test-uri-encode-filename-field @@ -0,0 +1,42 @@ +#!/bin/sh +set -e + +TESTDIR="$(readlink -f "$(dirname "$0")")" +. "$TESTDIR/framework" +setupenvironment +configarchitecture 'amd64' + +buildsimplenativepackage 'foo' 'all' '0+0~0' +setupaptarchive --no-update + +runtest() { + rm -rf rootdir/var/lib/apt/lists + testsuccess apt update + + cd downloaded + testsuccess apt download foo + testsuccess rm 'foo_0+0~0_all.deb' + testsuccess apt install foo + + mv '../aptarchive/pool/foo_0+0~0_all.deb' '../aptarchive/pool/foo_0%3a0+0~0_all.deb' + testsuccess apt purge foo -y + testfailure apt download foo + testfailure apt install foo + + sed -i -e 's#_0+0~0_#_0%3a0+0~0_#' ../rootdir/var/lib/apt/lists/*Packages + testsuccess apt download foo + # FIXME: we shouldn't take filename from file:/ in 'apt download' + if [ "$1" = 'file' ]; then + testsuccess rm 'foo_0%3a0+0~0_all.deb' + else + testsuccess rm 'foo_0+0~0_all.deb' + fi + testsuccess apt install foo + + cd "$TMPWORKINGDIRECTORY" >/dev/null + mv 'aptarchive/pool/foo_0%3a0+0~0_all.deb' 'aptarchive/pool/foo_0+0~0_all.deb' +} + +runtest 'file' +changetowebserver +runtest 'http' -- cgit v1.2.3-70-g09d2 From ba18c4323ecbc66e6a1e3fedae60721f9c5701b1 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 4 Jun 2021 14:15:46 +0200 Subject: Do not use filename of local sources in 'apt download' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a source is not copying files to the destination the download code forces the copy – which in practice are local repositories accessed via file:/ – but in that process takes the filename the local repo used rather than the filename it e.g. advertised via --print-uris. A local repository could hence override a file in the current directory if you use 'apt download', which is a rather weak ability, but still. --- apt-private/private-download.cc | 16 ++++++++++------ test/integration/framework | 3 +++ test/integration/test-uri-encode-filename-field | 7 +------ 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/apt-private/private-download.cc b/apt-private/private-download.cc index 16d11255b..eddb901d0 100644 --- a/apt-private/private-download.cc +++ b/apt-private/private-download.cc @@ -211,6 +211,7 @@ bool DoDownload(CommandLine &CmdL) I->Owner->FileSize << ' ' << I->Owner->HashSum() << std::endl; return true; } + auto const storecopy = storefile; if (_error->PendingError() == true || CheckAuth(Fetcher, false) == false) return false; @@ -220,19 +221,22 @@ bool DoDownload(CommandLine &CmdL) return false; // copy files in local sources to the current directory + i = 0; for (pkgAcquire::ItemIterator I = Fetcher.ItemsBegin(); I != Fetcher.ItemsEnd(); ++I) { - std::string const filename = cwd + flNotDir((*I)->DestFile); + if (dynamic_cast(*I) == nullptr) + continue; + if ((*I)->Local == true && - filename != (*I)->DestFile && - (*I)->Status == pkgAcquire::Item::StatDone && - dynamic_cast(*I) != nullptr) + (*I)->Status == pkgAcquire::Item::StatDone && + (*I)->DestFile != storecopy[i]) { std::ifstream src((*I)->DestFile.c_str(), std::ios::binary); - std::ofstream dst(filename.c_str(), std::ios::binary); + std::ofstream dst(storecopy[i].c_str(), std::ios::binary); dst << src.rdbuf(); - chmod(filename.c_str(), 0644); + chmod(storecopy[i].c_str(), 0644); } + ++i; } return Failed == false; } diff --git a/test/integration/framework b/test/integration/framework index 412a96577..f14b4da64 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1776,6 +1776,9 @@ msgfailoutput() { shift done echo '#### cmp output ####' + elif [ "$1" = 'rm' ]; then + echo "#### Directory listing of: $(pwd) ####" + ls -l fi catfile "$OUTPUT" msgfail "$MSG" diff --git a/test/integration/test-uri-encode-filename-field b/test/integration/test-uri-encode-filename-field index 136cce8d2..dffee21aa 100755 --- a/test/integration/test-uri-encode-filename-field +++ b/test/integration/test-uri-encode-filename-field @@ -25,12 +25,7 @@ runtest() { sed -i -e 's#_0+0~0_#_0%3a0+0~0_#' ../rootdir/var/lib/apt/lists/*Packages testsuccess apt download foo - # FIXME: we shouldn't take filename from file:/ in 'apt download' - if [ "$1" = 'file' ]; then - testsuccess rm 'foo_0%3a0+0~0_all.deb' - else - testsuccess rm 'foo_0+0~0_all.deb' - fi + testsuccess rm 'foo_0+0~0_all.deb' testsuccess apt install foo cd "$TMPWORKINGDIRECTORY" >/dev/null -- cgit v1.2.3-70-g09d2 From a2406cda4dd0aca523183ed6a8b651f06e0e63f9 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 4 Jun 2021 16:15:45 +0200 Subject: No URL decode and quoting support for Files in Sources The code exists since ever, but no other client supports this and the specification like debian-policy isn't asking for this either. What it does do is breaking than all others continue working through: If the filename includes in fact URI encoded bits (hopefully no quotes) which is rather unlikely, but none the less possible. --- apt-pkg/deb/debsrcrecords.cc | 30 ++++++++++++------------- test/integration/test-uri-encode-filename-field | 10 +++++++++ 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/apt-pkg/deb/debsrcrecords.cc b/apt-pkg/deb/debsrcrecords.cc index 89f3f1667..1fd0fdc12 100644 --- a/apt-pkg/deb/debsrcrecords.cc +++ b/apt-pkg/deb/debsrcrecords.cc @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -170,6 +171,7 @@ bool debSrcRecordParser::Files(std::vector &List) std::vector const compExts = APT::Configuration::getCompressorExtensions(); + auto const &posix = std::locale::classic(); for (char const * const * type = HashString::SupportedHashes(); *type != NULL; ++type) { // derive field from checksum type @@ -182,27 +184,23 @@ bool debSrcRecordParser::Files(std::vector &List) string const Files = Sect.FindS(checksumField.c_str()); if (Files.empty() == true) continue; + std::istringstream ss(Files); + ss.imbue(posix); - // Iterate over the entire list grabbing each triplet - const char *C = Files.c_str(); - while (*C != 0) + while (ss.good()) { - string hash, size, path; - - // Parse each of the elements - if (ParseQuoteWord(C, hash) == false || - ParseQuoteWord(C, size) == false || - ParseQuoteWord(C, path) == false) - return _error->Error("Error parsing file record in %s of source package %s", checksumField.c_str(), Package().c_str()); - + std::string hash, path; + unsigned long long size; if (iIndex == nullptr && checksumField == "Files") { - // the Files field has a different format than the rest in deb-changes files std::string ignore; - if (ParseQuoteWord(C, ignore) == false || - ParseQuoteWord(C, path) == false) - return _error->Error("Error parsing file record in %s of source package %s", checksumField.c_str(), Package().c_str()); + ss >> hash >> size >> ignore >> ignore >> path; } + else + ss >> hash >> size >> path; + + if (ss.fail() || hash.empty() || path.empty()) + return _error->Error("Error parsing file record in %s of source package %s", checksumField.c_str(), Package().c_str()); HashString const hashString(*type, hash); if (Base.empty() == false) @@ -226,7 +224,7 @@ bool debSrcRecordParser::Files(std::vector &List) // we haven't seen this file yet pkgSrcRecords::File F; F.Path = path; - F.FileSize = strtoull(size.c_str(), NULL, 10); + F.FileSize = size; F.Hashes.push_back(hashString); F.Hashes.FileSize(F.FileSize); diff --git a/test/integration/test-uri-encode-filename-field b/test/integration/test-uri-encode-filename-field index dffee21aa..233332d0a 100755 --- a/test/integration/test-uri-encode-filename-field +++ b/test/integration/test-uri-encode-filename-field @@ -17,19 +17,29 @@ runtest() { testsuccess apt download foo testsuccess rm 'foo_0+0~0_all.deb' testsuccess apt install foo + testsuccess apt source foo + testsuccess rm 'foo_0+0~0.dsc' 'foo_0+0~0.tar.xz' + testsuccess rm -r 'foo-0+0~0' mv '../aptarchive/pool/foo_0+0~0_all.deb' '../aptarchive/pool/foo_0%3a0+0~0_all.deb' + mv '../aptarchive/pool/foo_0+0~0.dsc' '../aptarchive/pool/foo_0%3a0+0~0.dsc' testsuccess apt purge foo -y testfailure apt download foo testfailure apt install foo + testfailure apt source foo --dsc-only sed -i -e 's#_0+0~0_#_0%3a0+0~0_#' ../rootdir/var/lib/apt/lists/*Packages + sed -i -e 's#_0+0~0.d#_0%3a0+0~0.d#' ../rootdir/var/lib/apt/lists/*Sources testsuccess apt download foo testsuccess rm 'foo_0+0~0_all.deb' testsuccess apt install foo + testsuccess apt source foo + testsuccess rm 'foo_0%3a0+0~0.dsc' 'foo_0+0~0.tar.xz' + testsuccess rm -r 'foo-0+0~0' cd "$TMPWORKINGDIRECTORY" >/dev/null mv 'aptarchive/pool/foo_0%3a0+0~0_all.deb' 'aptarchive/pool/foo_0+0~0_all.deb' + mv 'aptarchive/pool/foo_0%3a0+0~0.dsc' 'aptarchive/pool/foo_0+0~0.dsc' } runtest 'file' -- cgit v1.2.3-70-g09d2