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 --- apt-pkg/contrib/gpgv.cc | 205 ++++++++++----------- .../test-cve-2013-1051-InRelease-parsing | 7 +- test/libapt/openmaybeclearsignedfile_test.cc | 39 ++-- 3 files changed, 118 insertions(+), 133 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index f8ab8d715..f6594e62e 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -289,139 +290,131 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } /*}}}*/ // SplitClearSignedFile - split message into data/signature /*{{{*/ -static int GetLineErrno(char **lineptr, size_t *n, FILE *stream, std::string const &InFile) +static bool GetLineErrno(std::unique_ptr &buffer, size_t *n, FILE *stream, std::string const &InFile, bool acceptEoF = false) { - int result; - errno = 0; - result = getline(lineptr, n, stream); + 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) { - _error->Errno("getline", "Could not read from %s", InFile.c_str()); - return -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()); } - - return result; + // 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) { - FILE *in = fopen(InFile.c_str(), "r"); - if (in == NULL) + std::unique_ptr in{fopen(InFile.c_str(), "r"), &fclose}; + if (in.get() == nullptr) return _error->Errno("fopen", "can not open %s", InFile.c_str()); - bool found_message_start = false; - bool found_message_end = false; - bool skip_until_empty_line = false; - bool found_signature = false; - bool first_line = true; - bool signed_message_not_on_first_line = false; - bool found_garbage = false; - - char *buf = NULL; + struct ScopedErrors + { + ScopedErrors() { _error->PushToStack(); } + ~ScopedErrors() { _error->MergeWithStack(); } + } scoped; + std::unique_ptr buf{nullptr, &free}; size_t buf_size = 0; - _error->PushToStack(); - while (GetLineErrno(&buf, &buf_size, in, InFile) != -1) + + // start of the message + if (GetLineErrno(buf, &buf_size, in.get(), InFile) == false) + return false; // empty or read error + if (strcmp(buf.get(), "-----BEGIN PGP SIGNED MESSAGE-----") != 0) { - _strrstrip(buf); - if (found_message_start == false) - { - if (strcmp(buf, "-----BEGIN PGP SIGNED MESSAGE-----") == 0) - { - found_message_start = true; - skip_until_empty_line = true; - } - else - signed_message_not_on_first_line = found_garbage = true; - } - else if (skip_until_empty_line == true) - { - if (strlen(buf) == 0) - skip_until_empty_line = false; - // save "Hash" Armor Headers, others aren't allowed - else if (ContentHeader != NULL && strncmp(buf, "Hash: ", strlen("Hash: ")) == 0) - ContentHeader->push_back(buf); - } - else if (found_signature == false) + // this might be an unsigned file we don't want to report errors for, + // but still finish unsuccessful none the less. + while (GetLineErrno(buf, &buf_size, in.get(), InFile, true)) + if (strcmp(buf.get(), "-----BEGIN PGP SIGNED MESSAGE-----") == 0) + return _error->Error("Clearsigned file '%s' does not start with a signed message block.", InFile.c_str()); + + return false; + } + + // save "Hash" Armor Headers + while (true) + { + if (GetLineErrno(buf, &buf_size, in.get(), InFile) == false) + return false; + if (*buf == '\0') + break; // empty line ends the Armor Headers + if (ContentHeader != NULL && strncmp(buf.get(), "Hash: ", strlen("Hash: ")) == 0) + ContentHeader->push_back(buf.get()); + } + + // the message itself + bool first_line = true; + bool good_write = true; + while (true) + { + if (good_write == false || GetLineErrno(buf, &buf_size, in.get(), InFile) == false) + return false; + + if (strcmp(buf.get(), "-----BEGIN PGP SIGNATURE-----") == 0) { - if (strcmp(buf, "-----BEGIN PGP SIGNATURE-----") == 0) - { - found_signature = true; - found_message_end = true; - if (SignatureFile != NULL) - { - SignatureFile->Write(buf, strlen(buf)); - SignatureFile->Write("\n", 1); - } - } - else if (found_message_end == false) // we are in the message block + if (SignatureFile != nullptr) { - // we don't have any fields which need dash-escaped, - // but implementations are free to encode all lines … - char const * dashfree = buf; - if (strncmp(dashfree, "- ", 2) == 0) - dashfree += 2; - if(first_line == true) // first line does not need a newline - first_line = false; - else if (ContentFile != NULL) - ContentFile->Write("\n", 1); - else - continue; - if (ContentFile != NULL) - ContentFile->Write(dashfree, strlen(dashfree)); + good_write &= SignatureFile->Write(buf.get(), strlen(buf.get())); + good_write &= SignatureFile->Write("\n", 1); } - else - found_garbage = true; + break; } - else if (found_signature == true) + + // we don't have any fields which need dash-escaped, + // but implementations are free to encode all lines … + char const *dashfree = buf.get(); + if (strncmp(dashfree, "- ", 2) == 0) + dashfree += 2; + if (first_line == true) // first line does not need a newline + first_line = false; + else if (ContentFile != nullptr) + good_write &= ContentFile->Write("\n", 1); + if (ContentFile != nullptr) + good_write &= ContentFile->Write(dashfree, strlen(dashfree)); + } + + // collect all signatures + bool open_signature = true; + while (true) + { + if (good_write == false) + return false; + if (GetLineErrno(buf, &buf_size, in.get(), InFile, true) == false) + break; + + 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; + else if (open_signature == false) + return _error->Error("Clearsigned file '%s' contains unsigned lines.", InFile.c_str()); + + if (SignatureFile != nullptr) { - if (SignatureFile != NULL) - { - SignatureFile->Write(buf, strlen(buf)); - SignatureFile->Write("\n", 1); - } - if (strcmp(buf, "-----END PGP SIGNATURE-----") == 0) - found_signature = false; // look for other signatures + good_write &= SignatureFile->Write(buf.get(), strlen(buf.get())); + good_write &= SignatureFile->Write("\n", 1); } - // all the rest is whitespace, unsigned garbage or additional message blocks we ignore - else - found_garbage = true; } - fclose(in); - if (buf != NULL) - free(buf); + if (open_signature == true) + return _error->Error("Signature in file %s wasn't closed", InFile.c_str()); - // Flush the files. Errors will be checked below. + // Flush the files if (SignatureFile != nullptr) SignatureFile->Flush(); if (ContentFile != nullptr) ContentFile->Flush(); - if (found_message_start) - { - if (signed_message_not_on_first_line) - _error->Warning("Clearsigned file '%s' does not start with a signed message block.", InFile.c_str()); - else if (found_garbage) - _error->Warning("Clearsigned file '%s' contains unsigned lines.", InFile.c_str()); - } - - // An error occurred during reading - propagate it up - bool const hasErrored = _error->PendingError(); - _error->MergeWithStack(); - if (hasErrored) - return false; - - if (found_signature == true) - return _error->Error("Signature in file %s wasn't closed", InFile.c_str()); - - // if we haven't found any of them, this an unsigned file, - // so don't generate an error, but splitting was unsuccessful none-the-less - if (first_line == true && found_message_start == false && found_message_end == false) + // Catch-all for "unhandled" read/sync errors + if (_error->PendingError()) return false; - // otherwise one missing indicates a syntax error - else if (first_line == true || found_message_start == false || found_message_end == false) - return _error->Error("Splitting of file %s failed as it doesn't contain all expected parts %i %i %i", InFile.c_str(), first_line, found_message_start, found_message_end); - return true; } /*}}}*/ 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 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(-) 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 c8350b2b0b77bacf6a1b42eade20002546baac3a Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 28 Jan 2019 18:17:00 +0100 Subject: Explicitly remove the whitespaces defined by RFC RFC 4880 section 7.1 "Dash-Escaped Text" at the end defines that only space and tab are allowed, so we should remove only these even if due to use complaining (or now failing) you can't really make use of it. Note that strrstrip was removing '\r\n\t ', not other whitespaces like \v or \f and another big reason to do it explicitly here now is to avoid that a future change adding those could have unintended consequences. --- apt-pkg/contrib/gpgv.cc | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 054b815fb..087862b70 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -28,6 +28,20 @@ #include /*}}}*/ +// a "normal" find_last_not_of returns npos if not found +static int find_last_not_of_length(char * const buffer, APT::StringView const bad) +{ + if (buffer == nullptr) + return 0; + int result = strlen(buffer) - 1; + while (result >= 0) + { + if (std::find(bad.begin(), bad.end(), buffer[result]) == bad.end()) + break; + --result; + } + return result + 1; +} static bool GetLineErrno(std::unique_ptr &buffer, size_t *n, FILE *stream, std::string const &InFile, bool acceptEoF = false)/*{{{*/ { errno = 0; @@ -42,10 +56,12 @@ static bool GetLineErrno(std::unique_ptr &buffer, size_t 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()); + // a) remove newline characters, so we can work consistently with lines + auto line_length = find_last_not_of_length(buffer.get(), "\n\r"); + buffer.get()[line_length] = '\0'; + // b) remove trailing whitespaces as defined by rfc4880 §7.1 + line_length = find_last_not_of_length(buffer.get(), " \t"); + buffer.get()[line_length] = '\0'; return true; } /*}}}*/ -- cgit v1.2.3 From 93c9a49c1fd378cd0a3b472d68afb3378da145b8 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 28 Jan 2019 18:33:31 +0100 Subject: Use more abstraction to handle the current line buffer This is C++, so we can use a bit more abstraction to let the code look a tiny bit nicer hopefully improving readability a bit. Gbp-Dch: Ignore --- apt-pkg/contrib/gpgv.cc | 216 +++++++++++++++++++++++++++++------------------- 1 file changed, 133 insertions(+), 83 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 087862b70..6e4e9b3df 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -28,41 +28,103 @@ #include /*}}}*/ -// a "normal" find_last_not_of returns npos if not found -static int find_last_not_of_length(char * const buffer, APT::StringView const bad) +// syntactic sugar to wrap a raw pointer with a custom deleter in a std::unique_ptr +static std::unique_ptr make_unique_char(void *const str = nullptr) { - if (buffer == nullptr) - return 0; - int result = strlen(buffer) - 1; - while (result >= 0) - { - if (std::find(bad.begin(), bad.end(), buffer[result]) == bad.end()) - break; - --result; - } - return result + 1; + return {static_cast(str), &free}; } -static bool GetLineErrno(std::unique_ptr &buffer, size_t *n, FILE *stream, std::string const &InFile, bool acceptEoF = false)/*{{{*/ +static std::unique_ptr make_unique_FILE(std::string const &filename, char const *const mode) +{ + return {fopen(filename.c_str(), mode), &fclose}; +} + +class LineBuffer /*{{{*/ { - 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) + char *buffer = nullptr; + size_t buffer_size = 0; + int line_length = 0; + // a "normal" find_last_not_of returns npos if not found + int find_last_not_of_length(APT::StringView const bad) const + { + if (empty()) + return 0; + int result = line_length - 1; + while (result >= 0) + { + if (std::find(bad.begin(), bad.end(), buffer[result]) == bad.end()) + break; + --result; + } + return result + 1; + } + + public: + bool empty() const noexcept { - if (acceptEoF) + return line_length <= 0; + } + APT::StringView view() const noexcept + { + return {buffer, static_cast(line_length)}; + } + std::string str() const noexcept + { + if (empty()) + return {}; + return {buffer, static_cast(line_length)}; + } + bool starts_with(APT::StringView const start) const + { + auto const line = view(); + if (line.length() < start.length()) return false; - return _error->Error("Splitting of clearsigned file %s failed as it doesn't contain all expected parts", InFile.c_str()); + return line.compare(0, start.length(), start) == 0; } - // a) remove newline characters, so we can work consistently with lines - auto line_length = find_last_not_of_length(buffer.get(), "\n\r"); - buffer.get()[line_length] = '\0'; - // b) remove trailing whitespaces as defined by rfc4880 §7.1 - line_length = find_last_not_of_length(buffer.get(), " \t"); - buffer.get()[line_length] = '\0'; - return true; + bool writeTo(FileFd *const to, bool const prefixNL = false, bool const postfixNL = true, size_t offset = 0) const + { + if (to == nullptr) + return true; + if (prefixNL) + to->Write("\n", 1); + if (postfixNL) + { + buffer[line_length] = '\n'; + bool const result = to->Write(buffer + offset, line_length + 1 - offset); + buffer[line_length] = '\0'; + return result; + } + return to->Write(buffer + offset, line_length - offset); + } + + bool readFrom(FILE *stream, std::string const &InFile, bool acceptEoF = false) + { + errno = 0; + line_length = getline(&buffer, &buffer_size, stream); + if (errno != 0) + return _error->Errno("getline", "Could not read from %s", InFile.c_str()); + if (line_length == -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()); + } + // a) remove newline characters, so we can work consistently with lines + line_length = find_last_not_of_length("\n\r"); + // b) remove trailing whitespaces as defined by rfc4880 §7.1 + line_length = find_last_not_of_length(" \t"); + buffer[line_length] = '\0'; + return true; + } + + ~LineBuffer() { free(buffer); } +}; +static bool operator==(LineBuffer const &buf, APT::StringView const exp) noexcept +{ + return buf.view() == exp; +} +static bool operator!=(LineBuffer const &buf, APT::StringView const exp) noexcept +{ + return buf.view() != exp; } /*}}}*/ // ExecGPGV - returns the command needed for verify /*{{{*/ @@ -78,11 +140,10 @@ static bool GetLineErrno(std::unique_ptr &buffer, size_t */ static bool iovprintf(std::ostream &out, const char *format, va_list &args, ssize_t &size) { - char *S = (char*)malloc(size); - ssize_t const n = vsnprintf(S, size, format, args); + auto S = make_unique_char(malloc(size)); + ssize_t const n = vsnprintf(S.get(), size, format, args); if (n > -1 && n < size) { - out << S; - free(S); + out << S.get(); return true; } else { if (n > -1) @@ -90,7 +151,6 @@ static bool iovprintf(std::ostream &out, const char *format, else size *= 2; } - free(S); return false; } static void APT_PRINTF(4) apt_error(std::ostream &outterm, int const statusfd, int fd[2], const char *format, ...) @@ -179,9 +239,9 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } enum { DETACHED, CLEARSIGNED } releaseSignature = (FileGPG != File) ? DETACHED : CLEARSIGNED; - std::unique_ptr sig{nullptr, &free}; - std::unique_ptr data{nullptr, &free}; - std::unique_ptr conf{nullptr, &free}; + auto sig = make_unique_char(); + auto data = make_unique_char(); + auto conf = make_unique_char(); // Dump the configuration so apt-key picks up the correct Dir values { @@ -210,28 +270,33 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, if (releaseSignature == DETACHED) { - std::unique_ptr detached{fopen(FileGPG.c_str(), "r"), &fclose}; + auto detached = make_unique_FILE(FileGPG, "r"); 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; + LineBuffer buf; bool open_signature = false; bool found_badcontent = false; size_t found_signatures = 0; - while (GetLineErrno(buf, &buf_size, detached.get(), FileGPG, true)) + while (buf.readFrom(detached.get(), FileGPG, true)) { - if (open_signature && strcmp(buf.get(), "-----END PGP SIGNATURE-----") == 0) + if (open_signature == true && buf == "-----END PGP SIGNATURE-----") open_signature = false; - else if (open_signature == false && strcmp(buf.get(), "-----BEGIN PGP SIGNATURE-----") == 0) + else if (open_signature == false && buf == "-----BEGIN PGP SIGNATURE-----") { open_signature = true; ++found_signatures; + if (found_badcontent) + break; } else if (open_signature == false) + { found_badcontent = true; + if (found_signatures != 0) + break; + } } if (found_signatures == 0 && statusfd != -1) { @@ -355,7 +420,7 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, std::vector * const ContentHeader, FileFd * const SignatureFile) { - std::unique_ptr in{fopen(InFile.c_str(), "r"), &fclose}; + auto in = make_unique_FILE(InFile, "r"); if (in.get() == nullptr) return _error->Errno("fopen", "can not open %s", InFile.c_str()); @@ -364,18 +429,17 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, ScopedErrors() { _error->PushToStack(); } ~ScopedErrors() { _error->MergeWithStack(); } } scoped; - std::unique_ptr buf{nullptr, &free}; - size_t buf_size = 0; + LineBuffer buf; // start of the message - if (GetLineErrno(buf, &buf_size, in.get(), InFile) == false) + if (buf.readFrom(in.get(), InFile) == false) return false; // empty or read error - if (strcmp(buf.get(), "-----BEGIN PGP SIGNED MESSAGE-----") != 0) + if (buf != "-----BEGIN PGP SIGNED MESSAGE-----") { // this might be an unsigned file we don't want to report errors for, // but still finish unsuccessful none the less. - while (GetLineErrno(buf, &buf_size, in.get(), InFile, true)) - if (strcmp(buf.get(), "-----BEGIN PGP SIGNED MESSAGE-----") == 0) + while (buf.readFrom(in.get(), InFile, true)) + if (buf == "-----BEGIN PGP SIGNED MESSAGE-----") return _error->Error("Clearsigned file '%s' does not start with a signed message block.", InFile.c_str()); return false; @@ -384,66 +448,52 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, // save "Hash" Armor Headers while (true) { - if (GetLineErrno(buf, &buf_size, in.get(), InFile) == false) + if (buf.readFrom(in.get(), InFile) == false) return false; - if (*buf == '\0') + if (buf.empty()) break; // empty line ends the Armor Headers - if (ContentHeader != NULL && strncmp(buf.get(), "Hash: ", strlen("Hash: ")) == 0) - ContentHeader->push_back(buf.get()); + if (ContentHeader != nullptr && buf.starts_with("Hash: ")) + ContentHeader->push_back(buf.str()); } // the message itself bool first_line = true; - bool good_write = true; while (true) { - if (good_write == false || GetLineErrno(buf, &buf_size, in.get(), InFile) == false) + if (buf.readFrom(in.get(), InFile) == false) return false; - if (strcmp(buf.get(), "-----BEGIN PGP SIGNATURE-----") == 0) + if (buf == "-----BEGIN PGP SIGNATURE-----") { - if (SignatureFile != nullptr) - { - good_write &= SignatureFile->Write(buf.get(), strlen(buf.get())); - good_write &= SignatureFile->Write("\n", 1); - } + if (buf.writeTo(SignatureFile) == false) + return false; break; } - // we don't have any fields which need dash-escaped, - // but implementations are free to encode all lines … - char const *dashfree = buf.get(); - if (strncmp(dashfree, "- ", 2) == 0) - dashfree += 2; - if (first_line == true) // first line does not need a newline - first_line = false; - else if (ContentFile != nullptr) - good_write &= ContentFile->Write("\n", 1); - if (ContentFile != nullptr) - good_write &= ContentFile->Write(dashfree, strlen(dashfree)); + // 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) + return false; + first_line = false; } // collect all signatures bool open_signature = true; while (true) { - if (good_write == false) - return false; - if (GetLineErrno(buf, &buf_size, in.get(), InFile, true) == false) + if (buf.readFrom(in.get(), InFile, true) == false) break; - if (open_signature && strcmp(buf.get(), "-----END PGP SIGNATURE-----") == 0) + if (open_signature == true && buf == "-----END PGP SIGNATURE-----") open_signature = false; - else if (open_signature == false && strcmp(buf.get(), "-----BEGIN PGP SIGNATURE-----") == 0) + else if (open_signature == false && buf == "-----BEGIN PGP SIGNATURE-----") open_signature = true; else if (open_signature == false) return _error->Error("Clearsigned file '%s' contains unsigned lines.", InFile.c_str()); - if (SignatureFile != nullptr) - { - good_write &= SignatureFile->Write(buf.get(), strlen(buf.get())); - good_write &= SignatureFile->Write("\n", 1); - } + if (buf.writeTo(SignatureFile) == false) + return false; } if (open_signature == true) return _error->Error("Signature in file %s wasn't closed", InFile.c_str()); -- 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(-) 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 From 7107b3a056211daf7cd00b130f42168d6aa1e1b6 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 29 Jan 2019 13:57:19 +0100 Subject: Rework ifs to use not instead of == false/true No change in the logic itself, just dropping "== true", replacing "== false" with not and moving lines around to make branches more obvious. Suggested-By: Julian Andres Klode Gbp-Dch: Ignore --- apt-pkg/contrib/gpgv.cc | 120 ++++++++++++++++++++++++++---------------------- 1 file changed, 66 insertions(+), 54 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 0b595fc4c..88cf0afc9 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -164,7 +164,7 @@ static void APT_PRINTF(4) apt_error(std::ostream &outterm, int const statusfd, i va_start(args,format); ret = iovprintf(out, format, args, size); va_end(args); - if (ret == true) + if (ret) break; } if (statusfd != -1) @@ -172,8 +172,8 @@ static void APT_PRINTF(4) apt_error(std::ostream &outterm, int const statusfd, i auto const errtag = "[APTKEY:] ERROR "; outstr << '\n'; auto const errtext = outstr.str(); - if (FileFd::Write(fd[1], errtag, strlen(errtag)) == false || - FileFd::Write(fd[1], errtext.data(), errtext.size()) == false) + if (not FileFd::Write(fd[1], errtag, strlen(errtag)) || + not FileFd::Write(fd[1], errtext.data(), errtext.size())) outterm << errtext << std::flush; } } @@ -232,7 +232,7 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, Opts = Opts->Child; for (; Opts != 0; Opts = Opts->Next) { - if (Opts->Value.empty() == true) + if (Opts->Value.empty()) continue; Args.push_back(Opts->Value.c_str()); } @@ -282,28 +282,34 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, size_t found_signatures = 0; while (buf.readFrom(detached.get(), FileGPG, true)) { - if (open_signature == true && buf == "-----END PGP SIGNATURE-----") - open_signature = false; - else if (open_signature == false && buf == "-----BEGIN PGP SIGNATURE-----") + if (open_signature) { - open_signature = true; - ++found_signatures; - if (found_badcontent) - break; + if (buf == "-----END PGP SIGNATURE-----") + open_signature = false; + 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); + } } - else if (open_signature == false) + else //if (not open_signature) { - found_badcontent = true; - 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 (buf == "-----BEGIN PGP SIGNATURE-----") + { + open_signature = true; + ++found_signatures; + if (found_badcontent) + break; + } + else + { + found_badcontent = true; + if (found_signatures != 0) + break; + } } } if (found_signatures == 0 && statusfd != -1) @@ -319,7 +325,7 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, 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) + if (open_signature) { apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' contains unclosed signatures", FileGPG.c_str()); local_exit(112); @@ -341,8 +347,8 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, 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) + if (signature.Failed() || message.Failed() || + not SplitClearSignedFile(File, &message, nullptr, &signature)) { apt_error(std::cerr, statusfd, fd, "Splitting up %s into data and signature failed", File.c_str()); local_exit(112); @@ -353,7 +359,7 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, Args.push_back(NULL); - if (Debug == true) + if (Debug) { std::clog << "Preparing to exec: "; for (std::vector::const_iterator a = Args.begin(); *a != NULL; ++a) @@ -406,7 +412,7 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } // check if it exit'ed normally … - if (WIFEXITED(Status) == false) + if (not WIFEXITED(Status)) { apt_error(std::cerr, statusfd, fd, _("Sub-process %s exited unexpectedly"), "apt-key"); local_exit(EINTERNAL); @@ -440,7 +446,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, LineBuffer buf; // start of the message - if (buf.readFrom(in.get(), InFile) == false) + if (not buf.readFrom(in.get(), InFile)) return false; // empty or read error if (buf != "-----BEGIN PGP SIGNED MESSAGE-----") { @@ -456,7 +462,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, // save "Hash" Armor Headers while (true) { - if (buf.readFrom(in.get(), InFile) == false) + if (not buf.readFrom(in.get(), InFile)) return false; if (buf.empty()) break; // empty line ends the Armor Headers @@ -472,14 +478,14 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, bool first_line = true; while (true) { - if (buf.readFrom(in.get(), InFile) == false) + if (not buf.readFrom(in.get(), InFile)) return false; if (buf.starts_with("-")) { if (buf == "-----BEGIN PGP SIGNATURE-----") { - if (buf.writeTo(SignatureFile) == false) + if (not buf.writeTo(SignatureFile)) return false; break; } @@ -487,7 +493,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, { // 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) + if (not buf.writeTo(ContentFile, not first_line, false, 2)) return false; } else @@ -496,7 +502,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, // 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"); } - else if (buf.writeTo(ContentFile, first_line == false, false) == false) + else if (not buf.writeTo(ContentFile, not first_line, false)) return false; first_line = false; } @@ -505,25 +511,31 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, bool open_signature = true; while (true) { - if (buf.readFrom(in.get(), InFile, true) == false) + if (not buf.readFrom(in.get(), InFile, true)) break; - if (open_signature == true && buf == "-----END PGP SIGNATURE-----") - open_signature = false; - else if (open_signature == false && buf == "-----BEGIN PGP SIGNATURE-----") - 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) + if (open_signature) + { + if (buf == "-----END PGP SIGNATURE-----") + open_signature = false; + 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"); + } + else //if (not open_signature) + { + if (buf == "-----BEGIN PGP SIGNATURE-----") + open_signature = true; + else + return _error->Error("Clearsigned file '%s' contains unsigned lines.", InFile.c_str()); + } + + if (not buf.writeTo(SignatureFile)) return false; } - if (open_signature == true) + if (open_signature) return _error->Error("Signature in file %s wasn't closed", InFile.c_str()); // Flush the files @@ -542,18 +554,18 @@ bool OpenMaybeClearSignedFile(std::string const &ClearSignedFileName, FileFd &Me { if (GetTempFile("clearsigned.message", true, &MessageFile) == nullptr) return false; - if (MessageFile.Failed() == true) + if (MessageFile.Failed()) return _error->Error("Couldn't open temporary file to work with %s", ClearSignedFileName.c_str()); _error->PushToStack(); bool const splitDone = SplitClearSignedFile(ClearSignedFileName, &MessageFile, NULL, NULL); bool const errorDone = _error->PendingError(); _error->MergeWithStack(); - if (splitDone == false) + if (not splitDone) { MessageFile.Close(); - if (errorDone == true) + if (errorDone) return false; // we deal with an unsigned file @@ -561,10 +573,10 @@ bool OpenMaybeClearSignedFile(std::string const &ClearSignedFileName, FileFd &Me } else // clear-signed { - if (MessageFile.Seek(0) == false) + if (not MessageFile.Seek(0)) return _error->Errno("lseek", "Unable to seek back in message for file %s", ClearSignedFileName.c_str()); } - return MessageFile.Failed() == false; + return not MessageFile.Failed(); } /*}}}*/ -- cgit v1.2.3 From cd852177246a5ea52ae3fda12b8d991a1ad8d351 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 29 Jan 2019 15:34:56 +0100 Subject: Reuse APT::StringView more in LineBuffer No effective change in behaviour, just simplifying and reusing code. Suggested-By: Julian Andres Klode Gbp-Dch: Ignore --- apt-pkg/contrib/gpgv.cc | 41 +++++++++-------------------------------- 1 file changed, 9 insertions(+), 32 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 88cf0afc9..73fddf38c 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -46,40 +46,17 @@ class LineBuffer /*{{{*/ // a "normal" find_last_not_of returns npos if not found int find_last_not_of_length(APT::StringView const bad) const { - if (empty()) - return 0; - int result = line_length - 1; - while (result >= 0) - { - if (std::find(bad.begin(), bad.end(), buffer[result]) == bad.end()) - break; - --result; - } - return result + 1; + for (int result = line_length - 1; result >= 0; --result) + if (bad.find(buffer[result]) == APT::StringView::npos) + return result + 1; + return 0; } public: - bool empty() const noexcept - { - return line_length <= 0; - } - APT::StringView view() const noexcept - { - return {buffer, static_cast(line_length)}; - } - std::string str() const noexcept - { - if (empty()) - return {}; - return {buffer, static_cast(line_length)}; - } - bool starts_with(APT::StringView const start) const - { - auto const line = view(); - if (line.length() < start.length()) - return false; - return line.compare(0, start.length(), start) == 0; - } + bool empty() const noexcept { return view().empty(); } + APT::StringView view() const noexcept { return {buffer, static_cast(line_length)}; } + bool starts_with(APT::StringView const start) const { return view().substr(0, start.size()) == start; } + bool writeTo(FileFd *const to, bool const prefixNL = false, bool const postfixNL = true, size_t offset = 0) const { if (to == nullptr) @@ -471,7 +448,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, // 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()); + ContentHeader->push_back(buf.view().to_string()); } // the message itself -- cgit v1.2.3 From fd438818d2518901396d6835f845b0b90c3a82fa Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 1 Feb 2019 14:02:08 +0100 Subject: Avoid boolean flags by splitting writeTo functions Suggested-By: Julian Andres Klode Gbp-Dch: Ignore --- apt-pkg/contrib/gpgv.cc | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 73fddf38c..b845528d8 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -57,21 +57,27 @@ class LineBuffer /*{{{*/ APT::StringView view() const noexcept { return {buffer, static_cast(line_length)}; } bool starts_with(APT::StringView const start) const { return view().substr(0, start.size()) == start; } - bool writeTo(FileFd *const to, bool const prefixNL = false, bool const postfixNL = true, size_t offset = 0) const + bool writeTo(FileFd *const to, size_t offset = 0) const { if (to == nullptr) return true; - if (prefixNL) - to->Write("\n", 1); - if (postfixNL) - { - buffer[line_length] = '\n'; - bool const result = to->Write(buffer + offset, line_length + 1 - offset); - buffer[line_length] = '\0'; - return result; - } return to->Write(buffer + offset, line_length - offset); } + bool writeLineTo(FileFd *const to) const + { + if (to == nullptr) + return true; + buffer[line_length] = '\n'; + bool const result = to->Write(buffer, line_length + 1); + buffer[line_length] = '\0'; + return result; + } + bool writeNewLineIf(FileFd *const to, bool const condition) const + { + if (not condition || to == nullptr) + return true; + return to->Write("\n", 1); + } bool readFrom(FILE *stream, std::string const &InFile, bool acceptEoF = false) { @@ -462,7 +468,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, { if (buf == "-----BEGIN PGP SIGNATURE-----") { - if (not buf.writeTo(SignatureFile)) + if (not buf.writeLineTo(SignatureFile)) return false; break; } @@ -470,7 +476,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, { // we don't have any fields which need to be dash-escaped, // but implementations are free to escape all lines … - if (not buf.writeTo(ContentFile, not first_line, false, 2)) + if (not buf.writeNewLineIf(ContentFile, not first_line) || not buf.writeTo(ContentFile, 2)) return false; } else @@ -479,7 +485,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, // 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"); } - else if (not buf.writeTo(ContentFile, not first_line, false)) + else if (not buf.writeNewLineIf(ContentFile, not first_line) || not buf.writeTo(ContentFile)) return false; first_line = false; } @@ -509,7 +515,7 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, return _error->Error("Clearsigned file '%s' contains unsigned lines.", InFile.c_str()); } - if (not buf.writeTo(SignatureFile)) + if (not buf.writeLineTo(SignatureFile)) return false; } if (open_signature) -- cgit v1.2.3 From 8aa2053368d1bb82755164eaa36a10410b434c7c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 1 Feb 2019 14:08:08 +0100 Subject: Drop buffered writing from clearsigned message extraction It is dropped in the merged code, but the extraction of the clearsigned message code was the only one who had it previously, so the short-desc explains the change from a before-after merge of the branch PoV. It would make sense to enable it, but as we aren't in a time critical paths here we can delay this for after buster to avoid problems. References: 73e3459689c05cd62f15c29d2faddb0fc215ef5e Suggested-By: Julian Andres Klode --- apt-pkg/contrib/fileutl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index 52be3a6a6..8cb4b509c 100644 --- a/apt-pkg/contrib/fileutl.cc +++ b/apt-pkg/contrib/fileutl.cc @@ -3123,7 +3123,7 @@ FileFd* GetTempFile(std::string const &Prefix, bool ImmediateUnlink, FileFd * co delete Fd; return nullptr; } - if (!Fd->OpenDescriptor(fd, FileFd::ReadWrite | FileFd::BufferedWrite, FileFd::None, true)) + if (!Fd->OpenDescriptor(fd, FileFd::ReadWrite, FileFd::None, true)) { _error->Errno("GetTempFile",_("Unable to write to %s"),fn); if (TmpFd == nullptr) -- cgit v1.2.3