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 /apt-pkg | |
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
Diffstat (limited to 'apt-pkg')
-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 |
3 files changed, 279 insertions, 182 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() { |