From 3734cceb44b02ca4d5ee3c6f5cbfe1e12f17cffb Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 23 Jan 2019 17:47:49 +0100 Subject: Fail instead of warn for unsigned lines in InRelease The warnings were introduced 2 years ago without any reports from the wild about them actually appearing for anyone, so now seems to be an as good time as any to switch them to errors. This allows rewritting the code by failing earlier instead of trying to keep going which makes the diff a bit hard to follow but should help simplifying reasoning about it. References: 6376dfb8dfb99b9d182c2fb13aa34b2ac89805e3 --- .../test-cve-2013-1051-InRelease-parsing | 7 ++-- test/libapt/openmaybeclearsignedfile_test.cc | 39 ++++++++-------------- 2 files changed, 19 insertions(+), 27 deletions(-) (limited to 'test') diff --git a/test/integration/test-cve-2013-1051-InRelease-parsing b/test/integration/test-cve-2013-1051-InRelease-parsing index 6238057c3..1f0cbda04 100755 --- a/test/integration/test-cve-2013-1051-InRelease-parsing +++ b/test/integration/test-cve-2013-1051-InRelease-parsing @@ -46,9 +46,12 @@ touch -d '+1hour' aptarchive/dists/stable/InRelease listcurrentlistsdirectory | sed '/_InRelease/ d' > listsdir.lst msgtest 'apt-get update should ignore unsigned data in the' 'InRelease' testwarningequal "Get:1 http://localhost:${APTHTTPPORT} stable InRelease [$(stat -c%s aptarchive/dists/stable/InRelease) B] +Err:1 http://localhost:${APTHTTPPORT} stable InRelease + Splitting up ${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial/localhost:${APTHTTPPORT}_dists_stable_InRelease into data and signature failed Reading package lists... -W: Clearsigned file '${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial/localhost:${APTHTTPPORT}_dists_stable_InRelease' contains unsigned lines. -W: Clearsigned file '${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/localhost:${APTHTTPPORT}_dists_stable_InRelease' contains unsigned lines." --nomsg aptget update +W: An error occurred during the signature verification. The repository is not updated and the previous index files will be used. GPG error: http://localhost:${APTHTTPPORT} stable InRelease: Splitting up ${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial/localhost:${APTHTTPPORT}_dists_stable_InRelease into data and signature failed +W: Failed to fetch http://localhost:${APTHTTPPORT}/dists/stable/InRelease Splitting up ${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial/localhost:${APTHTTPPORT}_dists_stable_InRelease into data and signature failed +W: Some index files failed to download. They have been ignored, or old ones used instead." --nomsg aptget update testfileequal './listsdir.lst' "$(listcurrentlistsdirectory | sed '/_InRelease/ d')" # ensure there is no package diff --git a/test/libapt/openmaybeclearsignedfile_test.cc b/test/libapt/openmaybeclearsignedfile_test.cc index 1f63fb8fc..4c6a0090f 100644 --- a/test/libapt/openmaybeclearsignedfile_test.cc +++ b/test/libapt/openmaybeclearsignedfile_test.cc @@ -190,19 +190,16 @@ TEST(OpenMaybeClearSignedFileTest,TwoSimpleSignedFile) "-----END PGP SIGNATURE-----"); EXPECT_TRUE(_error->empty()); EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); - EXPECT_TRUE(OpenMaybeClearSignedFile(tempfile, fd)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); if (tempfile.empty() == false) unlink(tempfile.c_str()); EXPECT_FALSE(_error->empty()); - EXPECT_TRUE(fd.IsOpen()); - char buffer[100]; - EXPECT_TRUE(fd.ReadLine(buffer, sizeof(buffer))); - EXPECT_STREQ(buffer, "Test"); - EXPECT_TRUE(fd.Eof()); - ASSERT_FALSE(_error->empty()); + EXPECT_FALSE(fd.IsOpen()); + // technically they are signed, but we just want one message + EXPECT_TRUE(_error->PendingError()); std::string msg; - _error->PopMessage(msg); + EXPECT_TRUE(_error->PopMessage(msg)); EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unsigned lines.", msg); } @@ -244,19 +241,15 @@ TEST(OpenMaybeClearSignedFileTest,GarbageTop) "-----END PGP SIGNATURE-----\n"); EXPECT_FALSE(StartsWithGPGClearTextSignature(tempfile)); EXPECT_TRUE(_error->empty()); - EXPECT_TRUE(OpenMaybeClearSignedFile(tempfile, fd)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); if (tempfile.empty() == false) unlink(tempfile.c_str()); - EXPECT_TRUE(fd.IsOpen()); - char buffer[100]; - EXPECT_TRUE(fd.ReadLine(buffer, sizeof(buffer))); - EXPECT_STREQ(buffer, "Test"); - EXPECT_TRUE(fd.Eof()); + EXPECT_FALSE(fd.IsOpen()); ASSERT_FALSE(_error->empty()); - ASSERT_FALSE(_error->PendingError()); + ASSERT_TRUE(_error->PendingError()); std::string msg; - _error->PopMessage(msg); + EXPECT_TRUE(_error->PopMessage(msg)); EXPECT_EQ("Clearsigned file '" + tempfile + "' does not start with a signed message block.", msg); } @@ -313,19 +306,15 @@ TEST(OpenMaybeClearSignedFileTest,GarbageBottom) "Garbage"); EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); EXPECT_TRUE(_error->empty()); - EXPECT_TRUE(OpenMaybeClearSignedFile(tempfile, fd)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); if (tempfile.empty() == false) unlink(tempfile.c_str()); - EXPECT_TRUE(fd.IsOpen()); - char buffer[100]; - EXPECT_TRUE(fd.ReadLine(buffer, sizeof(buffer))); - EXPECT_STREQ(buffer, "Test"); - EXPECT_TRUE(fd.Eof()); + EXPECT_FALSE(fd.IsOpen()); ASSERT_FALSE(_error->empty()); - ASSERT_FALSE(_error->PendingError()); + ASSERT_TRUE(_error->PendingError()); std::string msg; - _error->PopMessage(msg); + EXPECT_TRUE(_error->PopMessage(msg)); EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unsigned lines.", msg); } @@ -347,7 +336,7 @@ TEST(OpenMaybeClearSignedFileTest,BogusNoSig) std::string msg; _error->PopMessage(msg); - EXPECT_EQ("Splitting of file " + tempfile + " failed as it doesn't contain all expected parts 0 1 0", msg); + EXPECT_EQ("Splitting of clearsigned file " + tempfile + " failed as it doesn't contain all expected parts", msg); } TEST(OpenMaybeClearSignedFileTest,BogusSigStart) -- cgit v1.2.3 From e2965b0b6bdd68ffcad0e06d11755412a7e16e50 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 23 Jan 2019 20:50:29 +0100 Subject: Fail on non-signature lines in Release.gpg The exploit for CVE-2019-3462 uses the fact that a Release.gpg file can contain additional content beside the expected detached signature(s). We were passing the file unchecked to gpgv which ignores these extras without complains, so we reuse the same line-reading implementation we use for InRelease splitting to detect if a Release.gpg file contains unexpected data and fail in this case given that we in the previous commit we established that we fail in the similar InRelease case now. --- apt-pkg/contrib/gpgv.cc | 84 ++++++++++++++++------ .../test-cve-2019-3462-Release.gpg-payload | 43 +++++++++++ test/integration/test-method-gpgv | 48 ++++++++----- 3 files changed, 139 insertions(+), 36 deletions(-) create mode 100755 test/integration/test-cve-2019-3462-Release.gpg-payload (limited to 'test') diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index f6594e62e..be71b1eed 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -27,6 +27,28 @@ #include /*}}}*/ + +static bool GetLineErrno(std::unique_ptr &buffer, size_t *n, FILE *stream, std::string const &InFile, bool acceptEoF = false)/*{{{*/ +{ + errno = 0; + auto lineptr = buffer.release(); + auto const result = getline(&lineptr, n, stream); + buffer.reset(lineptr); + if (errno != 0) + return _error->Errno("getline", "Could not read from %s", InFile.c_str()); + if (result == -1) + { + if (acceptEoF) + return false; + return _error->Error("Splitting of clearsigned file %s failed as it doesn't contain all expected parts", InFile.c_str()); + } + // We remove all whitespaces including newline here as + // a) gpgv ignores them for signature + // b) we can write out a \n in code later instead of dealing with \r\n or not + _strrstrip(buffer.get()); + return true; +} + /*}}}*/ static char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/ { std::string out; @@ -176,6 +198,48 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, if (releaseSignature == DETACHED) { + std::unique_ptr detached{fopen(FileGPG.c_str(), "r"), &fclose}; + if (detached.get() == nullptr) + { + apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' could not be opened", FileGPG.c_str()); + local_exit(EINTERNAL); + } + std::unique_ptr buf{nullptr, &free}; + size_t buf_size = 0; + bool open_signature = false; + bool found_badcontent = false; + size_t found_signatures = 0; + while (GetLineErrno(buf, &buf_size, detached.get(), FileGPG, true)) + { + if (open_signature && strcmp(buf.get(), "-----END PGP SIGNATURE-----") == 0) + open_signature = false; + else if (open_signature == false && strcmp(buf.get(), "-----BEGIN PGP SIGNATURE-----") == 0) + { + open_signature = true; + ++found_signatures; + } + else if (open_signature == false) + found_badcontent = true; + } + if (found_signatures == 0 && statusfd != -1) + { + // This is not an attack attempt but a file even gpgv would complain about + // likely the result of a paywall which is covered by the gpgv method + auto const errtag = "[GNUPG:] NODATA\n"; + FileFd::Write(fd[1], errtag, strlen(errtag)); + local_exit(113); + } + else if (found_badcontent) + { + apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' contains lines not belonging to a signature", FileGPG.c_str()); + local_exit(112); + } + if (open_signature == true) + { + apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' contains unclosed signatures", FileGPG.c_str()); + local_exit(112); + } + Args.push_back(FileGPG.c_str()); Args.push_back(File.c_str()); } @@ -290,26 +354,6 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } /*}}}*/ // SplitClearSignedFile - split message into data/signature /*{{{*/ -static bool GetLineErrno(std::unique_ptr &buffer, size_t *n, FILE *stream, std::string const &InFile, bool acceptEoF = false) -{ - errno = 0; - auto lineptr = buffer.release(); - auto const result = getline(&lineptr, n, stream); - buffer.reset(lineptr); - if (errno != 0) - return _error->Errno("getline", "Could not read from %s", InFile.c_str()); - if (result == -1) - { - if (acceptEoF) - return false; - return _error->Error("Splitting of clearsigned file %s failed as it doesn't contain all expected parts", InFile.c_str()); - } - // We remove all whitespaces including newline here as - // a) gpgv ignores them for signature - // b) we can write out a \n in code later instead of dealing with \r\n or not - _strrstrip(buffer.get()); - return true; -} bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, std::vector * const ContentHeader, FileFd * const SignatureFile) { diff --git a/test/integration/test-cve-2019-3462-Release.gpg-payload b/test/integration/test-cve-2019-3462-Release.gpg-payload new file mode 100755 index 000000000..fd0f96713 --- /dev/null +++ b/test/integration/test-cve-2019-3462-Release.gpg-payload @@ -0,0 +1,43 @@ +#!/bin/sh +set -e + +# This is not covered by the CVE and harmless by itself, but used in +# the exploit and while harmless it is also pointless to allow it + +TESTDIR="$(readlink -f "$(dirname "$0")")" +. "$TESTDIR/framework" + +setupenvironment +configarchitecture 'amd64' + +export APT_DONT_SIGN='InRelease' + +insertpackage 'unstable' 'foo' 'all' '1' +setupaptarchive +rm -rf rootdir/var/lib/apt/lists + +verify() { + testfailure apt update + testsuccess grep '^ Detached signature file' rootdir/tmp/testfailure.output + testfailure apt show foo +} + +msgmsg 'Payload after detached signature' +find aptarchive -name 'Release.gpg' | while read FILE; do + cp -a "$FILE" "${FILE}.bak" + echo "evil payload" >> "$FILE" +done +verify + +msgmsg 'Payload in-between detached signatures' +find aptarchive -name 'Release.gpg' | while read FILE; do + cat "${FILE}.bak" >> "$FILE" +done +verify + +msgmsg 'Payload before detached signature' +find aptarchive -name 'Release.gpg' | while read FILE; do + echo "evil payload" > "$FILE" + cat "${FILE}.bak" >> "$FILE" +done +verify diff --git a/test/integration/test-method-gpgv b/test/integration/test-method-gpgv index 70521881d..bfa5af4c2 100755 --- a/test/integration/test-method-gpgv +++ b/test/integration/test-method-gpgv @@ -71,44 +71,60 @@ testrun() { [GNUPG:] VALIDSIG 891CC50E605796A0C6E733F74BC0A39C27CE74F9 2016-09-01 1472742629 0 4 0 1 11 00 891CC50E605796A0C6E733F74BC0A39C27CE74F9' } +echo 'Test' > message.data +cat >message.sig < [GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2018-08-16 1534459673 0 4 0 1 11 00 4281DEDBD466EAE8C1F4157E5B6896415D44C43E' -- cgit v1.2.3 From 73e3459689c05cd62f15c29d2faddb0fc215ef5e Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 23 Jan 2019 22:50:45 +0100 Subject: Merge and reuse tmp file handling across the board Having many rather similar implementations especially if one is exported while others aren't (and the rest of it not factored out at all) seems suboptimal. --- apt-pkg/contrib/fileutl.cc | 6 ++- apt-pkg/contrib/gpgv.cc | 78 ++++++++++-------------------- apt-pkg/deb/debindexfile.cc | 11 ++--- cmdline/apt-extracttemplates.cc | 34 +++++-------- test/integration/test-apt-extracttemplates | 7 +++ 5 files changed, 51 insertions(+), 85 deletions(-) (limited to 'test') diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index eab05de4f..52be3a6a6 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -3114,7 +3114,7 @@ FileFd* GetTempFile(std::string const &Prefix, bool ImmediateUnlink, FileFd * co snprintf(fn, sizeof(fn), "%s/%s.XXXXXX", tempdir.c_str(), Prefix.c_str()); int const fd = mkstemp(fn); - if(ImmediateUnlink) + if (ImmediateUnlink) unlink(fn); if (fd < 0) { @@ -3123,13 +3123,15 @@ FileFd* GetTempFile(std::string const &Prefix, bool ImmediateUnlink, FileFd * co delete Fd; return nullptr; } - if (!Fd->OpenDescriptor(fd, FileFd::ReadWrite, FileFd::None, true)) + if (!Fd->OpenDescriptor(fd, FileFd::ReadWrite | FileFd::BufferedWrite, FileFd::None, true)) { _error->Errno("GetTempFile",_("Unable to write to %s"),fn); if (TmpFd == nullptr) delete Fd; return nullptr; } + if (ImmediateUnlink == false) + Fd->SetFileName(fn); return Fd; } /*}}}*/ diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index be71b1eed..054b815fb 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -49,14 +49,6 @@ static bool GetLineErrno(std::unique_ptr &buffer, size_t return true; } /*}}}*/ -static char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/ -{ - std::string out; - std::string tmpdir = GetTempDir(); - strprintf(out, "%s/%s.XXXXXX", tmpdir.c_str(), basename); - return strdup(out.c_str()); -} - /*}}}*/ // ExecGPGV - returns the command needed for verify /*{{{*/ // --------------------------------------------------------------------- /* Generating the commandline for calling gpg is somehow complicated as @@ -171,29 +163,33 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } enum { DETACHED, CLEARSIGNED } releaseSignature = (FileGPG != File) ? DETACHED : CLEARSIGNED; - char * sig = NULL; - char * data = NULL; - char * conf = nullptr; + std::unique_ptr sig{nullptr, &free}; + std::unique_ptr data{nullptr, &free}; + std::unique_ptr conf{nullptr, &free}; // Dump the configuration so apt-key picks up the correct Dir values { - conf = GenerateTemporaryFileTemplate("apt.conf"); + { + std::string tmpfile; + strprintf(tmpfile, "%s/apt.conf.XXXXXX", GetTempDir().c_str()); + conf.reset(strdup(tmpfile.c_str())); + } if (conf == nullptr) { apt_error(std::cerr, statusfd, fd, "Couldn't create tempfile names for passing config to apt-key"); local_exit(EINTERNAL); } - int confFd = mkstemp(conf); + int confFd = mkstemp(conf.get()); if (confFd == -1) { - apt_error(std::cerr, statusfd, fd, "Couldn't create temporary file %s for passing config to apt-key", conf); + apt_error(std::cerr, statusfd, fd, "Couldn't create temporary file %s for passing config to apt-key", conf.get()); local_exit(EINTERNAL); } - local_exit.files.push_back(conf); + local_exit.files.push_back(conf.get()); - std::ofstream confStream(conf); + std::ofstream confStream(conf.get()); close(confFd); _config->Dump(confStream); confStream.close(); - setenv("APT_CONFIG", conf, 1); + setenv("APT_CONFIG", conf.get(), 1); } if (releaseSignature == DETACHED) @@ -245,30 +241,16 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } else // clear-signed file { - sig = GenerateTemporaryFileTemplate("apt.sig"); - data = GenerateTemporaryFileTemplate("apt.data"); - if (sig == NULL || data == NULL) - { - apt_error(std::cerr, statusfd, fd, "Couldn't create tempfile names for splitting up %s", File.c_str()); - local_exit(EINTERNAL); - } - - int const sigFd = mkstemp(sig); - int const dataFd = mkstemp(data); - if (dataFd != -1) - local_exit.files.push_back(data); - if (sigFd != -1) - local_exit.files.push_back(sig); - if (sigFd == -1 || dataFd == -1) - { - apt_error(std::cerr, statusfd, fd, "Couldn't create tempfiles for splitting up %s", File.c_str()); - local_exit(EINTERNAL); - } - FileFd signature; - signature.OpenDescriptor(sigFd, FileFd::WriteOnly, true); + if (GetTempFile("apt.sig", false, &signature) == nullptr) + local_exit(EINTERNAL); + sig.reset(strdup(signature.Name().c_str())); + local_exit.files.push_back(sig.get()); FileFd message; - message.OpenDescriptor(dataFd, FileFd::WriteOnly, true); + if (GetTempFile("apt.data", false, &message) == nullptr) + local_exit(EINTERNAL); + data.reset(strdup(message.Name().c_str())); + local_exit.files.push_back(data.get()); if (signature.Failed() == true || message.Failed() == true || SplitClearSignedFile(File, &message, nullptr, &signature) == false) @@ -276,8 +258,8 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, apt_error(std::cerr, statusfd, fd, "Splitting up %s into data and signature failed", File.c_str()); local_exit(112); } - Args.push_back(sig); - Args.push_back(data); + Args.push_back(sig.get()); + Args.push_back(data.get()); } Args.push_back(NULL); @@ -464,18 +446,8 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, /*}}}*/ bool OpenMaybeClearSignedFile(std::string const &ClearSignedFileName, FileFd &MessageFile) /*{{{*/ { - char * const message = GenerateTemporaryFileTemplate("fileutl.message"); - int const messageFd = mkstemp(message); - if (messageFd == -1) - { - free(message); - return _error->Errno("mkstemp", "Couldn't create temporary file to work with %s", ClearSignedFileName.c_str()); - } - // we have the fd, that's enough for us - unlink(message); - free(message); - - MessageFile.OpenDescriptor(messageFd, FileFd::ReadWrite | FileFd::BufferedWrite, true); + if (GetTempFile("clearsigned.message", true, &MessageFile) == nullptr) + return false; if (MessageFile.Failed() == true) return _error->Error("Couldn't open temporary file to work with %s", ClearSignedFileName.c_str()); diff --git a/apt-pkg/deb/debindexfile.cc b/apt-pkg/deb/debindexfile.cc index f7e3c7a5c..25e0a3312 100644 --- a/apt-pkg/deb/debindexfile.cc +++ b/apt-pkg/deb/debindexfile.cc @@ -315,13 +315,10 @@ const pkgIndexFile::Type *debStringPackageIndex::GetType() const debStringPackageIndex::debStringPackageIndex(std::string const &content) : pkgDebianIndexRealFile("", false), d(NULL) { - char fn[1024]; - std::string const tempdir = GetTempDir(); - snprintf(fn, sizeof(fn), "%s/%s.XXXXXX", tempdir.c_str(), "apt-tmp-index"); - int const fd = mkstemp(fn); - File = fn; - FileFd::Write(fd, content.data(), content.length()); - close(fd); + FileFd fd; + GetTempFile("apt-tmp-index", false, &fd); + fd.Write(content.data(), content.length()); + File = fd.Name(); } debStringPackageIndex::~debStringPackageIndex() { diff --git a/cmdline/apt-extracttemplates.cc b/cmdline/apt-extracttemplates.cc index bd23453f3..bc8a27dbe 100644 --- a/cmdline/apt-extracttemplates.cc +++ b/cmdline/apt-extracttemplates.cc @@ -229,29 +229,13 @@ static bool ShowHelp(CommandLine &) /*{{{*/ /* */ static string WriteFile(const char *package, const char *prefix, const char *data) { - char fn[512]; - - std::string tempdir = GetTempDir(); - snprintf(fn, sizeof(fn), "%s/%s.%s.XXXXXX", - _config->Find("APT::ExtractTemplates::TempDir", - tempdir.c_str()).c_str(), - package, prefix); - FileFd f; - if (data == NULL) - data = ""; - int fd = mkstemp(fn); - if (fd < 0) { - _error->Errno("ofstream::ofstream",_("Unable to mkstemp %s"),fn); - return string(); - } - if (!f.OpenDescriptor(fd, FileFd::WriteOnly, FileFd::None, true)) - { - _error->Errno("ofstream::ofstream",_("Unable to write to %s"),fn); - return string(); - } - f.Write(data, strlen(data)); - f.Close(); - return fn; + FileFd f; + std::string tplname; + strprintf(tplname, "%s.%s", package, prefix); + GetTempFile(tplname, false, &f); + if (data != nullptr) + f.Write(data, strlen(data)); + return f.Name(); } /*}}}*/ // WriteConfig - write out the config data from a debian package file /*{{{*/ @@ -289,6 +273,10 @@ static bool Go(CommandLine &CmdL) if (debconfver.empty() == true) return _error->Error( _("Cannot get debconf version. Is debconf installed?")); + auto const tmpdir = _config->Find("APT::ExtractTemplates::TempDir"); + if (tmpdir.empty() == false) + setenv("TMPDIR", tmpdir.c_str(), 1); + // Process each package passsed in for (unsigned int I = 0; I != CmdL.FileSize(); I++) { diff --git a/test/integration/test-apt-extracttemplates b/test/integration/test-apt-extracttemplates index 9b07ef79f..a47257cfd 100755 --- a/test/integration/test-apt-extracttemplates +++ b/test/integration/test-apt-extracttemplates @@ -44,6 +44,13 @@ Description: Some bar var testfileequal "$TEMPLATE" "$TEMPLATE_STR" CONFIG=$(cut -f4 -d' ' $OUT) testfileequal "$CONFIG" "$CONFIG_STR" + msgtest 'No extra files or directories in extraction directory' + if [ "$(find ./extracttemplates-out | wc -l)" = '3' ]; then + msgpass + else + msgfail + ls -l ./extracttemplates-out + fi # ensure that the format of the output string has the right number of dots for s in "$CONFIG" "$TEMPLATE"; do -- cgit v1.2.3 From 9b840b59cc80a072e14b8adc9d76669a7a50ab87 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 28 Jan 2019 20:45:02 +0100 Subject: Refuse files with lines unexpectedly starting with a dash We support dash-encoding even if we don't really work with files who would need it as implementations are free to encode every line, but otherwise a line starting with a dash must either be a header we parse explicitly or the file is refused. This is against the RFC which says clients should warn on such files, but given that we aren't expecting any files with dash-started lines to begin with this looks a lot like a we should not continue to touch the file as it smells like an attempt to confuse different parsers by "hiding" headers in-between others. The other slightly more reasonable explanation would be an armor header key starting with a dash, but no existing key does that and it seems unlikely that this could ever happen. Also, it is recommended that clients warn about unknown keys, so new appearance is limited. --- apt-pkg/contrib/gpgv.cc | 46 ++++++++-- test/libapt/openmaybeclearsignedfile_test.cc | 125 ++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 10 deletions(-) (limited to 'test') diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 6e4e9b3df..0b595fc4c 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -297,6 +297,14 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, if (found_signatures != 0) break; } + else if (buf.starts_with("-")) + { + // the used Radix-64 is not using dash for any value, so a valid line can't + // start with one. Header keys could, but no existent one does and seems unlikely. + // Instead it smells a lot like a header the parser didn't recognize. + apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' contains unexpected line starting with a dash", FileGPG.c_str()); + local_exit(112); + } } if (found_signatures == 0 && statusfd != -1) { @@ -452,6 +460,10 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, return false; if (buf.empty()) break; // empty line ends the Armor Headers + if (buf.starts_with("-")) + // § 6.2 says unknown keys should be reported to the user. We don't go that far, + // but we assume that there will never be a header key starting with a dash + return _error->Error("Clearsigned file '%s' contains unexpected line starting with a dash (%s)", InFile.c_str(), "armor"); if (ContentHeader != nullptr && buf.starts_with("Hash: ")) ContentHeader->push_back(buf.str()); } @@ -463,17 +475,28 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, if (buf.readFrom(in.get(), InFile) == false) return false; - if (buf == "-----BEGIN PGP SIGNATURE-----") + if (buf.starts_with("-")) { - if (buf.writeTo(SignatureFile) == false) - return false; - break; + if (buf == "-----BEGIN PGP SIGNATURE-----") + { + if (buf.writeTo(SignatureFile) == false) + return false; + break; + } + else if (buf.starts_with("- ")) + { + // we don't have any fields which need to be dash-escaped, + // but implementations are free to escape all lines … + if (buf.writeTo(ContentFile, first_line == false, false, 2) == false) + return false; + } + else + // § 7.1 says a client should warn, but we don't really work with files which + // should contain lines starting with a dash, so it is a lot more likely that + // this is an attempt to trick our parser vs. gpgv parser into ignoring a header + return _error->Error("Clearsigned file '%s' contains unexpected line starting with a dash (%s)", InFile.c_str(), "msg"); } - - // we don't have any fields which need to be dash-escaped, - // but implementations are free to escape all lines … - auto offset = buf.starts_with("- ") ? 2 : 0; - if (buf.writeTo(ContentFile, first_line == false, false, offset) == false) + else if (buf.writeTo(ContentFile, first_line == false, false) == false) return false; first_line = false; } @@ -491,6 +514,11 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, open_signature = true; else if (open_signature == false) return _error->Error("Clearsigned file '%s' contains unsigned lines.", InFile.c_str()); + else if (buf.starts_with("-")) + // the used Radix-64 is not using dash for any value, so a valid line can't + // start with one. Header keys could, but no existent one does and seems unlikely. + // Instead it smells a lot like a header the parser didn't recognize. + return _error->Error("Clearsigned file '%s' contains unexpected line starting with a dash (%s)", InFile.c_str(), "sig"); if (buf.writeTo(SignatureFile) == false) return false; diff --git a/test/libapt/openmaybeclearsignedfile_test.cc b/test/libapt/openmaybeclearsignedfile_test.cc index 4c6a0090f..0a4d4438a 100644 --- a/test/libapt/openmaybeclearsignedfile_test.cc +++ b/test/libapt/openmaybeclearsignedfile_test.cc @@ -111,7 +111,6 @@ TEST(OpenMaybeClearSignedFileTest,SignedFileWithContentHeaders) EXPECT_TRUE(fd.Eof()); } -// That isn't how multiple signatures are done TEST(OpenMaybeClearSignedFileTest,SignedFileWithTwoSignatures) { std::string tempfile; @@ -360,3 +359,127 @@ TEST(OpenMaybeClearSignedFileTest,BogusSigStart) _error->PopMessage(msg); EXPECT_EQ("Signature in file " + tempfile + " wasn't closed", msg); } + +TEST(OpenMaybeClearSignedFileTest,DashedSignedFile) +{ + std::string tempfile; + FileFd fd; + createTemporaryFile("dashedsignedfile", fd, &tempfile, "-----BEGIN PGP SIGNED MESSAGE-----\n" +"Hash: SHA512\n" +"\n" +"- Test\n" +"-----BEGIN PGP SIGNATURE-----\n" +"\n" +"iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt\n" +"cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l\n" +"3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg\n" +"X/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k\n" +"V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx\n" +"pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns\n" +"JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq\n" +"=TB1F\n" +"-----END PGP SIGNATURE-----\n"); + EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); + EXPECT_TRUE(OpenMaybeClearSignedFile(tempfile, fd)); + if (tempfile.empty() == false) + unlink(tempfile.c_str()); + EXPECT_TRUE(fd.IsOpen()); + char buffer[100]; + EXPECT_TRUE(fd.ReadLine(buffer, sizeof(buffer))); + EXPECT_STREQ(buffer, "Test"); + EXPECT_TRUE(fd.Eof()); +} +TEST(OpenMaybeClearSignedFileTest,StrangeDashArmorFile) +{ + std::string tempfile; + FileFd fd; + createTemporaryFile("strangedashfile", fd, &tempfile, "-----BEGIN PGP SIGNED MESSAGE-----\n" +"Hash: SHA512\n" +"-Hash: SHA512\n" +"\n" +"Test\n" +"-----BEGIN PGP SIGNATURE-----\n" +"\n" +"iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt\n" +"cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l\n" +"3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg\n" +"X/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k\n" +"V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx\n" +"pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns\n" +"JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq\n" +"=TB1F\n" +"-----END PGP SIGNATURE-----\n"); + EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); + if (tempfile.empty() == false) + unlink(tempfile.c_str()); + EXPECT_FALSE(_error->empty()); + EXPECT_FALSE(fd.IsOpen()); + + std::string msg; + EXPECT_TRUE(_error->PendingError()); + EXPECT_TRUE(_error->PopMessage(msg)); + EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unexpected line starting with a dash (armor)", msg); +} +TEST(OpenMaybeClearSignedFileTest,StrangeDashMsgFile) +{ + std::string tempfile; + FileFd fd; + createTemporaryFile("strangedashfile", fd, &tempfile, "-----BEGIN PGP SIGNED MESSAGE-----\n" +"Hash: SHA512\n" +"\n" +"-Test\n" +"-----BEGIN PGP SIGNATURE-----\n" +"\n" +"iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt\n" +"cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l\n" +"3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg\n" +"X/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k\n" +"V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx\n" +"pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns\n" +"JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq\n" +"=TB1F\n" +"-----END PGP SIGNATURE-----\n"); + EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); + if (tempfile.empty() == false) + unlink(tempfile.c_str()); + EXPECT_FALSE(_error->empty()); + EXPECT_FALSE(fd.IsOpen()); + + std::string msg; + EXPECT_TRUE(_error->PendingError()); + EXPECT_TRUE(_error->PopMessage(msg)); + EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unexpected line starting with a dash (msg)", msg); +} +TEST(OpenMaybeClearSignedFileTest,StrangeDashSigFile) +{ + std::string tempfile; + FileFd fd; + createTemporaryFile("strangedashfile", fd, &tempfile, "-----BEGIN PGP SIGNED MESSAGE-----\n" +"Hash: SHA512\n" +"\n" +"Test\n" +"-----BEGIN PGP SIGNATURE-----\n" +"\n" +"iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt\n" +"cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l\n" +"3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg\n" +"-/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k\n" +"V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx\n" +"pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns\n" +"JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq\n" +"=TB1F\n" +"-----END PGP SIGNATURE-----\n"); + EXPECT_TRUE(StartsWithGPGClearTextSignature(tempfile)); + EXPECT_FALSE(OpenMaybeClearSignedFile(tempfile, fd)); + if (tempfile.empty() == false) + unlink(tempfile.c_str()); + EXPECT_FALSE(_error->empty()); + EXPECT_FALSE(fd.IsOpen()); + + std::string msg; + EXPECT_TRUE(_error->PendingError()); + EXPECT_TRUE(_error->PopMessage(msg)); + EXPECT_EQ("Clearsigned file '" + tempfile + "' contains unexpected line starting with a dash (sig)", msg); +} -- cgit v1.2.3