summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Kalnischkies <david@kalnischkies.de>2019-01-23 17:47:49 +0100
committerDavid Kalnischkies <david@kalnischkies.de>2019-01-23 19:10:47 +0100
commit3734cceb44b02ca4d5ee3c6f5cbfe1e12f17cffb (patch)
treeeb7582aa93bbdd0393a5718d99576bfbf220a8bd
parent4200469bb5a14c4659285917ed30c46a0b15c286 (diff)
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
-rw-r--r--apt-pkg/contrib/gpgv.cc205
-rwxr-xr-xtest/integration/test-cve-2013-1051-InRelease-parsing7
-rw-r--r--test/libapt/openmaybeclearsignedfile_test.cc39
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 <algorithm>
#include <fstream>
#include <iostream>
+#include <memory>
#include <sstream>
#include <string>
#include <vector>
@@ -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<char, decltype(&free)> &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<std::string> * const ContentHeader, FileFd * const SignatureFile)
{
- FILE *in = fopen(InFile.c_str(), "r");
- if (in == NULL)
+ std::unique_ptr<FILE, decltype(&fclose)> 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<char, decltype(&free)> 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)