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

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)