Browse Source

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: 6376dfb8df
tags/debian/1.8.0_rc1
David Kalnischkies 2 years ago
parent
commit
3734cceb44
3 changed files with 118 additions and 133 deletions
  1. +99
    -106
      apt-pkg/contrib/gpgv.cc
  2. +5
    -2
      test/integration/test-cve-2013-1051-InRelease-parsing
  3. +14
    -25
      test/libapt/openmaybeclearsignedfile_test.cc

+ 99
- 106
apt-pkg/contrib/gpgv.cc View File

@@ -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;
}
/*}}}*/


+ 5
- 2
test/integration/test-cve-2013-1051-InRelease-parsing View File

@@ -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


+ 14
- 25
test/libapt/openmaybeclearsignedfile_test.cc View File

@@ -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)


Loading…
Cancel
Save