From 2f5b615169aef2d9c74bb337af229dee2dce595e Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 15 Mar 2013 14:17:01 +0100 Subject: * apt-pkg/indexcopy.cc: - rename RunGPGV to ExecGPGV and move it to apt-pkg/contrib/gpgv.cc --- apt-pkg/contrib/gpgv.cc | 138 ++++++++++++++++++++++++++++++++++++++++++++++++ apt-pkg/contrib/gpgv.h | 24 +++++++++ apt-pkg/indexcopy.cc | 122 +----------------------------------------- apt-pkg/indexcopy.h | 15 +++--- apt-pkg/makefile | 14 +++-- debian/changelog | 8 +++ methods/gpgv.cc | 3 +- 7 files changed, 189 insertions(+), 135 deletions(-) create mode 100644 apt-pkg/contrib/gpgv.cc create mode 100644 apt-pkg/contrib/gpgv.h diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc new file mode 100644 index 000000000..9b008dd4f --- /dev/null +++ b/apt-pkg/contrib/gpgv.cc @@ -0,0 +1,138 @@ +// -*- mode: cpp; mode: fold -*- +// Include Files /*{{{*/ +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include + /*}}}*/ + +using namespace std; + +// RunGPGV - returns the command needed for verify /*{{{*/ +// --------------------------------------------------------------------- +/* Generating the commandline for calling gpgv is somehow complicated as + we need to add multiple keyrings and user supplied options. */ +bool ExecGPGV(std::string const &File, std::string const &FileGPG, + int const &statusfd, int fd[2]) +{ + if (File == FileGPG) + { + #define SIGMSG "-----BEGIN PGP SIGNED MESSAGE-----\n" + char buffer[sizeof(SIGMSG)]; + FILE* gpg = fopen(File.c_str(), "r"); + if (gpg == NULL) + return _error->Errno("RunGPGV", _("Could not open file %s"), File.c_str()); + char const * const test = fgets(buffer, sizeof(buffer), gpg); + fclose(gpg); + if (test == NULL || strcmp(buffer, SIGMSG) != 0) + return _error->Error(_("File %s doesn't start with a clearsigned message"), File.c_str()); + #undef SIGMSG + } + + + string const gpgvpath = _config->Find("Dir::Bin::gpg", "/usr/bin/gpgv"); + // FIXME: remove support for deprecated APT::GPGV setting + string const trustedFile = _config->Find("APT::GPGV::TrustedKeyring", _config->FindFile("Dir::Etc::Trusted")); + string const trustedPath = _config->FindDir("Dir::Etc::TrustedParts"); + + bool const Debug = _config->FindB("Debug::Acquire::gpgv", false); + + if (Debug == true) + { + std::clog << "gpgv path: " << gpgvpath << std::endl; + std::clog << "Keyring file: " << trustedFile << std::endl; + std::clog << "Keyring path: " << trustedPath << std::endl; + } + + std::vector keyrings; + if (DirectoryExists(trustedPath)) + keyrings = GetListOfFilesInDir(trustedPath, "gpg", false, true); + if (RealFileExists(trustedFile) == true) + keyrings.push_back(trustedFile); + + std::vector Args; + Args.reserve(30); + + if (keyrings.empty() == true) + { + // TRANSLATOR: %s is the trusted keyring parts directory + return _error->Error(_("No keyring installed in %s."), + _config->FindDir("Dir::Etc::TrustedParts").c_str()); + } + + Args.push_back(gpgvpath.c_str()); + Args.push_back("--ignore-time-conflict"); + + if (statusfd != -1) + { + Args.push_back("--status-fd"); + char fd[10]; + snprintf(fd, sizeof(fd), "%i", statusfd); + Args.push_back(fd); + } + + for (vector::const_iterator K = keyrings.begin(); + K != keyrings.end(); ++K) + { + Args.push_back("--keyring"); + Args.push_back(K->c_str()); + } + + Configuration::Item const *Opts; + Opts = _config->Tree("Acquire::gpgv::Options"); + if (Opts != 0) + { + Opts = Opts->Child; + for (; Opts != 0; Opts = Opts->Next) + { + if (Opts->Value.empty() == true) + continue; + Args.push_back(Opts->Value.c_str()); + } + } + + Args.push_back(FileGPG.c_str()); + if (FileGPG != File) + Args.push_back(File.c_str()); + Args.push_back(NULL); + + if (Debug == true) + { + std::clog << "Preparing to exec: " << gpgvpath; + for (std::vector::const_iterator a = Args.begin(); *a != NULL; ++a) + std::clog << " " << *a; + std::clog << std::endl; + } + + if (statusfd != -1) + { + int const nullfd = open("/dev/null", O_RDONLY); + close(fd[0]); + // Redirect output to /dev/null; we read from the status fd + dup2(nullfd, STDOUT_FILENO); + dup2(nullfd, STDERR_FILENO); + // Redirect the pipe to the status fd (3) + dup2(fd[1], statusfd); + + putenv((char *)"LANG="); + putenv((char *)"LC_ALL="); + putenv((char *)"LC_MESSAGES="); + } + + execvp(gpgvpath.c_str(), (char **) &Args[0]); + return true; +} + /*}}}*/ diff --git a/apt-pkg/contrib/gpgv.h b/apt-pkg/contrib/gpgv.h new file mode 100644 index 000000000..c15166c94 --- /dev/null +++ b/apt-pkg/contrib/gpgv.h @@ -0,0 +1,24 @@ +// -*- mode: cpp; mode: fold -*- +// Description /*{{{*/ +/* ###################################################################### + + Helpers to deal with gpgv better and more easily + + ##################################################################### */ + /*}}}*/ +#ifndef CONTRIB_GPGV_H +#define CONTRIB_GPGV_H + +#include + +/** \brief generates and run the command to verify a file with gpgv */ +bool ExecGPGV(std::string const &File, std::string const &FileOut, + int const &statusfd, int fd[2]); + +inline bool ExecGPGV(std::string const &File, std::string const &FileOut, + int const &statusfd = -1) { + int fd[2]; + return ExecGPGV(File, FileOut, statusfd, fd); +} + +#endif diff --git a/apt-pkg/indexcopy.cc b/apt-pkg/indexcopy.cc index aa1f01a4a..f53989bdb 100644 --- a/apt-pkg/indexcopy.cc +++ b/apt-pkg/indexcopy.cc @@ -593,9 +593,9 @@ bool SigVerify::CopyAndVerify(string CDROM,string Name,vector &SigList, if(pid == 0) { if (useInRelease == true) - RunGPGV(inrelease, inrelease); + ExecGPGV(inrelease, inrelease); else - RunGPGV(release, releasegpg); + ExecGPGV(release, releasegpg); } if(!ExecWait(pid, "gpgv")) { @@ -639,124 +639,6 @@ bool SigVerify::CopyAndVerify(string CDROM,string Name,vector &SigList, } } - return true; -} - /*}}}*/ -// SigVerify::RunGPGV - returns the command needed for verify /*{{{*/ -// --------------------------------------------------------------------- -/* Generating the commandline for calling gpgv is somehow complicated as - we need to add multiple keyrings and user supplied options. Also, as - the cdrom code currently can not use the gpgv method we have two places - these need to be done - so the place for this method is wrong but better - than code duplication… */ -bool SigVerify::RunGPGV(std::string const &File, std::string const &FileGPG, - int const &statusfd, int fd[2]) -{ - if (File == FileGPG) - { - #define SIGMSG "-----BEGIN PGP SIGNED MESSAGE-----\n" - char buffer[sizeof(SIGMSG)]; - FILE* gpg = fopen(File.c_str(), "r"); - if (gpg == NULL) - return _error->Errno("RunGPGV", _("Could not open file %s"), File.c_str()); - char const * const test = fgets(buffer, sizeof(buffer), gpg); - fclose(gpg); - if (test == NULL || strcmp(buffer, SIGMSG) != 0) - return _error->Error(_("File %s doesn't start with a clearsigned message"), File.c_str()); - #undef SIGMSG - } - - - string const gpgvpath = _config->Find("Dir::Bin::gpg", "/usr/bin/gpgv"); - // FIXME: remove support for deprecated APT::GPGV setting - string const trustedFile = _config->Find("APT::GPGV::TrustedKeyring", _config->FindFile("Dir::Etc::Trusted")); - string const trustedPath = _config->FindDir("Dir::Etc::TrustedParts"); - - bool const Debug = _config->FindB("Debug::Acquire::gpgv", false); - - if (Debug == true) - { - std::clog << "gpgv path: " << gpgvpath << std::endl; - std::clog << "Keyring file: " << trustedFile << std::endl; - std::clog << "Keyring path: " << trustedPath << std::endl; - } - - std::vector keyrings; - if (DirectoryExists(trustedPath)) - keyrings = GetListOfFilesInDir(trustedPath, "gpg", false, true); - if (RealFileExists(trustedFile) == true) - keyrings.push_back(trustedFile); - - std::vector Args; - Args.reserve(30); - - if (keyrings.empty() == true) - { - // TRANSLATOR: %s is the trusted keyring parts directory - return _error->Error(_("No keyring installed in %s."), - _config->FindDir("Dir::Etc::TrustedParts").c_str()); - } - - Args.push_back(gpgvpath.c_str()); - Args.push_back("--ignore-time-conflict"); - - if (statusfd != -1) - { - Args.push_back("--status-fd"); - char fd[10]; - snprintf(fd, sizeof(fd), "%i", statusfd); - Args.push_back(fd); - } - - for (vector::const_iterator K = keyrings.begin(); - K != keyrings.end(); ++K) - { - Args.push_back("--keyring"); - Args.push_back(K->c_str()); - } - - Configuration::Item const *Opts; - Opts = _config->Tree("Acquire::gpgv::Options"); - if (Opts != 0) - { - Opts = Opts->Child; - for (; Opts != 0; Opts = Opts->Next) - { - if (Opts->Value.empty() == true) - continue; - Args.push_back(Opts->Value.c_str()); - } - } - - Args.push_back(FileGPG.c_str()); - if (FileGPG != File) - Args.push_back(File.c_str()); - Args.push_back(NULL); - - if (Debug == true) - { - std::clog << "Preparing to exec: " << gpgvpath; - for (std::vector::const_iterator a = Args.begin(); *a != NULL; ++a) - std::clog << " " << *a; - std::clog << std::endl; - } - - if (statusfd != -1) - { - int const nullfd = open("/dev/null", O_RDONLY); - close(fd[0]); - // Redirect output to /dev/null; we read from the status fd - dup2(nullfd, STDOUT_FILENO); - dup2(nullfd, STDERR_FILENO); - // Redirect the pipe to the status fd (3) - dup2(fd[1], statusfd); - - putenv((char *)"LANG="); - putenv((char *)"LC_ALL="); - putenv((char *)"LC_MESSAGES="); - } - - execvp(gpgvpath.c_str(), (char **) &Args[0]); return true; } /*}}}*/ diff --git a/apt-pkg/indexcopy.h b/apt-pkg/indexcopy.h index e3de1afd9..49e724f2f 100644 --- a/apt-pkg/indexcopy.h +++ b/apt-pkg/indexcopy.h @@ -14,6 +14,9 @@ #include #include +#include +#include + #ifndef APT_8_CLEANER_HEADERS using std::string; using std::vector; @@ -96,13 +99,13 @@ class SigVerify /*{{{*/ bool CopyAndVerify(std::string CDROM,std::string Name,std::vector &SigList, std::vector PkgList,std::vector SrcList); - /** \brief generates and run the command to verify a file with gpgv */ - static bool RunGPGV(std::string const &File, std::string const &FileOut, - int const &statusfd, int fd[2]); - inline static bool RunGPGV(std::string const &File, std::string const &FileOut, + __deprecated static bool RunGPGV(std::string const &File, std::string const &FileOut, + int const &statusfd, int fd[2]) { + return ExecGPGV(File, FileOut, statusfd, fd); + }; + __deprecated static bool RunGPGV(std::string const &File, std::string const &FileOut, int const &statusfd = -1) { - int fd[2]; - return RunGPGV(File, FileOut, statusfd, fd); + return ExecGPGV(File, FileOut, statusfd); }; }; /*}}}*/ diff --git a/apt-pkg/makefile b/apt-pkg/makefile index 27d7ead24..59729faf5 100644 --- a/apt-pkg/makefile +++ b/apt-pkg/makefile @@ -27,15 +27,13 @@ APT_DOMAIN:=libapt-pkg$(LIBAPTPKG_MAJOR) SOURCE = contrib/mmap.cc contrib/error.cc contrib/strutl.cc \ contrib/configuration.cc contrib/progress.cc contrib/cmndline.cc \ contrib/hashsum.cc contrib/md5.cc contrib/sha1.cc \ - contrib/sha2_internal.cc\ - contrib/hashes.cc \ + contrib/sha2_internal.cc contrib/hashes.cc \ contrib/cdromutl.cc contrib/crc-16.cc contrib/netrc.cc \ - contrib/fileutl.cc -HEADERS = mmap.h error.h configuration.h fileutl.h cmndline.h netrc.h\ - md5.h crc-16.h cdromutl.h strutl.h sptr.h sha1.h sha2.h sha256.h\ - sha2_internal.h \ - hashes.h hashsum_template.h\ - macros.h weakptr.h + contrib/fileutl.cc contrib/gpgv.cc +HEADERS = mmap.h error.h configuration.h fileutl.h cmndline.h netrc.h \ + md5.h crc-16.h cdromutl.h strutl.h sptr.h sha1.h sha2.h sha256.h \ + sha2_internal.h hashes.h hashsum_template.h \ + macros.h weakptr.h gpgv.h # Source code for the core main library SOURCE+= pkgcache.cc version.cc depcache.cc \ diff --git a/debian/changelog b/debian/changelog index 9ed9b4d61..ac630ad7e 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,11 @@ +apt (0.9.7.9) UNRELEASED; urgency=low + + [ David Kalnischkies ] + * apt-pkg/indexcopy.cc: + - rename RunGPGV to ExecGPGV and move it to apt-pkg/contrib/gpgv.cc + + -- David Kalnischkies Fri, 15 Mar 2013 14:15:43 +0100 + apt (0.9.7.8) unstable; urgency=criticial * SECURITY UPDATE: InRelease verification bypass diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 25ba0d063..98381b845 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -72,7 +73,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, else if (pid == 0) { _error->PushToStack(); - bool const success = SigVerify::RunGPGV(outfile, file, 3, fd); + bool const success = ExecGPGV(outfile, file, 3, fd); if (success == false) { string errmsg; -- cgit v1.2.3 From 99ed26d32226f0dffe5a37fb78c5588f9d9ecfd5 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 15 Mar 2013 14:29:46 +0100 Subject: * apt-pkg/contrib/gpgv.cc: - ExecGPGV is a method which should never return, so mark it as such and fix the inconsistency of returning in error cases --- apt-pkg/contrib/gpgv.cc | 22 ++++++++++++++++------ apt-pkg/contrib/gpgv.h | 26 ++++++++++++++++++++------ apt-pkg/indexcopy.h | 6 ++++-- debian/changelog | 3 +++ methods/gpgv.cc | 14 +------------- 5 files changed, 44 insertions(+), 27 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 9b008dd4f..9760bd21f 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -25,20 +25,28 @@ using namespace std; // --------------------------------------------------------------------- /* Generating the commandline for calling gpgv is somehow complicated as we need to add multiple keyrings and user supplied options. */ -bool ExecGPGV(std::string const &File, std::string const &FileGPG, +void ExecGPGV(std::string const &File, std::string const &FileGPG, int const &statusfd, int fd[2]) { + #define EINTERNAL 111 + if (File == FileGPG) { #define SIGMSG "-----BEGIN PGP SIGNED MESSAGE-----\n" char buffer[sizeof(SIGMSG)]; FILE* gpg = fopen(File.c_str(), "r"); if (gpg == NULL) - return _error->Errno("RunGPGV", _("Could not open file %s"), File.c_str()); + { + ioprintf(std::cerr, _("Could not open file %s"), File.c_str()); + exit(EINTERNAL); + } char const * const test = fgets(buffer, sizeof(buffer), gpg); fclose(gpg); if (test == NULL || strcmp(buffer, SIGMSG) != 0) - return _error->Error(_("File %s doesn't start with a clearsigned message"), File.c_str()); + { + ioprintf(std::cerr, _("File %s doesn't start with a clearsigned message"), File.c_str()); + exit(EINTERNAL); + } #undef SIGMSG } @@ -69,8 +77,9 @@ bool ExecGPGV(std::string const &File, std::string const &FileGPG, if (keyrings.empty() == true) { // TRANSLATOR: %s is the trusted keyring parts directory - return _error->Error(_("No keyring installed in %s."), - _config->FindDir("Dir::Etc::TrustedParts").c_str()); + ioprintf(std::cerr, _("No keyring installed in %s."), + _config->FindDir("Dir::Etc::TrustedParts").c_str()); + exit(EINTERNAL); } Args.push_back(gpgvpath.c_str()); @@ -133,6 +142,7 @@ bool ExecGPGV(std::string const &File, std::string const &FileGPG, } execvp(gpgvpath.c_str(), (char **) &Args[0]); - return true; + ioprintf(std::cerr, "Couldn't execute %s to check %s", Args[0], File.c_str()); + exit(EINTERNAL); } /*}}}*/ diff --git a/apt-pkg/contrib/gpgv.h b/apt-pkg/contrib/gpgv.h index c15166c94..8aeea2fb3 100644 --- a/apt-pkg/contrib/gpgv.h +++ b/apt-pkg/contrib/gpgv.h @@ -11,14 +11,28 @@ #include -/** \brief generates and run the command to verify a file with gpgv */ -bool ExecGPGV(std::string const &File, std::string const &FileOut, - int const &statusfd, int fd[2]); +#if __GNUC__ >= 4 + #define APT_noreturn __attribute__ ((noreturn)) +#else + #define APT_noreturn /* no support */ +#endif -inline bool ExecGPGV(std::string const &File, std::string const &FileOut, +/** \brief generates and run the command to verify a file with gpgv + * + * If File and FileSig specify the same file it is assumed that we + * deal with a clear-signed message. + * + * @param File is the message (unsigned or clear-signed) + * @param FileSig is the signature (detached or clear-signed) + */ +void ExecGPGV(std::string const &File, std::string const &FileSig, + int const &statusfd, int fd[2]) APT_noreturn; +inline void ExecGPGV(std::string const &File, std::string const &FileSig, int const &statusfd = -1) { int fd[2]; - return ExecGPGV(File, FileOut, statusfd, fd); -} + ExecGPGV(File, FileSig, statusfd, fd); +}; + +#undef APT_noreturn #endif diff --git a/apt-pkg/indexcopy.h b/apt-pkg/indexcopy.h index 49e724f2f..aa221158e 100644 --- a/apt-pkg/indexcopy.h +++ b/apt-pkg/indexcopy.h @@ -101,11 +101,13 @@ class SigVerify /*{{{*/ __deprecated static bool RunGPGV(std::string const &File, std::string const &FileOut, int const &statusfd, int fd[2]) { - return ExecGPGV(File, FileOut, statusfd, fd); + ExecGPGV(File, FileOut, statusfd, fd); + return false; }; __deprecated static bool RunGPGV(std::string const &File, std::string const &FileOut, int const &statusfd = -1) { - return ExecGPGV(File, FileOut, statusfd); + ExecGPGV(File, FileOut, statusfd); + return false; }; }; /*}}}*/ diff --git a/debian/changelog b/debian/changelog index ac630ad7e..bd4116406 100644 --- a/debian/changelog +++ b/debian/changelog @@ -3,6 +3,9 @@ apt (0.9.7.9) UNRELEASED; urgency=low [ David Kalnischkies ] * apt-pkg/indexcopy.cc: - rename RunGPGV to ExecGPGV and move it to apt-pkg/contrib/gpgv.cc + * apt-pkg/contrib/gpgv.cc: + - ExecGPGV is a method which should never return, so mark it as such + and fix the inconsistency of returning in error cases -- David Kalnischkies Fri, 15 Mar 2013 14:15:43 +0100 diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 98381b845..3f814b9f0 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -71,19 +71,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, if (pid < 0) return string("Couldn't spawn new process") + strerror(errno); else if (pid == 0) - { - _error->PushToStack(); - bool const success = ExecGPGV(outfile, file, 3, fd); - if (success == false) - { - string errmsg; - _error->PopMessage(errmsg); - _error->RevertToStack(); - return errmsg; - } - _error->RevertToStack(); - exit(111); - } + ExecGPGV(outfile, file, 3, fd); close(fd[1]); FILE *pipein = fdopen(fd[0], "r"); -- cgit v1.2.3 From b38bb727530a7e836689ef100b07926522066986 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 15 Mar 2013 14:49:05 +0100 Subject: don't close stdout/stderr if it is also the statusfd --- apt-pkg/contrib/gpgv.cc | 21 +++++++++++---------- debian/changelog | 1 + 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 9760bd21f..94a1f8778 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -2,14 +2,13 @@ // Include Files /*{{{*/ #include -#include -#include -#include +#include +#include #include #include #include -#include -#include + +#include #include #include @@ -85,12 +84,12 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, Args.push_back(gpgvpath.c_str()); Args.push_back("--ignore-time-conflict"); + char statusfdstr[10]; if (statusfd != -1) { Args.push_back("--status-fd"); - char fd[10]; - snprintf(fd, sizeof(fd), "%i", statusfd); - Args.push_back(fd); + snprintf(statusfdstr, sizeof(fd), "%i", statusfd); + Args.push_back(statusfdstr); } for (vector::const_iterator K = keyrings.begin(); @@ -131,8 +130,10 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, int const nullfd = open("/dev/null", O_RDONLY); close(fd[0]); // Redirect output to /dev/null; we read from the status fd - dup2(nullfd, STDOUT_FILENO); - dup2(nullfd, STDERR_FILENO); + if (statusfd != STDOUT_FILENO) + dup2(nullfd, STDOUT_FILENO); + if (statusfd != STDERR_FILENO) + dup2(nullfd, STDERR_FILENO); // Redirect the pipe to the status fd (3) dup2(fd[1], statusfd); diff --git a/debian/changelog b/debian/changelog index bd4116406..0423cefa6 100644 --- a/debian/changelog +++ b/debian/changelog @@ -6,6 +6,7 @@ apt (0.9.7.9) UNRELEASED; urgency=low * apt-pkg/contrib/gpgv.cc: - ExecGPGV is a method which should never return, so mark it as such and fix the inconsistency of returning in error cases + - don't close stdout/stderr if it is also the statusfd -- David Kalnischkies Fri, 15 Mar 2013 14:15:43 +0100 -- cgit v1.2.3 From 39f38a81b85edf2e1bdc4e92267a63cc6c928734 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 15 Mar 2013 14:55:43 +0100 Subject: * apt-pkg/acquire-item.cc: - keep the last good InRelease file around just as we do it with Release.gpg in case the new one we download isn't good for us --- apt-pkg/acquire-item.cc | 26 ++++++++++++++++---------- debian/changelog | 3 +++ 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index a30e98858..39b842dfb 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -1503,20 +1503,17 @@ void pkgAcqMetaIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf) if (AuthPass == true) { // gpgv method failed, if we have a good signature - string LastGoodSigFile = _config->FindDir("Dir::State::lists"); - if (DestFile == SigFile) - LastGoodSigFile.append(URItoFileName(RealURI)); - else - LastGoodSigFile.append("partial/").append(URItoFileName(RealURI)).append(".gpg.reverify"); + string LastGoodSigFile = _config->FindDir("Dir::State::lists").append("partial/").append(URItoFileName(RealURI)); + if (DestFile != SigFile) + LastGoodSigFile.append(".gpg"); + LastGoodSigFile.append(".reverify"); if(FileExists(LastGoodSigFile)) { + string VerifiedSigFile = _config->FindDir("Dir::State::lists") + URItoFileName(RealURI); if (DestFile != SigFile) - { - string VerifiedSigFile = _config->FindDir("Dir::State::lists") + - URItoFileName(RealURI) + ".gpg"; - Rename(LastGoodSigFile,VerifiedSigFile); - } + VerifiedSigFile.append(".gpg"); + Rename(LastGoodSigFile, VerifiedSigFile); Status = StatTransientNetworkError; _error->Warning(_("A error occurred during the signature " "verification. The repository is not updated " @@ -1577,6 +1574,15 @@ pkgAcqMetaClearSig::pkgAcqMetaClearSig(pkgAcquire *Owner, /*{{{*/ MetaSigURI(MetaSigURI), MetaSigURIDesc(MetaSigURIDesc), MetaSigShortDesc(MetaSigShortDesc) { SigFile = DestFile; + + // keep the old InRelease around in case of transistent network errors + string const Final = _config->FindDir("Dir::State::lists") + URItoFileName(RealURI); + struct stat Buf; + if (stat(Final.c_str(),&Buf) == 0) + { + string const LastGoodSig = DestFile + ".reverify"; + Rename(Final,LastGoodSig); + } } /*}}}*/ // pkgAcqMetaClearSig::Custom600Headers - Insert custom request headers /*{{{*/ diff --git a/debian/changelog b/debian/changelog index 0423cefa6..77e8674a0 100644 --- a/debian/changelog +++ b/debian/changelog @@ -7,6 +7,9 @@ apt (0.9.7.9) UNRELEASED; urgency=low - ExecGPGV is a method which should never return, so mark it as such and fix the inconsistency of returning in error cases - don't close stdout/stderr if it is also the statusfd + * apt-pkg/acquire-item.cc: + - keep the last good InRelease file around just as we do it with + Release.gpg in case the new one we download isn't good for us -- David Kalnischkies Fri, 15 Mar 2013 14:15:43 +0100 -- cgit v1.2.3 From 4fb400a66f2436cba6c89cecdc9560c9b1c54337 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 15 Mar 2013 14:57:27 +0100 Subject: split out a method to strip whitespaces only on the right side --- apt-pkg/contrib/strutl.cc | 8 +++++++- apt-pkg/contrib/strutl.h | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index ca096d736..9726138a0 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -117,7 +117,13 @@ char *_strstrip(char *String) if (*String == 0) return String; - + return _strrstrip(String); +} + /*}}}*/ +// strrstrip - Remove white space from the back of a string /*{{{*/ +// --------------------------------------------------------------------- +char *_strrstrip(char *String) +{ char *End = String + strlen(String) - 1; for (;End != String - 1 && (*End == ' ' || *End == '\t' || *End == '\n' || *End == '\r'); End--); diff --git a/apt-pkg/contrib/strutl.h b/apt-pkg/contrib/strutl.h index 337139d5d..e92f91dc0 100644 --- a/apt-pkg/contrib/strutl.h +++ b/apt-pkg/contrib/strutl.h @@ -35,6 +35,7 @@ using std::ostream; bool UTF8ToCodeset(const char *codeset, const std::string &orig, std::string *dest); char *_strstrip(char *String); +char *_strrstrip(char *String); // right strip only char *_strtabexpand(char *String,size_t Len); bool ParseQuoteWord(const char *&String,std::string &Res); bool ParseCWord(const char *&String,std::string &Res); -- cgit v1.2.3 From 2d3fe9cfadb33556b7563a98bb5a4698888e6c40 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 15 Mar 2013 18:53:53 +0100 Subject: - if ExecGPGV deals with a clear-signed file it will split this file into data and signatures, pass it to gpgv for verification and recombines it after that in a known-good way without unsigned blocks and whitespaces resulting usually in more or less the same file as before, but later code can be sure about the format * apt-pkg/deb/debmetaindex.cc: - reenable InRelease by default --- apt-pkg/contrib/gpgv.cc | 302 ++++++++++++++++++--- apt-pkg/contrib/gpgv.h | 41 ++- apt-pkg/deb/debmetaindex.cc | 31 ++- debian/changelog | 7 + ...st-ubuntu-bug-784473-InRelease-one-message-only | 14 +- 5 files changed, 337 insertions(+), 58 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 94a1f8778..c236a7289 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -2,58 +2,54 @@ // Include Files /*{{{*/ #include +#include #include +#include #include +#include #include #include -#include - -#include +#include -#include -#include -#include -#include +#include +#include +#include +#include +#include #include /*}}}*/ +char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/ +{ + const char *tmpdir = getenv("TMPDIR"); +#ifdef P_tmpdir + if (!tmpdir) + tmpdir = P_tmpdir; +#endif + if (!tmpdir) + tmpdir = "/tmp"; -using namespace std; - -// RunGPGV - returns the command needed for verify /*{{{*/ + std::string out; + strprintf(out, "%s/%s.XXXXXX", tmpdir, basename); + return strdup(out.c_str()); +} + /*}}}*/ +// ExecGPGV - returns the command needed for verify /*{{{*/ // --------------------------------------------------------------------- /* Generating the commandline for calling gpgv is somehow complicated as - we need to add multiple keyrings and user supplied options. */ + we need to add multiple keyrings and user supplied options. + Also, as gpgv has no options to enforce a certain reduced style of + clear-signed files (=the complete content of the file is signed and + the content isn't encoded) we do a divide and conquer approach here +*/ void ExecGPGV(std::string const &File, std::string const &FileGPG, - int const &statusfd, int fd[2]) + int const &statusfd, int fd[2]) { #define EINTERNAL 111 - - if (File == FileGPG) - { - #define SIGMSG "-----BEGIN PGP SIGNED MESSAGE-----\n" - char buffer[sizeof(SIGMSG)]; - FILE* gpg = fopen(File.c_str(), "r"); - if (gpg == NULL) - { - ioprintf(std::cerr, _("Could not open file %s"), File.c_str()); - exit(EINTERNAL); - } - char const * const test = fgets(buffer, sizeof(buffer), gpg); - fclose(gpg); - if (test == NULL || strcmp(buffer, SIGMSG) != 0) - { - ioprintf(std::cerr, _("File %s doesn't start with a clearsigned message"), File.c_str()); - exit(EINTERNAL); - } - #undef SIGMSG - } - - - string const gpgvpath = _config->Find("Dir::Bin::gpg", "/usr/bin/gpgv"); + std::string const gpgvpath = _config->Find("Dir::Bin::gpg", "/usr/bin/gpgv"); // FIXME: remove support for deprecated APT::GPGV setting - string const trustedFile = _config->Find("APT::GPGV::TrustedKeyring", _config->FindFile("Dir::Etc::Trusted")); - string const trustedPath = _config->FindDir("Dir::Etc::TrustedParts"); + std::string const trustedFile = _config->Find("APT::GPGV::TrustedKeyring", _config->FindFile("Dir::Etc::Trusted")); + std::string const trustedPath = _config->FindDir("Dir::Etc::TrustedParts"); bool const Debug = _config->FindB("Debug::Acquire::gpgv", false); @@ -64,7 +60,7 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, std::clog << "Keyring path: " << trustedPath << std::endl; } - std::vector keyrings; + std::vector keyrings; if (DirectoryExists(trustedPath)) keyrings = GetListOfFilesInDir(trustedPath, "gpg", false, true); if (RealFileExists(trustedFile) == true) @@ -88,11 +84,11 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, if (statusfd != -1) { Args.push_back("--status-fd"); - snprintf(statusfdstr, sizeof(fd), "%i", statusfd); + snprintf(statusfdstr, sizeof(statusfdstr), "%i", statusfd); Args.push_back(statusfdstr); } - for (vector::const_iterator K = keyrings.begin(); + for (std::vector::const_iterator K = keyrings.begin(); K != keyrings.end(); ++K) { Args.push_back("--keyring"); @@ -112,9 +108,49 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } } - Args.push_back(FileGPG.c_str()); + int sigFd = -1; + int dataFd = -1; + std::vector dataHeader; + char * sig = NULL; + char * data = NULL; + + // file with detached signature if (FileGPG != File) + { + 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) + { + ioprintf(std::cerr, "Couldn't create tempfiles for splitting up %s", File.c_str()); + exit(EINTERNAL); + } + + sigFd = mkstemp(sig); + dataFd = mkstemp(data); + int const duppedSigFd = dup(sigFd); + int const duppedDataFd = dup(dataFd); + + if (dataFd == -1 || sigFd == -1 || duppedDataFd == -1 || duppedSigFd == -1 || + SplitClearSignedFile(File, duppedDataFd, &dataHeader, duppedSigFd) == false) + { + if (dataFd != -1) + unlink(sig); + if (sigFd != -1) + unlink(data); + ioprintf(std::cerr, "Splitting up %s into data and signature failed", File.c_str()); + exit(EINTERNAL); + } + lseek(dataFd, 0, SEEK_SET); + lseek(sigFd, 0, SEEK_SET); + Args.push_back(sig); + Args.push_back(data); + } + Args.push_back(NULL); if (Debug == true) @@ -142,8 +178,186 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, putenv((char *)"LC_MESSAGES="); } - execvp(gpgvpath.c_str(), (char **) &Args[0]); - ioprintf(std::cerr, "Couldn't execute %s to check %s", Args[0], File.c_str()); - exit(EINTERNAL); + if (FileGPG != File) + { + execvp(gpgvpath.c_str(), (char **) &Args[0]); + ioprintf(std::cerr, "Couldn't execute %s to check %s", Args[0], File.c_str()); + exit(EINTERNAL); + } + else + { +//#define UNLINK_EXIT(X) exit(X) +#define UNLINK_EXIT(X) unlink(sig);unlink(data);exit(X) + + // for clear-signed files we have created tempfiles we have to clean up + // and we do an additional check, so fork yet another time … + pid_t pid = ExecFork(); + if(pid < 0) { + ioprintf(std::cerr, "Fork failed for %s to check %s", Args[0], File.c_str()); + UNLINK_EXIT(EINTERNAL); + } + if(pid == 0) + { + if (statusfd != -1) + dup2(fd[1], statusfd); + execvp(gpgvpath.c_str(), (char **) &Args[0]); + ioprintf(std::cerr, "Couldn't execute %s to check %s", Args[0], File.c_str()); + UNLINK_EXIT(EINTERNAL); + } + + // Wait and collect the error code - taken from WaitPid as we need the exact Status + int Status; + while (waitpid(pid,&Status,0) != pid) + { + if (errno == EINTR) + continue; + ioprintf(std::cerr, _("Waited for %s but it wasn't there"), "gpgv"); + UNLINK_EXIT(EINTERNAL); + } + + // check if it exit'ed normally … + if (WIFEXITED(Status) == false) + { + ioprintf(std::cerr, _("Sub-process %s exited unexpectedly"), "gpgv"); + UNLINK_EXIT(EINTERNAL); + } + + // … and with a good exit code + if (WEXITSTATUS(Status) != 0) + { + ioprintf(std::cerr, _("Sub-process %s returned an error code (%u)"), "gpgv", WEXITSTATUS(Status)); + UNLINK_EXIT(WEXITSTATUS(Status)); + } + + /* looks like its fine. Our caller will check the status fd, + but we construct a good-known clear-signed file without garbage + and other non-sense. In a perfect world, we get the same file, + but empty lines, trailing whitespaces and stuff makes it inperfect … */ + if (RecombineToClearSignedFile(File, dataFd, dataHeader, sigFd) == false) + { + _error->DumpErrors(std::cerr); + UNLINK_EXIT(EINTERNAL); + } + + // everything fine, we have a clean file now! + UNLINK_EXIT(0); +#undef UNLINK_EXIT + } + exit(EINTERNAL); // unreachable safe-guard +} + /*}}}*/ +// RecombineToClearSignedFile - combine data/signature to message /*{{{*/ +bool RecombineToClearSignedFile(std::string const &OutFile, int const ContentFile, + std::vector const &ContentHeader, int const SignatureFile) +{ + FILE *clean_file = fopen(OutFile.c_str(), "w"); + fputs("-----BEGIN PGP SIGNED MESSAGE-----\n", clean_file); + for (std::vector::const_iterator h = ContentHeader.begin(); h != ContentHeader.end(); ++h) + fprintf(clean_file, "%s\n", h->c_str()); + fputs("\n", clean_file); + + FILE *data_file = fdopen(ContentFile, "r"); + FILE *sig_file = fdopen(SignatureFile, "r"); + if (data_file == NULL || sig_file == NULL) + return _error->Error("Couldn't open splitfiles to recombine them into %s", OutFile.c_str()); + char *buf = NULL; + size_t buf_size = 0; + while (getline(&buf, &buf_size, data_file) != -1) + fputs(buf, clean_file); + fclose(data_file); + fputs("\n", clean_file); + while (getline(&buf, &buf_size, sig_file) != -1) + fputs(buf, clean_file); + fclose(sig_file); + fclose(clean_file); + return true; } /*}}}*/ +// SplitClearSignedFile - split message into data/signature /*{{{*/ +bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, + std::vector * const ContentHeader, int const SignatureFile) +{ + FILE *in = fopen(InFile.c_str(), "r"); + if (in == NULL) + return _error->Errno("fopen", "can not open %s", InFile.c_str()); + + FILE *out_content = NULL; + FILE *out_signature = NULL; + if (ContentFile != -1) + { + out_content = fdopen(ContentFile, "w"); + if (out_content == NULL) + return _error->Errno("fdopen", "Failed to open file to write content to from %s", InFile.c_str()); + } + if (SignatureFile != -1) + { + out_signature = fdopen(SignatureFile, "w"); + if (out_signature == NULL) + return _error->Errno("fdopen", "Failed to open file to write signature to from %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; + + char *buf = NULL; + size_t buf_size = 0; + while (getline(&buf, &buf_size, in) != -1) + { + _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 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) + { + if (strcmp(buf, "-----BEGIN PGP SIGNATURE-----") == 0) + { + found_signature = true; + found_message_end = true; + if (out_signature != NULL) + fprintf(out_signature, "%s\n", buf); + } + else if (found_message_end == false) + { + // we are in the message block + if(first_line == true) // first line does not need a newline + { + if (out_content != NULL) + fprintf(out_content, "%s", buf); + first_line = false; + } + else if (out_content != NULL) + fprintf(out_content, "\n%s", buf); + } + } + else if (found_signature == true) + { + if (out_signature != NULL) + fprintf(out_signature, "%s\n", buf); + 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 + } + if (out_content != NULL) + fclose(out_content); + if (out_signature != NULL) + fclose(out_signature); + + return true; +} diff --git a/apt-pkg/contrib/gpgv.h b/apt-pkg/contrib/gpgv.h index 8aeea2fb3..8e04855e4 100644 --- a/apt-pkg/contrib/gpgv.h +++ b/apt-pkg/contrib/gpgv.h @@ -10,6 +10,7 @@ #define CONTRIB_GPGV_H #include +#include #if __GNUC__ >= 4 #define APT_noreturn __attribute__ ((noreturn)) @@ -20,7 +21,9 @@ /** \brief generates and run the command to verify a file with gpgv * * If File and FileSig specify the same file it is assumed that we - * deal with a clear-signed message. + * deal with a clear-signed message. In that case the file will be + * rewritten to be in a good-known format without uneeded whitespaces + * and additional messages (unsigned or signed). * * @param File is the message (unsigned or clear-signed) * @param FileSig is the signature (detached or clear-signed) @@ -35,4 +38,40 @@ inline void ExecGPGV(std::string const &File, std::string const &FileSig, #undef APT_noreturn +/** \brief Split an inline signature into message and signature + * + * Takes a clear-signed message and puts the first signed message + * in the content file and all signatures following it into the + * second. Unsigned messages, additional messages as well as + * whitespaces are discarded. The resulting files are suitable to + * be checked with gpgv. + * + * If one or all Fds are -1 they will not be used and the content + * which would have been written to them is discarded. + * + * The code doesn't support dash-encoded lines as these are not + * expected to be present in files we have to deal with. + * + * @param InFile is the clear-signed file + * @param ContentFile is the Fd the message will be written to + * @param ContentHeader is a list of all required Amored Headers for the message + * @param SignatureFile is the Fd all signatures will be written to + */ +bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, + std::vector * const ContentHeader, int const SignatureFile); + +/** \brief recombines message and signature to an inline signature + * + * Reverses the splitting down by #SplitClearSignedFile by writing + * a well-formed clear-signed message without unsigned messages, + * additional signed messages or just trailing whitespaces + * + * @param OutFile will be clear-signed file + * @param ContentFile is the Fd the message will be read from + * @param ContentHeader is a list of all required Amored Headers for the message + * @param SignatureFile is the Fd all signatures will be read from + */ +bool RecombineToClearSignedFile(std::string const &OutFile, int const ContentFile, + std::vector const &ContentHeader, int const SignatureFile); + #endif diff --git a/apt-pkg/deb/debmetaindex.cc b/apt-pkg/deb/debmetaindex.cc index 6c191fd95..7a88d71e3 100644 --- a/apt-pkg/deb/debmetaindex.cc +++ b/apt-pkg/deb/debmetaindex.cc @@ -229,6 +229,8 @@ vector * debReleaseIndex::ComputeIndexTargets() const { /*}}}*/ bool debReleaseIndex::GetIndexes(pkgAcquire *Owner, bool const &GetAll) const { + bool const tryInRelease = _config->FindB("Acquire::TryInRelease", true); + // special case for --print-uris if (GetAll) { vector *targets = ComputeIndexTargets(); @@ -239,18 +241,27 @@ bool debReleaseIndex::GetIndexes(pkgAcquire *Owner, bool const &GetAll) const // this is normally created in pkgAcqMetaSig, but if we run // in --print-uris mode, we add it here - new pkgAcqMetaIndex(Owner, MetaIndexURI("Release"), - MetaIndexInfo("Release"), "Release", - MetaIndexURI("Release.gpg"), - ComputeIndexTargets(), - new indexRecords (Dist)); + if (tryInRelease == false) + new pkgAcqMetaIndex(Owner, MetaIndexURI("Release"), + MetaIndexInfo("Release"), "Release", + MetaIndexURI("Release.gpg"), + ComputeIndexTargets(), + new indexRecords (Dist)); } - new pkgAcqMetaSig(Owner, MetaIndexURI("Release.gpg"), - MetaIndexInfo("Release.gpg"), "Release.gpg", - MetaIndexURI("Release"), MetaIndexInfo("Release"), "Release", - ComputeIndexTargets(), - new indexRecords (Dist)); + if (tryInRelease == true) + new pkgAcqMetaClearSig(Owner, MetaIndexURI("InRelease"), + MetaIndexInfo("InRelease"), "InRelease", + MetaIndexURI("Release"), MetaIndexInfo("Release"), "Release", + MetaIndexURI("Release.gpg"), MetaIndexInfo("Release.gpg"), "Release.gpg", + ComputeIndexTargets(), + new indexRecords (Dist)); + else + new pkgAcqMetaSig(Owner, MetaIndexURI("Release.gpg"), + MetaIndexInfo("Release.gpg"), "Release.gpg", + MetaIndexURI("Release"), MetaIndexInfo("Release"), "Release", + ComputeIndexTargets(), + new indexRecords (Dist)); return true; } diff --git a/debian/changelog b/debian/changelog index 77e8674a0..7ebbb1cb4 100644 --- a/debian/changelog +++ b/debian/changelog @@ -7,9 +7,16 @@ apt (0.9.7.9) UNRELEASED; urgency=low - ExecGPGV is a method which should never return, so mark it as such and fix the inconsistency of returning in error cases - don't close stdout/stderr if it is also the statusfd + - if ExecGPGV deals with a clear-signed file it will split this file + into data and signatures, pass it to gpgv for verification and + recombines it after that in a known-good way without unsigned blocks + and whitespaces resulting usually in more or less the same file as + before, but later code can be sure about the format * apt-pkg/acquire-item.cc: - keep the last good InRelease file around just as we do it with Release.gpg in case the new one we download isn't good for us + * apt-pkg/deb/debmetaindex.cc: + - reenable InRelease by default -- David Kalnischkies Fri, 15 Mar 2013 14:15:43 +0100 diff --git a/test/integration/test-ubuntu-bug-784473-InRelease-one-message-only b/test/integration/test-ubuntu-bug-784473-InRelease-one-message-only index d97011914..fad5488fb 100755 --- a/test/integration/test-ubuntu-bug-784473-InRelease-one-message-only +++ b/test/integration/test-ubuntu-bug-784473-InRelease-one-message-only @@ -26,6 +26,14 @@ MD5Sum: 2182897e0a2a0c09e760beaae117a015 2023 Packages.diff/Index 1b895931853981ad8204d2439821b999 4144 Packages.gz'; echo; cat ${RELEASE}.old;) > ${RELEASE} done -aptget update -qq > /dev/null 2> starts-with-unsigned.msg -sed -i 's#File .*InRelease#File InRelease#' starts-with-unsigned.msg -testfileequal starts-with-unsigned.msg "W: GPG error: file: unstable InRelease: File InRelease doesn't start with a clearsigned message" + +msgtest 'The unsigned garbage before signed block is' 'ignored' +aptget update -qq > /dev/null 2>&1 && msgpass || msgfail + +ROOTDIR="$(readlink -f .)" +testequal "Package files: + 100 ${ROOTDIR}/rootdir/var/lib/dpkg/status + release a=now + 500 file:${ROOTDIR}/aptarchive/ unstable/main i386 Packages + release a=unstable,n=unstable,c=main +Pinned packages:" aptcache policy -- cgit v1.2.3 From ad000f6b68f9216412a6a70bcfe6cb11fb0c2fe6 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Sat, 16 Mar 2013 10:08:28 +0100 Subject: add testcase and update changelog --- debian/changelog | 5 ++ test/integration/test-inrelease-verification-fail | 80 +++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100755 test/integration/test-inrelease-verification-fail diff --git a/debian/changelog b/debian/changelog index 7ebbb1cb4..3ef652c56 100644 --- a/debian/changelog +++ b/debian/changelog @@ -17,6 +17,11 @@ apt (0.9.7.9) UNRELEASED; urgency=low Release.gpg in case the new one we download isn't good for us * apt-pkg/deb/debmetaindex.cc: - reenable InRelease by default + + [ Michael Vogt ] + * add regression test for CVE-2013-1051 + * implement GPGSplit() based on the idea from Ansgar Burchardt + (many thanks!) -- David Kalnischkies Fri, 15 Mar 2013 14:15:43 +0100 diff --git a/test/integration/test-inrelease-verification-fail b/test/integration/test-inrelease-verification-fail new file mode 100755 index 000000000..5cbf1ab4d --- /dev/null +++ b/test/integration/test-inrelease-verification-fail @@ -0,0 +1,80 @@ +#!/bin/sh + +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework + +setupenvironment +configarchitecture "i386" + +buildsimplenativepackage 'good-pkg' 'all' '1.0' 'stable' + +setupaptarchive + +# now exchange to the Packages file, note that this could be +# done via MITM too +cat > aptarchive/dists/stable/main/binary-i386/Packages < aptarchive/dists/stable/main/binary-i386/Packages.$extension +done + +# add a space into the BEGIN PGP SIGNATURE PART/END PGP SIGNATURE part +# to trick apt - this is still legal to gpg(v) +sed -i '/^-----BEGIN PGP SIGNATURE-----/,/^-----END PGP SIGNATURE-----/ s/^$/ /g' aptarchive/dists/stable/InRelease + +# and append our own hashes for the modified Packages files +cat >> aptarchive/dists/stable/InRelease <> aptarchive/dists/stable/InRelease + # Sources + s="$(sha512sum aptarchive/dists/stable/main/source/Sources$comp | cut -f1 -d' ') $(stat -c %s aptarchive/dists/stable/main/source/Sources$comp) main/source/Sources$comp" + echo " $s" >> aptarchive/dists/stable/InRelease +done; + +# deliver this +changetowebserver + +# ensure the update fails +# useful for debugging to add "-o Debug::pkgAcquire::auth=true" +if aptget update -qq; then + msgfail "apt-get update should NOT work for MITM" + exit 1 +fi + +# ensure there is no package +testequal 'Reading package lists... +Building dependency tree... +E: Unable to locate package bad-mitm' aptget install bad-mitm + +# and verify that its not picked up +#testequal 'N: Unable to locate package bad-mitm' aptcache policy bad-mitm + +# and that the right one is used +#testequal 'good-pkg: +#+ Installed: (none) +#+ Candidate: 1.0 +#+ Version table: +#+ 1.0 0 +#+ 500 http://localhost/ stable/main i386 Packages' aptcache policy good-pkg -- cgit v1.2.3 From bea263c2c0ecd6715ce996fd9b54599ac2cfa7c2 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 16 Mar 2013 12:40:43 +0100 Subject: ensure that we fclose/unlink/free in the new gpg-code as soon as possible --- apt-pkg/contrib/gpgv.cc | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index c236a7289..5921d7c67 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -214,19 +214,25 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, ioprintf(std::cerr, _("Waited for %s but it wasn't there"), "gpgv"); UNLINK_EXIT(EINTERNAL); } +#undef UNLINK_EXIT + // we don't need the files any longer as we have the filedescriptors still open + unlink(sig); + unlink(data); + free(sig); + free(data); // check if it exit'ed normally … if (WIFEXITED(Status) == false) { ioprintf(std::cerr, _("Sub-process %s exited unexpectedly"), "gpgv"); - UNLINK_EXIT(EINTERNAL); + exit(EINTERNAL); } // … and with a good exit code if (WEXITSTATUS(Status) != 0) { ioprintf(std::cerr, _("Sub-process %s returned an error code (%u)"), "gpgv", WEXITSTATUS(Status)); - UNLINK_EXIT(WEXITSTATUS(Status)); + exit(WEXITSTATUS(Status)); } /* looks like its fine. Our caller will check the status fd, @@ -236,12 +242,11 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, if (RecombineToClearSignedFile(File, dataFd, dataHeader, sigFd) == false) { _error->DumpErrors(std::cerr); - UNLINK_EXIT(EINTERNAL); + exit(EINTERNAL); } // everything fine, we have a clean file now! - UNLINK_EXIT(0); -#undef UNLINK_EXIT + exit(0); } exit(EINTERNAL); // unreachable safe-guard } @@ -259,7 +264,10 @@ bool RecombineToClearSignedFile(std::string const &OutFile, int const ContentFil FILE *data_file = fdopen(ContentFile, "r"); FILE *sig_file = fdopen(SignatureFile, "r"); if (data_file == NULL || sig_file == NULL) + { + fclose(clean_file); return _error->Error("Couldn't open splitfiles to recombine them into %s", OutFile.c_str()); + } char *buf = NULL; size_t buf_size = 0; while (getline(&buf, &buf_size, data_file) != -1) @@ -287,13 +295,21 @@ bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, { out_content = fdopen(ContentFile, "w"); if (out_content == NULL) + { + fclose(in); return _error->Errno("fdopen", "Failed to open file to write content to from %s", InFile.c_str()); + } } if (SignatureFile != -1) { out_signature = fdopen(SignatureFile, "w"); if (out_signature == NULL) + { + fclose(in); + if (out_content != NULL) + fclose(out_content); return _error->Errno("fdopen", "Failed to open file to write signature to from %s", InFile.c_str()); + } } bool found_message_start = false; @@ -358,6 +374,7 @@ bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, fclose(out_content); if (out_signature != NULL) fclose(out_signature); + fclose(in); return true; } -- cgit v1.2.3 From 34747d46be3a15105d896266d8043f55d04e7735 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 18 Mar 2013 17:06:51 +0100 Subject: rename testcase to mention CVE number, make the code more consistent with the rest and add some more tests (by fixing commented ones) --- .../test-cve-2013-1051-InRelease-parsing | 61 +++++++++++++++++ test/integration/test-inrelease-verification-fail | 80 ---------------------- 2 files changed, 61 insertions(+), 80 deletions(-) create mode 100755 test/integration/test-cve-2013-1051-InRelease-parsing delete mode 100755 test/integration/test-inrelease-verification-fail diff --git a/test/integration/test-cve-2013-1051-InRelease-parsing b/test/integration/test-cve-2013-1051-InRelease-parsing new file mode 100755 index 000000000..bd68fccf6 --- /dev/null +++ b/test/integration/test-cve-2013-1051-InRelease-parsing @@ -0,0 +1,61 @@ +#!/bin/sh +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework + +setupenvironment +configarchitecture 'i386' + +insertpackage 'stable' 'good-pkg' 'all' '1.0' + +setupaptarchive + +changetowebserver +ARCHIVE='http://localhost/' +msgtest 'Initial apt-get update should work with' 'InRelease' +aptget update -qq && msgpass || msgfail + +# check that the setup is correct +testequal "good-pkg: + Installed: (none) + Candidate: 1.0 + Version table: + 1.0 0 + 500 ${ARCHIVE} stable/main i386 Packages" aptcache policy good-pkg + +# now exchange to the Packages file, note that this could be +# done via MITM too +insertpackage 'stable' 'bad-mitm' 'all' '1.0' + +# this builds compressed files and a new (unsigned) Release +buildaptarchivefromfiles '+1hour' + +# add a space into the BEGIN PGP SIGNATURE PART/END PGP SIGNATURE part +# to trick apt - this is still legal to gpg(v) +sed -i '/^-----BEGIN PGP SIGNATURE-----/,/^-----END PGP SIGNATURE-----/ s/^$/ /g' aptarchive/dists/stable/InRelease + +# we append the (evil unsigned) Release file to the (good signed) InRelease +cat aptarchive/dists/stable/Release >> aptarchive/dists/stable/InRelease + + +# ensure the update fails +# useful for debugging to add "-o Debug::pkgAcquire::auth=true" +msgtest 'apt-get update for should fail with the modified' 'InRelease' +aptget update 2>&1 | grep -q 'Hash Sum mismatch' > /dev/null && msgpass || msgfail + +# ensure there is no package +testequal 'Reading package lists... +Building dependency tree... +E: Unable to locate package bad-mitm' aptget install bad-mitm -s + +# and verify that its not picked up +testequal 'N: Unable to locate package bad-mitm' aptcache policy bad-mitm -q=0 + +# and that the right one is used +testequal "good-pkg: + Installed: (none) + Candidate: 1.0 + Version table: + 1.0 0 + 500 ${ARCHIVE} stable/main i386 Packages" aptcache policy good-pkg diff --git a/test/integration/test-inrelease-verification-fail b/test/integration/test-inrelease-verification-fail deleted file mode 100755 index 5cbf1ab4d..000000000 --- a/test/integration/test-inrelease-verification-fail +++ /dev/null @@ -1,80 +0,0 @@ -#!/bin/sh - -set -e - -TESTDIR=$(readlink -f $(dirname $0)) -. $TESTDIR/framework - -setupenvironment -configarchitecture "i386" - -buildsimplenativepackage 'good-pkg' 'all' '1.0' 'stable' - -setupaptarchive - -# now exchange to the Packages file, note that this could be -# done via MITM too -cat > aptarchive/dists/stable/main/binary-i386/Packages < aptarchive/dists/stable/main/binary-i386/Packages.$extension -done - -# add a space into the BEGIN PGP SIGNATURE PART/END PGP SIGNATURE part -# to trick apt - this is still legal to gpg(v) -sed -i '/^-----BEGIN PGP SIGNATURE-----/,/^-----END PGP SIGNATURE-----/ s/^$/ /g' aptarchive/dists/stable/InRelease - -# and append our own hashes for the modified Packages files -cat >> aptarchive/dists/stable/InRelease <> aptarchive/dists/stable/InRelease - # Sources - s="$(sha512sum aptarchive/dists/stable/main/source/Sources$comp | cut -f1 -d' ') $(stat -c %s aptarchive/dists/stable/main/source/Sources$comp) main/source/Sources$comp" - echo " $s" >> aptarchive/dists/stable/InRelease -done; - -# deliver this -changetowebserver - -# ensure the update fails -# useful for debugging to add "-o Debug::pkgAcquire::auth=true" -if aptget update -qq; then - msgfail "apt-get update should NOT work for MITM" - exit 1 -fi - -# ensure there is no package -testequal 'Reading package lists... -Building dependency tree... -E: Unable to locate package bad-mitm' aptget install bad-mitm - -# and verify that its not picked up -#testequal 'N: Unable to locate package bad-mitm' aptcache policy bad-mitm - -# and that the right one is used -#testequal 'good-pkg: -#+ Installed: (none) -#+ Candidate: 1.0 -#+ Version table: -#+ 1.0 0 -#+ 500 http://localhost/ stable/main i386 Packages' aptcache policy good-pkg -- cgit v1.2.3 From f1828b6977972b4ef6da6401602b7938f6570c32 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 18 Mar 2013 19:36:55 +0100 Subject: - add method to open (maybe) clearsigned files transparently * ftparchive/writer.cc: - use OpenMaybeClearSignedFile to be free from detecting and skipping clearsigning metadata in dsc files --- apt-pkg/contrib/gpgv.cc | 55 +++++++++++++++++++++++- apt-pkg/contrib/gpgv.h | 22 ++++++++++ debian/changelog | 6 ++- ftparchive/writer.cc | 105 ++++++++++++++++++++------------------------- test/integration/framework | 10 ++++- 5 files changed, 135 insertions(+), 63 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 5921d7c67..fc16dd32c 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -19,7 +19,7 @@ #include /*}}}*/ -char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/ +static char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/ { const char *tmpdir = getenv("TMPDIR"); #ifdef P_tmpdir @@ -376,5 +376,58 @@ bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, fclose(out_signature); fclose(in); + 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 (found_message_start == false && found_message_end == false) + return false; + // otherwise one missing indicates a syntax error + else if (found_message_start == false || found_message_end == false) + return _error->Error("Splitting of file %s failed as it doesn't contain all expected parts", InFile.c_str()); + 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, thats enough for us + unlink(message); + free(message); + + int const duppedMsg = dup(messageFd); + if (duppedMsg == -1) + return _error->Errno("dup", "Couldn't duplicate FD to work with %s", ClearSignedFileName.c_str()); + + _error->PushToStack(); + bool const splitDone = SplitClearSignedFile(ClearSignedFileName.c_str(), messageFd, NULL, -1); + bool const errorDone = _error->PendingError(); + _error->MergeWithStack(); + if (splitDone == false) + { + close(duppedMsg); + + if (errorDone == true) + return false; + + // we deal with an unsigned file + MessageFile.Open(ClearSignedFileName, FileFd::ReadOnly); + } + else // clear-signed + { + if (lseek(duppedMsg, 0, SEEK_SET) < 0) + return _error->Errno("lseek", "Unable to seek back in message fd for file %s", ClearSignedFileName.c_str()); + MessageFile.OpenDescriptor(duppedMsg, FileFd::ReadOnly, true); + } + + return MessageFile.Failed() == false; +} + /*}}}*/ diff --git a/apt-pkg/contrib/gpgv.h b/apt-pkg/contrib/gpgv.h index 8e04855e4..ab7d35ab1 100644 --- a/apt-pkg/contrib/gpgv.h +++ b/apt-pkg/contrib/gpgv.h @@ -12,6 +12,8 @@ #include #include +#include + #if __GNUC__ >= 4 #define APT_noreturn __attribute__ ((noreturn)) #else @@ -52,10 +54,17 @@ inline void ExecGPGV(std::string const &File, std::string const &FileSig, * The code doesn't support dash-encoded lines as these are not * expected to be present in files we have to deal with. * + * The content of the split files is undefined if the splitting was + * unsuccessful. + * + * Note that trying to split an unsigned file will fail, but + * not generate an error message. + * * @param InFile is the clear-signed file * @param ContentFile is the Fd the message will be written to * @param ContentHeader is a list of all required Amored Headers for the message * @param SignatureFile is the Fd all signatures will be written to + * @return true if the splitting was successful, false otherwise */ bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, std::vector * const ContentHeader, int const SignatureFile); @@ -74,4 +83,17 @@ bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, bool RecombineToClearSignedFile(std::string const &OutFile, int const ContentFile, std::vector const &ContentHeader, int const SignatureFile); +/** \brief open a file which might be clear-signed + * + * This method tries to extract the (signed) message of a file. + * If the file isn't signed it will just open the given filename. + * Otherwise the message is extracted to a temporary file which + * will be opened instead. + * + * @param ClearSignedFileName is the name of the file to open + * @param[out] MessageFile is the FileFd in which the file will be opened + * @return true if opening was successful, otherwise false + */ +bool OpenMaybeClearSignedFile(std::string const &ClearSignedFileName, FileFd &MessageFile); + #endif diff --git a/debian/changelog b/debian/changelog index 3ef652c56..27fae657c 100644 --- a/debian/changelog +++ b/debian/changelog @@ -12,12 +12,16 @@ apt (0.9.7.9) UNRELEASED; urgency=low recombines it after that in a known-good way without unsigned blocks and whitespaces resulting usually in more or less the same file as before, but later code can be sure about the format + - add method to open (maybe) clearsigned files transparently * apt-pkg/acquire-item.cc: - keep the last good InRelease file around just as we do it with Release.gpg in case the new one we download isn't good for us * apt-pkg/deb/debmetaindex.cc: - reenable InRelease by default - + * ftparchive/writer.cc: + - use OpenMaybeClearSignedFile to be free from detecting and + skipping clearsigning metadata in dsc files + [ Michael Vogt ] * add regression test for CVE-2013-1051 * implement GPGSplit() based on the idea from Ansgar Burchardt diff --git a/ftparchive/writer.cc b/ftparchive/writer.cc index 3065526ad..d26b160f9 100644 --- a/ftparchive/writer.cc +++ b/ftparchive/writer.cc @@ -20,6 +20,8 @@ #include #include #include +#include +#include #include #include @@ -598,77 +600,62 @@ SourcesWriter::SourcesWriter(string const &BOverrides,string const &SOverrides, // --------------------------------------------------------------------- /* */ bool SourcesWriter::DoPackage(string FileName) -{ +{ // Open the archive - FileFd F(FileName,FileFd::ReadOnly); - if (_error->PendingError() == true) + FileFd F; + if (OpenMaybeClearSignedFile(FileName, F) == false) return false; - - // Stat the file for later - struct stat St; - if (fstat(F.Fd(),&St) != 0) - return _error->Errno("fstat","Failed to stat %s",FileName.c_str()); - if (St.st_size > 128*1024) + unsigned long long const FSize = F.FileSize(); + //FIXME: do we really need to enforce a maximum size of the dsc file? + if (FSize > 128*1024) return _error->Error("DSC file '%s' is too large!",FileName.c_str()); - - if (BufSize < (unsigned long long)St.st_size+1) + + if (BufSize < FSize + 2) { - BufSize = St.st_size+1; - Buffer = (char *)realloc(Buffer,St.st_size+1); + BufSize = FSize + 2; + Buffer = (char *)realloc(Buffer , BufSize); } - - if (F.Read(Buffer,St.st_size) == false) + + if (F.Read(Buffer, FSize) == false) return false; + // Stat the file for later (F might be clearsigned, so not F.FileSize()) + struct stat St; + if (stat(FileName.c_str(), &St) != 0) + return _error->Errno("fstat","Failed to stat %s",FileName.c_str()); + // Hash the file char *Start = Buffer; - char *BlkEnd = Buffer + St.st_size; - - MD5Summation MD5; - SHA1Summation SHA1; - SHA256Summation SHA256; - SHA256Summation SHA512; - - if (DoMD5 == true) - MD5.Add((unsigned char *)Start,BlkEnd - Start); - if (DoSHA1 == true) - SHA1.Add((unsigned char *)Start,BlkEnd - Start); - if (DoSHA256 == true) - SHA256.Add((unsigned char *)Start,BlkEnd - Start); - if (DoSHA512 == true) - SHA512.Add((unsigned char *)Start,BlkEnd - Start); + char *BlkEnd = Buffer + FSize; - // Add an extra \n to the end, just in case - *BlkEnd++ = '\n'; - - /* Remove the PGP trailer. Some .dsc's have this without a blank line - before */ - const char *Key = "-----BEGIN PGP SIGNATURE-----"; - for (char *MsgEnd = Start; MsgEnd < BlkEnd - strlen(Key) -1; MsgEnd++) + Hashes DscHashes; + if (FSize == (unsigned long long) St.st_size) { - if (*MsgEnd == '\n' && strncmp(MsgEnd+1,Key,strlen(Key)) == 0) - { - MsgEnd[1] = '\n'; - break; - } + if (DoMD5 == true) + DscHashes.MD5.Add((unsigned char *)Start,BlkEnd - Start); + if (DoSHA1 == true) + DscHashes.SHA1.Add((unsigned char *)Start,BlkEnd - Start); + if (DoSHA256 == true) + DscHashes.SHA256.Add((unsigned char *)Start,BlkEnd - Start); + if (DoSHA512 == true) + DscHashes.SHA512.Add((unsigned char *)Start,BlkEnd - Start); } - - /* Read records until we locate the Source record. This neatly skips the - GPG header (which is RFC822 formed) without any trouble. */ - pkgTagSection Tags; - do + else { - unsigned Pos; - if (Tags.Scan(Start,BlkEnd - Start) == false) - return _error->Error("Could not find a record in the DSC '%s'",FileName.c_str()); - if (Tags.Find("Source",Pos) == true) - break; - Start += Tags.size(); + FileFd DscFile(FileName, FileFd::ReadOnly); + DscHashes.AddFD(DscFile, St.st_size, DoMD5, DoSHA1, DoSHA256, DoSHA512); } - while (1); + + // Add extra \n to the end, just in case (as in clearsigned they are missing) + *BlkEnd++ = '\n'; + *BlkEnd++ = '\n'; + + pkgTagSection Tags; + if (Tags.Scan(Start,BlkEnd - Start) == false || Tags.Exists("Source") == false) + return _error->Error("Could not find a record in the DSC '%s'",FileName.c_str()); Tags.Trim(); - + // Lookup the overide information, finding first the best priority. string BestPrio; string Bins = Tags.FindS("Binary"); @@ -732,25 +719,25 @@ bool SourcesWriter::DoPackage(string FileName) string const strippedName = flNotDir(FileName); std::ostringstream ostreamFiles; if (DoMD5 == true && Tags.Exists("Files")) - ostreamFiles << "\n " << string(MD5.Result()) << " " << St.st_size << " " + ostreamFiles << "\n " << string(DscHashes.MD5.Result()) << " " << St.st_size << " " << strippedName << "\n " << Tags.FindS("Files"); string const Files = ostreamFiles.str(); std::ostringstream ostreamSha1; if (DoSHA1 == true && Tags.Exists("Checksums-Sha1")) - ostreamSha1 << "\n " << string(SHA1.Result()) << " " << St.st_size << " " + ostreamSha1 << "\n " << string(DscHashes.SHA1.Result()) << " " << St.st_size << " " << strippedName << "\n " << Tags.FindS("Checksums-Sha1"); string const ChecksumsSha1 = ostreamSha1.str(); std::ostringstream ostreamSha256; if (DoSHA256 == true && Tags.Exists("Checksums-Sha256")) - ostreamSha256 << "\n " << string(SHA256.Result()) << " " << St.st_size << " " + ostreamSha256 << "\n " << string(DscHashes.SHA256.Result()) << " " << St.st_size << " " << strippedName << "\n " << Tags.FindS("Checksums-Sha256"); string const ChecksumsSha256 = ostreamSha256.str(); std::ostringstream ostreamSha512; if (Tags.Exists("Checksums-Sha512")) - ostreamSha512 << "\n " << string(SHA512.Result()) << " " << St.st_size << " " + ostreamSha512 << "\n " << string(DscHashes.SHA512.Result()) << " " << St.st_size << " " << strippedName << "\n " << Tags.FindS("Checksums-Sha512"); string const ChecksumsSha512 = ostreamSha512.str(); diff --git a/test/integration/framework b/test/integration/framework index 1c4872c8e..2ef61ca84 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -328,9 +328,15 @@ Package: $NAME" >> ${BUILDDIR}/debian/control fi echo '3.0 (native)' > ${BUILDDIR}/debian/source/format - local SRCS="$( (cd ${BUILDDIR}/..; dpkg-source -b ${NAME}-${VERSION} 2>&1) | grep '^dpkg-source: info: building' | grep -o '[a-z0-9._+~-]*$')" - for SRC in $SRCS; do + (cd ${BUILDDIR}/..; dpkg-source -b ${NAME}-${VERSION} 2>&1) | sed -n 's#^dpkg-source: info: building [^ ]\+ in ##p' \ + | while read SRC; do echo "pool/${SRC}" >> ${BUILDDIR}/../${RELEASE}.${DISTSECTION}.srclist +# if expr match "${SRC}" '.*\.dsc' >/dev/null 2>&1; then +# gpg --yes --no-default-keyring --secret-keyring ./keys/joesixpack.sec \ +# --keyring ./keys/joesixpack.pub --default-key 'Joe Sixpack' \ +# --clearsign -o "${BUILDDIR}/../${SRC}.sign" "${BUILDDIR}/../$SRC" +# mv "${BUILDDIR}/../${SRC}.sign" "${BUILDDIR}/../$SRC" +# fi done for arch in $(echo "$ARCH" | sed -e 's#,#\n#g' | sed -e "s#^native\$#$(getarchitecture 'native')#"); do -- cgit v1.2.3 From 233b78083f6f79730fcb5a6faeb74e2a78b6038a Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 18 Mar 2013 22:57:08 +0100 Subject: * apt-pkg/deb/debindexfile.cc, apt-pkg/deb/deblistparser.cc: - use OpenMaybeClearSignedFile to be free from detecting and skipping clearsigning metadata in dsc and Release files We can't write a "clean" file to disk as not all acquire methods copy Release files before checking them (e.g. cdrom), so this reverts recombining, but uses the method we use for dsc files also in the two places we deal with Release files --- apt-pkg/contrib/gpgv.cc | 44 ++--------------------------------------- apt-pkg/contrib/gpgv.h | 14 ------------- apt-pkg/deb/debindexfile.cc | 8 +++++++- apt-pkg/deb/deblistparser.cc | 12 +---------- apt-pkg/indexrecords.cc | 6 +++++- debian/changelog | 11 +++++------ test/integration/framework | 2 +- test/integration/test-apt-cdrom | 2 ++ 8 files changed, 23 insertions(+), 76 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index fc16dd32c..7e244c623 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -215,7 +215,7 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, UNLINK_EXIT(EINTERNAL); } #undef UNLINK_EXIT - // we don't need the files any longer as we have the filedescriptors still open + // we don't need the files any longer unlink(sig); unlink(data); free(sig); @@ -235,52 +235,12 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, exit(WEXITSTATUS(Status)); } - /* looks like its fine. Our caller will check the status fd, - but we construct a good-known clear-signed file without garbage - and other non-sense. In a perfect world, we get the same file, - but empty lines, trailing whitespaces and stuff makes it inperfect … */ - if (RecombineToClearSignedFile(File, dataFd, dataHeader, sigFd) == false) - { - _error->DumpErrors(std::cerr); - exit(EINTERNAL); - } - - // everything fine, we have a clean file now! + // everything fine exit(0); } exit(EINTERNAL); // unreachable safe-guard } /*}}}*/ -// RecombineToClearSignedFile - combine data/signature to message /*{{{*/ -bool RecombineToClearSignedFile(std::string const &OutFile, int const ContentFile, - std::vector const &ContentHeader, int const SignatureFile) -{ - FILE *clean_file = fopen(OutFile.c_str(), "w"); - fputs("-----BEGIN PGP SIGNED MESSAGE-----\n", clean_file); - for (std::vector::const_iterator h = ContentHeader.begin(); h != ContentHeader.end(); ++h) - fprintf(clean_file, "%s\n", h->c_str()); - fputs("\n", clean_file); - - FILE *data_file = fdopen(ContentFile, "r"); - FILE *sig_file = fdopen(SignatureFile, "r"); - if (data_file == NULL || sig_file == NULL) - { - fclose(clean_file); - return _error->Error("Couldn't open splitfiles to recombine them into %s", OutFile.c_str()); - } - char *buf = NULL; - size_t buf_size = 0; - while (getline(&buf, &buf_size, data_file) != -1) - fputs(buf, clean_file); - fclose(data_file); - fputs("\n", clean_file); - while (getline(&buf, &buf_size, sig_file) != -1) - fputs(buf, clean_file); - fclose(sig_file); - fclose(clean_file); - return true; -} - /*}}}*/ // SplitClearSignedFile - split message into data/signature /*{{{*/ bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, std::vector * const ContentHeader, int const SignatureFile) diff --git a/apt-pkg/contrib/gpgv.h b/apt-pkg/contrib/gpgv.h index ab7d35ab1..8cbe553bc 100644 --- a/apt-pkg/contrib/gpgv.h +++ b/apt-pkg/contrib/gpgv.h @@ -69,20 +69,6 @@ inline void ExecGPGV(std::string const &File, std::string const &FileSig, bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, std::vector * const ContentHeader, int const SignatureFile); -/** \brief recombines message and signature to an inline signature - * - * Reverses the splitting down by #SplitClearSignedFile by writing - * a well-formed clear-signed message without unsigned messages, - * additional signed messages or just trailing whitespaces - * - * @param OutFile will be clear-signed file - * @param ContentFile is the Fd the message will be read from - * @param ContentHeader is a list of all required Amored Headers for the message - * @param SignatureFile is the Fd all signatures will be read from - */ -bool RecombineToClearSignedFile(std::string const &OutFile, int const ContentFile, - std::vector const &ContentHeader, int const SignatureFile); - /** \brief open a file which might be clear-signed * * This method tries to extract the (signed) message of a file. diff --git a/apt-pkg/deb/debindexfile.cc b/apt-pkg/deb/debindexfile.cc index de645bb6e..909dfcf47 100644 --- a/apt-pkg/deb/debindexfile.cc +++ b/apt-pkg/deb/debindexfile.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include /*}}}*/ @@ -337,7 +338,12 @@ bool debPackagesIndex::Merge(pkgCacheGenerator &Gen,OpProgress *Prog) const if (releaseExists == true || FileExists(ReleaseFile) == true) { - FileFd Rel(ReleaseFile,FileFd::ReadOnly); + FileFd Rel; + // Beware: The 'Release' file might be clearsigned in case the + // signature for an 'InRelease' file couldn't be checked + if (OpenMaybeClearSignedFile(ReleaseFile, Rel) == false) + return false; + if (_error->PendingError() == true) return false; Parser.LoadReleaseInfo(File,Rel,Section); diff --git a/apt-pkg/deb/deblistparser.cc b/apt-pkg/deb/deblistparser.cc index b84bd6fdd..2c014a734 100644 --- a/apt-pkg/deb/deblistparser.cc +++ b/apt-pkg/deb/deblistparser.cc @@ -800,13 +800,12 @@ bool debListParser::LoadReleaseInfo(pkgCache::PkgFileIterator &FileI, map_ptrloc const storage = WriteUniqString(component); FileI->Component = storage; - // FIXME: Code depends on the fact that Release files aren't compressed + // FIXME: should use FileFd and TagSection FILE* release = fdopen(dup(File.Fd()), "r"); if (release == NULL) return false; char buffer[101]; - bool gpgClose = false; while (fgets(buffer, sizeof(buffer), release) != NULL) { size_t len = 0; @@ -818,15 +817,6 @@ bool debListParser::LoadReleaseInfo(pkgCache::PkgFileIterator &FileI, if (buffer[len] == '\0') continue; - // only evalute the first GPG section - if (strncmp("-----", buffer, 5) == 0) - { - if (gpgClose == true) - break; - gpgClose = true; - continue; - } - // seperate the tag from the data const char* dataStart = strchr(buffer + len, ':'); if (dataStart == NULL) diff --git a/apt-pkg/indexrecords.cc b/apt-pkg/indexrecords.cc index af2639beb..1461a24bb 100644 --- a/apt-pkg/indexrecords.cc +++ b/apt-pkg/indexrecords.cc @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -57,7 +58,10 @@ bool indexRecords::Exists(string const &MetaKey) const bool indexRecords::Load(const string Filename) /*{{{*/ { - FileFd Fd(Filename, FileFd::ReadOnly); + FileFd Fd; + if (OpenMaybeClearSignedFile(Filename, Fd) == false) + return false; + pkgTagFile TagFile(&Fd, Fd.Size() + 256); // XXX if (_error->PendingError() == true) { diff --git a/debian/changelog b/debian/changelog index 27fae657c..7c02b2689 100644 --- a/debian/changelog +++ b/debian/changelog @@ -8,19 +8,18 @@ apt (0.9.7.9) UNRELEASED; urgency=low and fix the inconsistency of returning in error cases - don't close stdout/stderr if it is also the statusfd - if ExecGPGV deals with a clear-signed file it will split this file - into data and signatures, pass it to gpgv for verification and - recombines it after that in a known-good way without unsigned blocks - and whitespaces resulting usually in more or less the same file as - before, but later code can be sure about the format + into data and signatures, pass it to gpgv for verification - add method to open (maybe) clearsigned files transparently * apt-pkg/acquire-item.cc: - keep the last good InRelease file around just as we do it with Release.gpg in case the new one we download isn't good for us * apt-pkg/deb/debmetaindex.cc: - reenable InRelease by default - * ftparchive/writer.cc: + * ftparchive/writer.cc, + apt-pkg/deb/debindexfile.cc, + apt-pkg/deb/deblistparser.cc: - use OpenMaybeClearSignedFile to be free from detecting and - skipping clearsigning metadata in dsc files + skipping clearsigning metadata in dsc and Release files [ Michael Vogt ] * add regression test for CVE-2013-1051 diff --git a/test/integration/framework b/test/integration/framework index 2ef61ca84..86e6ed7c3 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -114,7 +114,7 @@ gdb() { } addtrap() { - CURRENTTRAP="$CURRENTTRAP $1" + CURRENTTRAP="$1 $CURRENTTRAP" trap "$CURRENTTRAP exit;" 0 HUP INT QUIT ILL ABRT FPE SEGV PIPE TERM } diff --git a/test/integration/test-apt-cdrom b/test/integration/test-apt-cdrom index f24c99b36..f1c4fd9d3 100755 --- a/test/integration/test-apt-cdrom +++ b/test/integration/test-apt-cdrom @@ -24,6 +24,8 @@ cat Translation-de | xz --format=lzma > Translation-de.lzma cat Translation-de | xz > Translation-de.xz rm Translation-en Translation-de cd - > /dev/null +addtrap "chmod -R +w $PWD/rootdir/media/cdrom/dists/;" +chmod -R -w rootdir/media/cdrom/dists aptcdrom add -m -o quiet=1 > apt-cdrom.log 2>&1 sed -i -e '/^Using CD-ROM/ d' -e '/gpgv/ d' -e '/^Identifying/ d' -e '/Reading / d' apt-cdrom.log -- cgit v1.2.3 From b408e4ad0010b273dac0af7dc87ab61062d89e49 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 19 Mar 2013 10:49:57 +0100 Subject: use FileFd instead of int fds to tidy up the interface a bit --- apt-pkg/contrib/gpgv.cc | 103 ++++++++++++++++++++++-------------------------- apt-pkg/contrib/gpgv.h | 12 +++--- 2 files changed, 53 insertions(+), 62 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 7e244c623..54cc4c6d0 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -41,6 +41,7 @@ static char * GenerateTemporaryFileTemplate(const char *basename) /*{{{*/ Also, as gpgv has no options to enforce a certain reduced style of clear-signed files (=the complete content of the file is signed and the content isn't encoded) we do a divide and conquer approach here + and split up the clear-signed file in message and signature for gpgv */ void ExecGPGV(std::string const &File, std::string const &FileGPG, int const &statusfd, int fd[2]) @@ -108,8 +109,6 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } } - int sigFd = -1; - int dataFd = -1; std::vector dataHeader; char * sig = NULL; char * data = NULL; @@ -126,17 +125,29 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, data = GenerateTemporaryFileTemplate("apt.data"); if (sig == NULL || data == NULL) { + ioprintf(std::cerr, "Couldn't create tempfile names for splitting up %s", File.c_str()); + exit(EINTERNAL); + } + + int const sigFd = mkstemp(sig); + int const dataFd = mkstemp(data); + if (sigFd == -1 || dataFd == -1) + { + if (dataFd != -1) + unlink(sig); + if (sigFd != -1) + unlink(data); ioprintf(std::cerr, "Couldn't create tempfiles for splitting up %s", File.c_str()); exit(EINTERNAL); } - sigFd = mkstemp(sig); - dataFd = mkstemp(data); - int const duppedSigFd = dup(sigFd); - int const duppedDataFd = dup(dataFd); + FileFd signature; + signature.OpenDescriptor(sigFd, FileFd::WriteOnly, true); + FileFd message; + message.OpenDescriptor(dataFd, FileFd::WriteOnly, true); - if (dataFd == -1 || sigFd == -1 || duppedDataFd == -1 || duppedSigFd == -1 || - SplitClearSignedFile(File, duppedDataFd, &dataHeader, duppedSigFd) == false) + if (signature.Failed() == true || message.Failed() == true || + SplitClearSignedFile(File, &message, &dataHeader, &signature) == false) { if (dataFd != -1) unlink(sig); @@ -145,8 +156,6 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, ioprintf(std::cerr, "Splitting up %s into data and signature failed", File.c_str()); exit(EINTERNAL); } - lseek(dataFd, 0, SEEK_SET); - lseek(sigFd, 0, SEEK_SET); Args.push_back(sig); Args.push_back(data); } @@ -242,36 +251,13 @@ void ExecGPGV(std::string const &File, std::string const &FileGPG, } /*}}}*/ // SplitClearSignedFile - split message into data/signature /*{{{*/ -bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, - std::vector * const ContentHeader, int const SignatureFile) +bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, + std::vector * 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()); - FILE *out_content = NULL; - FILE *out_signature = NULL; - if (ContentFile != -1) - { - out_content = fdopen(ContentFile, "w"); - if (out_content == NULL) - { - fclose(in); - return _error->Errno("fdopen", "Failed to open file to write content to from %s", InFile.c_str()); - } - } - if (SignatureFile != -1) - { - out_signature = fdopen(SignatureFile, "w"); - if (out_signature == NULL) - { - fclose(in); - if (out_content != NULL) - fclose(out_content); - return _error->Errno("fdopen", "Failed to open file to write signature to from %s", InFile.c_str()); - } - } - bool found_message_start = false; bool found_message_end = false; bool skip_until_empty_line = false; @@ -280,7 +266,8 @@ bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, char *buf = NULL; size_t buf_size = 0; - while (getline(&buf, &buf_size, in) != -1) + ssize_t line_len = 0; + while ((line_len = getline(&buf, &buf_size, in)) != -1) { _strrstrip(buf); if (found_message_start == false) @@ -305,35 +292,40 @@ bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, { found_signature = true; found_message_end = true; - if (out_signature != NULL) - fprintf(out_signature, "%s\n", buf); + 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(first_line == true) // first line does not need a newline { - if (out_content != NULL) - fprintf(out_content, "%s", buf); + if (ContentFile != NULL) + ContentFile->Write(buf, strlen(buf)); first_line = false; } - else if (out_content != NULL) - fprintf(out_content, "\n%s", buf); + else if (ContentFile != NULL) + { + ContentFile->Write("\n", 1); + ContentFile->Write(buf, strlen(buf)); + } } } else if (found_signature == true) { - if (out_signature != NULL) - fprintf(out_signature, "%s\n", buf); + 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 } - if (out_content != NULL) - fclose(out_content); - if (out_signature != NULL) - fclose(out_signature); fclose(in); if (found_signature == true) @@ -363,17 +355,17 @@ bool OpenMaybeClearSignedFile(std::string const &ClearSignedFileName, FileFd &Me unlink(message); free(message); - int const duppedMsg = dup(messageFd); - if (duppedMsg == -1) - return _error->Errno("dup", "Couldn't duplicate FD to work with %s", ClearSignedFileName.c_str()); + MessageFile.OpenDescriptor(messageFd, FileFd::ReadWrite, true); + if (MessageFile.Failed() == true) + return _error->Error("Couldn't open temporary file to work with %s", ClearSignedFileName.c_str()); _error->PushToStack(); - bool const splitDone = SplitClearSignedFile(ClearSignedFileName.c_str(), messageFd, NULL, -1); + bool const splitDone = SplitClearSignedFile(ClearSignedFileName.c_str(), &MessageFile, NULL, NULL); bool const errorDone = _error->PendingError(); _error->MergeWithStack(); if (splitDone == false) { - close(duppedMsg); + MessageFile.Close(); if (errorDone == true) return false; @@ -383,9 +375,8 @@ bool OpenMaybeClearSignedFile(std::string const &ClearSignedFileName, FileFd &Me } else // clear-signed { - if (lseek(duppedMsg, 0, SEEK_SET) < 0) - return _error->Errno("lseek", "Unable to seek back in message fd for file %s", ClearSignedFileName.c_str()); - MessageFile.OpenDescriptor(duppedMsg, FileFd::ReadOnly, true); + if (MessageFile.Seek(0) == false) + return _error->Errno("lseek", "Unable to seek back in message for file %s", ClearSignedFileName.c_str()); } return MessageFile.Failed() == false; diff --git a/apt-pkg/contrib/gpgv.h b/apt-pkg/contrib/gpgv.h index 8cbe553bc..1f877fc2d 100644 --- a/apt-pkg/contrib/gpgv.h +++ b/apt-pkg/contrib/gpgv.h @@ -48,8 +48,8 @@ inline void ExecGPGV(std::string const &File, std::string const &FileSig, * whitespaces are discarded. The resulting files are suitable to * be checked with gpgv. * - * If one or all Fds are -1 they will not be used and the content - * which would have been written to them is discarded. + * If a FileFd pointers is NULL it will not be used and the content + * which would have been written to it is silently discarded. * * The code doesn't support dash-encoded lines as these are not * expected to be present in files we have to deal with. @@ -61,13 +61,13 @@ inline void ExecGPGV(std::string const &File, std::string const &FileSig, * not generate an error message. * * @param InFile is the clear-signed file - * @param ContentFile is the Fd the message will be written to + * @param ContentFile is the FileFd the message will be written to * @param ContentHeader is a list of all required Amored Headers for the message - * @param SignatureFile is the Fd all signatures will be written to + * @param SignatureFile is the FileFd all signatures will be written to * @return true if the splitting was successful, false otherwise */ -bool SplitClearSignedFile(std::string const &InFile, int const ContentFile, - std::vector * const ContentHeader, int const SignatureFile); +bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, + std::vector * const ContentHeader, FileFd * const SignatureFile); /** \brief open a file which might be clear-signed * -- cgit v1.2.3 From cb32348956441e33733e6bd8c2c572f19600dc25 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 19 Mar 2013 12:37:50 +0100 Subject: support dash-escaped text in clearsigned files as implementations are free to escape all lines (we have no lines in our files which need to be escaped as these would be invalid fieldnames) and while ExecGPGV would detect dash-escaped text as invalid (as its not expected in messages with detached signatures) it would be possible to "comment" lines in (signed) dsc files which are only parsed but not verified --- apt-pkg/contrib/gpgv.cc | 23 ++++++++++++----------- apt-pkg/contrib/gpgv.h | 3 --- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/apt-pkg/contrib/gpgv.cc b/apt-pkg/contrib/gpgv.cc index 54cc4c6d0..ba059dd87 100644 --- a/apt-pkg/contrib/gpgv.cc +++ b/apt-pkg/contrib/gpgv.cc @@ -298,20 +298,21 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, SignatureFile->Write("\n", 1); } } - else if (found_message_end == false) + else if (found_message_end == false) // we are in the message block { - // 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 - { - if (ContentFile != NULL) - ContentFile->Write(buf, strlen(buf)); first_line = false; - } else if (ContentFile != NULL) - { ContentFile->Write("\n", 1); - ContentFile->Write(buf, strlen(buf)); - } + else + continue; + if (ContentFile != NULL) + ContentFile->Write(dashfree, strlen(dashfree)); } } else if (found_signature == true) @@ -333,10 +334,10 @@ bool SplitClearSignedFile(std::string const &InFile, FileFd * const ContentFile, // 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 (found_message_start == false && found_message_end == false) + if (first_line == true && found_message_start == false && found_message_end == false) return false; // otherwise one missing indicates a syntax error - else if (found_message_start == false || found_message_end == false) + else if (first_line == false || found_message_start == false || found_message_end == false) return _error->Error("Splitting of file %s failed as it doesn't contain all expected parts", InFile.c_str()); return true; diff --git a/apt-pkg/contrib/gpgv.h b/apt-pkg/contrib/gpgv.h index 1f877fc2d..08b10a97a 100644 --- a/apt-pkg/contrib/gpgv.h +++ b/apt-pkg/contrib/gpgv.h @@ -51,9 +51,6 @@ inline void ExecGPGV(std::string const &File, std::string const &FileSig, * If a FileFd pointers is NULL it will not be used and the content * which would have been written to it is silently discarded. * - * The code doesn't support dash-encoded lines as these are not - * expected to be present in files we have to deal with. - * * The content of the split files is undefined if the splitting was * unsuccessful. * -- cgit v1.2.3