diff options
author | Julian Andres Klode <jak@debian.org> | 2019-02-01 14:40:06 +0000 |
---|---|---|
committer | Julian Andres Klode <jak@debian.org> | 2019-02-01 14:40:06 +0000 |
commit | d5dcc2e9d3008b57c3fae0bcb5b1c2a197f5430c (patch) | |
tree | 18472bd719bbd40e687d58f09c578382ae6a72ac | |
parent | b358bd64fc537de4e25c25b79de87346ec51a50c (diff) | |
parent | 8aa2053368d1bb82755164eaa36a10410b434c7c (diff) |
Merge branch 'pu/refuseunsignedlines' into 'master'
Fail if InRelease or Release.gpg contain unsigned lines
See merge request apt-team/apt!45
-rw-r--r-- | apt-pkg/contrib/fileutl.cc | 4 | ||||
-rw-r--r-- | apt-pkg/contrib/gpgv.cc | 446 | ||||
-rw-r--r-- | apt-pkg/deb/debindexfile.cc | 11 | ||||
-rw-r--r-- | cmdline/apt-extracttemplates.cc | 34 | ||||
-rwxr-xr-x | test/integration/test-apt-extracttemplates | 7 | ||||
-rwxr-xr-x | test/integration/test-cve-2013-1051-InRelease-parsing | 7 | ||||
-rwxr-xr-x | test/integration/test-cve-2019-3462-Release.gpg-payload | 43 | ||||
-rwxr-xr-x | test/integration/test-method-gpgv | 48 | ||||
-rw-r--r-- | test/libapt/openmaybeclearsignedfile_test.cc | 164 |
9 files changed, 515 insertions, 249 deletions
diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc index eab05de4f..8cb4b509c 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) { @@ -3130,6 +3130,8 @@ FileFd* GetTempFile(std::string const &Prefix, bool ImmediateUnlink, FileFd * co 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 f8ab8d715..b845528d8 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -20,18 +20,94 @@ #include <algorithm> #include <fstream> #include <iostream> +#include <memory> #include <sstream> #include <string> #include <vector> #include <apti18n.h> /*}}}*/ -static char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/ + +// syntactic sugar to wrap a raw pointer with a custom deleter in a std::unique_ptr +static std::unique_ptr<char, decltype(&free)> make_unique_char(void *const str = nullptr) +{ + return {static_cast<char *>(str), &free}; +} +static std::unique_ptr<FILE, decltype(&fclose)> make_unique_FILE(std::string const &filename, char const *const mode) { - std::string out; - std::string tmpdir = GetTempDir(); - strprintf(out, "%s/%s.XXXXXX", tmpdir.c_str(), basename); - return strdup(out.c_str()); + return {fopen(filename.c_str(), mode), &fclose}; +} + +class LineBuffer /*{{{*/ +{ + 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 + { + 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 view().empty(); } + APT::StringView view() const noexcept { return {buffer, static_cast<size_t>(line_length)}; } + bool starts_with(APT::StringView const start) const { return view().substr(0, start.size()) == start; } + + bool writeTo(FileFd *const to, size_t offset = 0) const + { + if (to == nullptr) + return true; + 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) + { + 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 /*{{{*/ @@ -47,11 +123,10 @@ static char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/ */ 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) @@ -59,7 +134,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, ...) @@ -73,7 +147,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) @@ -81,8 +155,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; } } @@ -141,83 +215,134 @@ 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()); } } enum { DETACHED, CLEARSIGNED } releaseSignature = (FileGPG != File) ? DETACHED : CLEARSIGNED; - char * sig = NULL; - char * data = NULL; - char * conf = nullptr; + 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 { - 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) { - Args.push_back(FileGPG.c_str()); - Args.push_back(File.c_str()); - } - else // clear-signed file - { - sig = GenerateTemporaryFileTemplate("apt.sig"); - data = GenerateTemporaryFileTemplate("apt.data"); - if (sig == NULL || data == NULL) + auto detached = make_unique_FILE(FileGPG, "r"); + if (detached.get() == nullptr) { - apt_error(std::cerr, statusfd, fd, "Couldn't create tempfile names for splitting up %s", File.c_str()); + apt_error(std::cerr, statusfd, fd, "Detached signature file '%s' could not be opened", FileGPG.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) + LineBuffer buf; + bool open_signature = false; + bool found_badcontent = false; + size_t found_signatures = 0; + while (buf.readFrom(detached.get(), FileGPG, true)) { - apt_error(std::cerr, statusfd, fd, "Couldn't create tempfiles for splitting up %s", File.c_str()); - local_exit(EINTERNAL); + 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. + 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 (not open_signature) + { + 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) + { + // 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) + { + 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()); + } + else // clear-signed file + { 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) + 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); } - Args.push_back(sig); - Args.push_back(data); + Args.push_back(sig.get()); + Args.push_back(data.get()); } Args.push_back(NULL); - if (Debug == true) + if (Debug) { std::clog << "Preparing to exec: "; for (std::vector<const char *>::const_iterator a = Args.begin(); *a != NULL; ++a) @@ -270,7 +395,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); @@ -289,168 +414,141 @@ 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) +bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, + std::vector<std::string> * const ContentHeader, FileFd * const SignatureFile) { - int result; + auto in = make_unique_FILE(InFile, "r"); + if (in.get() == nullptr) + return _error->Errno("fopen", "can not open %s", InFile.c_str()); - errno = 0; - result = getline(lineptr, n, stream); - if (errno != 0) + struct ScopedErrors + { + ScopedErrors() { _error->PushToStack(); } + ~ScopedErrors() { _error->MergeWithStack(); } + } scoped; + LineBuffer buf; + + // start of the message + if (not buf.readFrom(in.get(), InFile)) + return false; // empty or read error + if (buf != "-----BEGIN PGP SIGNED MESSAGE-----") { - _error->Errno("getline", "Could not read from %s", InFile.c_str()); - return -1; + // this might be an unsigned file we don't want to report errors for, + // but still finish unsuccessful none the less. + 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; } - return result; -} -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) - return _error->Errno("fopen", "can not open %s", InFile.c_str()); + // save "Hash" Armor Headers + while (true) + { + if (not buf.readFrom(in.get(), InFile)) + 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.view().to_string()); + } - bool found_message_start = false; - bool found_message_end = false; - bool skip_until_empty_line = false; - bool found_signature = false; + // the message itself bool first_line = true; - bool signed_message_not_on_first_line = false; - bool found_garbage = false; - - char *buf = NULL; - size_t buf_size = 0; - _error->PushToStack(); - while (GetLineErrno(&buf, &buf_size, in, InFile) != -1) + while (true) { - _strrstrip(buf); - if (found_message_start == false) + if (not buf.readFrom(in.get(), InFile)) + return false; + + if (buf.starts_with("-")) { - if (strcmp(buf, "-----BEGIN PGP SIGNED MESSAGE-----") == 0) + if (buf == "-----BEGIN PGP SIGNATURE-----") + { + if (not buf.writeLineTo(SignatureFile)) + return false; + break; + } + else if (buf.starts_with("- ")) { - found_message_start = true; - skip_until_empty_line = true; + // we don't have any fields which need to be dash-escaped, + // but implementations are free to escape all lines … + if (not buf.writeNewLineIf(ContentFile, not first_line) || not buf.writeTo(ContentFile, 2)) + return false; } else - signed_message_not_on_first_line = found_garbage = true; + // § 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"); } - else if (skip_until_empty_line == true) + else if (not buf.writeNewLineIf(ContentFile, not first_line) || not buf.writeTo(ContentFile)) + return false; + first_line = false; + } + + // collect all signatures + bool open_signature = true; + while (true) + { + if (not buf.readFrom(in.get(), InFile, true)) + break; + + if (open_signature) { - 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); + 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 (found_signature == false) + else //if (not open_signature) { - 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 - { - // 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)); - } + if (buf == "-----BEGIN PGP SIGNATURE-----") + open_signature = true; else - found_garbage = true; + return _error->Error("Clearsigned file '%s' contains unsigned lines.", InFile.c_str()); } - else if (found_signature == true) - { - 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 - } - // all the rest is whitespace, unsigned garbage or additional message blocks we ignore - else - found_garbage = true; + + if (not buf.writeLineTo(SignatureFile)) + return false; } - fclose(in); - if (buf != NULL) - free(buf); + if (open_signature) + 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; } /*}}}*/ 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 (MessageFile.Failed() == true) + if (GetTempFile("clearsigned.message", true, &MessageFile) == nullptr) + return false; + 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 @@ -458,10 +556,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(); } /*}}}*/ 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 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/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 <<EOF +-----BEGIN PGP SIGNATURE----- + +iQFEBAEBCgAuFiEENKjp0Y2zIPNn6OqgWpDRQdusja4FAlhT7+kQHGpvZUBleGFt +cGxlLm9yZwAKCRBakNFB26yNrjvEB/9/e3jA1l0fvPafx9LEXcH8CLpUFQK7ra9l +3M4YAH4JKQlTG1be7ixruBRlCTh3YiSs66fKMeJeUYoxA2HPhvbGFEjQFAxunEYg +X/LBKv1mQWa+Q34P5GBjK8kQdLCN+yJAiUErmWNQG3GPninrxsC9tY5jcWvHeP1k +V7N3MLnNqzXaCJM24mnKidC5IDadUdQ8qC8c3rjUexQ8vBz0eucH56jbqV5oOcvx +pjlW965dCPIf3OI8q6J7bIOjyY+u/PTcVlqPq3TUz/ti6RkVbKpLH0D4ll3lUTns +JQt/+gJCPxHUJphy8sccBKhW29CLELJIIafvU30E1nWn9szh2Xjq +=TB1F +-----END PGP SIGNATURE----- +EOF + + gpgvmethod() { - echo '601 Configuration + echo "601 Configuration Config-Item: Debug::Acquire::gpgv=1 Config-Item: Dir::Bin::apt-key=./faked-apt-key Config-Item: APT::Hashes::SHA1::Weak=true 600 URI Acquire -URI: file:///dev/null -Filename: /dev/zero -' | runapt "${METHODSDIR}/gpgv" +URI: file://${TMPWORKINGDIRECTORY}/message.sig +Filename: ${TMPWORKINGDIRECTORY}/message.data +" | runapt "${METHODSDIR}/gpgv" } testrun gpgvmethod() { - echo '601 Configuration + echo "601 Configuration Config-Item: Debug::Acquire::gpgv=1 Config-Item: Dir::Bin::apt-key=./faked-apt-key Config-Item: APT::Hashes::SHA1::Weak=true 600 URI Acquire -URI: file:///dev/null -Filename: /dev/zero +URI: file://${TMPWORKINGDIRECTORY}/message.sig +Filename: ${TMPWORKINGDIRECTORY}/message.data Signed-By: /dev/null,34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE -' | runapt "${METHODSDIR}/gpgv" +" | runapt "${METHODSDIR}/gpgv" } testrun gpgvmethod() { - echo '601 Configuration + echo "601 Configuration Config-Item: Debug::Acquire::gpgv=1 Config-Item: Dir::Bin::apt-key=./faked-apt-key Config-Item: APT::Hashes::SHA1::Weak=true 600 URI Acquire -URI: file:///dev/null -Filename: /dev/zero +URI: file://${TMPWORKINGDIRECTORY}/message.sig +Filename: ${TMPWORKINGDIRECTORY}/message.data Signed-By: 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE,/dev/null -' | runapt "${METHODSDIR}/gpgv" +" | runapt "${METHODSDIR}/gpgv" } testrun @@ -122,16 +138,16 @@ testsuccess grep '^\s\+Good:\s\+$' method.output testsuccess grep 'verified because the public key is not available: GOODSIG' method.output gpgvmethod() { - echo '601 Configuration + echo "601 Configuration Config-Item: Debug::Acquire::gpgv=1 Config-Item: Dir::Bin::apt-key=./faked-apt-key Config-Item: APT::Hashes::SHA1::Weak=true 600 URI Acquire -URI: file:///dev/null -Filename: /dev/zero +URI: file://${TMPWORKINGDIRECTORY}/message.sig +Filename: ${TMPWORKINGDIRECTORY}/message.data Signed-By: 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE! -' | runapt "${METHODSDIR}/gpgv" +" | runapt "${METHODSDIR}/gpgv" } testgpgv 'Exact matched subkey signed with long keyid' 'Good: GOODSIG 5A90D141DBAC8DAE' '34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE!' '[GNUPG:] GOODSIG 5A90D141DBAC8DAE Sebastian Subkey <subkey@example.org> [GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2018-08-16 1534459673 0 4 0 1 11 00 4281DEDBD466EAE8C1F4157E5B6896415D44C43E' diff --git a/test/libapt/openmaybeclearsignedfile_test.cc b/test/libapt/openmaybeclearsignedfile_test.cc index 1f63fb8fc..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; @@ -190,19 +189,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 +240,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 +305,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 +335,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) @@ -371,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); +} |