From 08b7761a251a36fa65cbe022a86c51d7f091a88d Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 22 Mar 2016 01:26:29 +0100 Subject: handle gpgv's weak-digests ERRSIG Our own gpgv method can declare a digest algorithm as untrusted and handles these as worthless signatures. If gpgv comes with inbuilt untrusted (which is called weak in official terminology) which it e.g. does for MD5 in recent versions we should handle it in the same way. To check this we use the most uncommon still fully trusted hash as a configureable one via a hidden config option to toggle through all of the three states a hash can be in. --- methods/gpgv.cc | 57 +++++++++++++++++++++--- test/integration/test-releasefile-verification | 61 +++++++++++++++++++------- 2 files changed, 95 insertions(+), 23 deletions(-) diff --git a/methods/gpgv.cc b/methods/gpgv.cc index b6e0fa7bd..43f1df878 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -32,6 +32,7 @@ using std::vector; #define GNUPGPREFIX "[GNUPG:]" #define GNUPGBADSIG "[GNUPG:] BADSIG" +#define GNUPGERRSIG "[GNUPG:] ERRSIG" #define GNUPGNOPUBKEY "[GNUPG:] NO_PUBKEY" #define GNUPGVALIDSIG "[GNUPG:] VALIDSIG" #define GNUPGGOODSIG "[GNUPG:] GOODSIG" @@ -44,8 +45,20 @@ struct Digest { Untrusted, Weak, Trusted, + Configureable } state; char name[32]; + + State getState() const { + if (state != Digest::State::Configureable) + return state; + std::string const digestconfig = _config->Find("Debug::Acquire::gpgv::configdigest::truststate", "trusted"); + if (digestconfig == "weak") + return State::Weak; + else if (digestconfig == "untrusted") + return State::Untrusted; + return State::Trusted; + } }; static constexpr Digest Digests[] = { @@ -60,8 +73,9 @@ static constexpr Digest Digests[] = { {Digest::State::Trusted, "SHA256"}, {Digest::State::Trusted, "SHA384"}, {Digest::State::Trusted, "SHA512"}, - {Digest::State::Trusted, "SHA224"}, + {Digest::State::Configureable, "SHA224"}, }; +static_assert(Digests[_count(Digests) - 1].state == Digest::State::Configureable, "the last digest algo isn't the configurable one which we expect for tests"); static Digest FindDigest(std::string const & Digest) { @@ -77,6 +91,10 @@ struct Signer { std::string key; std::string note; }; +static bool IsTheSameKey(std::string const &validsig, std::string const &goodsig) { + // VALIDSIG reports a keyid (40 = 24 + 16), GOODSIG is a longid (16) only + return validsig.compare(24, 16, goodsig, strlen("GOODSIG "), 16) == 0; +} class GPGVMethod : public aptMethod { @@ -91,7 +109,6 @@ class GPGVMethod : public aptMethod protected: virtual bool URIAcquire(std::string const &Message, FetchItem *Itm) APT_OVERRIDE; public: - GPGVMethod() : aptMethod("gpgv","1.0",SingleInstance | SendConfig) {}; }; @@ -125,6 +142,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, // Loop over the output of apt-key (which really is gnupg), and check the signatures. std::vector ValidSigners; + std::vector ErrSigners; size_t buffersize = 0; char *buffer = NULL; while (1) @@ -144,11 +162,19 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, std::clog << "Got BADSIG! " << std::endl; BadSigners.push_back(string(buffer+sizeof(GNUPGPREFIX))); } + else if (strncmp(buffer, GNUPGERRSIG, sizeof(GNUPGERRSIG)-1) == 0) + { + if (Debug == true) + std::clog << "Got ERRSIG " << std::endl; + ErrSigners.push_back(string(buffer, strlen(GNUPGPREFIX), strlen("ERRSIG ") + 16)); + } else if (strncmp(buffer, GNUPGNOPUBKEY, sizeof(GNUPGNOPUBKEY)-1) == 0) { if (Debug == true) std::clog << "Got NO_PUBKEY " << std::endl; NoPubKeySigners.push_back(string(buffer+sizeof(GNUPGPREFIX))); + ErrSigners.erase(std::remove_if(ErrSigners.begin(), ErrSigners.end(), [&](std::string const &errsig) { + return errsig.compare(strlen("ERRSIG "), 16, buffer, strlen(GNUPGNOPUBKEY), 16) == 0; }), ErrSigners.end()); } else if (strncmp(buffer, GNUPGNODATA, sizeof(GNUPGBADSIG)-1) == 0) { @@ -191,7 +217,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, *p = 0; // Reject weak digest algorithms Digest digest = FindDigest(tokens[7]); - switch (digest.state) { + switch (digest.getState()) { case Digest::State::Weak: // Treat them like an expired key: For that a message about expiry // is emitted, a VALIDSIG, but no GOODSIG. @@ -203,10 +229,12 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, // Treat them like an expired key: For that a message about expiry // is emitted, a VALIDSIG, but no GOODSIG. WorthlessSigners.push_back(string(sig)); - GoodSigners.erase(std::remove(GoodSigners.begin(), GoodSigners.end(), string(sig))); + GoodSigners.erase(std::remove_if(GoodSigners.begin(), GoodSigners.end(), [&](std::string const &goodsig) { + return IsTheSameKey(string(sig), goodsig); }), GoodSigners.end()); if (Debug == true) std::clog << "Got untrusted VALIDSIG, key ID: " << sig << std::endl; break; + case Digest::State::Configureable: case Digest::State::Trusted: if (Debug == true) std::clog << "Got trusted VALIDSIG, key ID: " << sig << std::endl; @@ -218,6 +246,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, } fclose(pipein); free(buffer); + std::move(ErrSigners.begin(), ErrSigners.end(), std::back_inserter(WorthlessSigners)); // apt-key has a --keyid parameter, but this requires gpg, so we call it without it // and instead check after the fact which keyids where used for verification @@ -252,7 +281,22 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, { ioprintf(std::clog, "gpgv exited with status %i\n", WEXITSTATUS(status)); } - + + if (Debug) + { + std::cerr << "Summary:" << std::endl << " Good: "; + std::copy(GoodSigners.begin(), GoodSigners.end(), std::ostream_iterator(std::cerr, ", ")); + std::cerr << std::endl << " Bad: "; + std::copy(BadSigners.begin(), BadSigners.end(), std::ostream_iterator(std::cerr, ", ")); + std::cerr << std::endl << " Worthless: "; + std::copy(WorthlessSigners.begin(), WorthlessSigners.end(), std::ostream_iterator(std::cerr, ", ")); + std::cerr << std::endl << " SoonWorthless: "; + std::for_each(SoonWorthlessSigners.begin(), SoonWorthlessSigners.end(), [](Signer const &sig) { std::cerr << sig.key << ", "; }); + std::cerr << std::endl << " NoPubKey: "; + std::copy(NoPubKeySigners.begin(), NoPubKeySigners.end(), std::ostream_iterator(std::cerr, ", ")); + std::cerr << std::endl; + } + if (WEXITSTATUS(status) == 0) { if (keyIsID) @@ -309,8 +353,7 @@ bool GPGVMethod::URIAcquire(std::string const &Message, FetchItem *Itm) // Check if all good signers are soon worthless and warn in that case if (std::all_of(GoodSigners.begin(), GoodSigners.end(), [&](std::string const &good) { return std::any_of(SoonWorthlessSigners.begin(), SoonWorthlessSigners.end(), [&](Signer const &weak) { - // VALIDSIG reports a keyid (40 = 24 + 16), GOODSIG is a longid (16) only - return weak.key.compare(24, 16, good, strlen("GOODSIG "), 16) == 0; + return IsTheSameKey(weak.key, good); }); })) { diff --git a/test/integration/test-releasefile-verification b/test/integration/test-releasefile-verification index 54483ba9a..ffb5073b6 100755 --- a/test/integration/test-releasefile-verification +++ b/test/integration/test-releasefile-verification @@ -97,6 +97,7 @@ updatewithwarnings() { } runtest() { + local DELETEFILE="$1" msgmsg 'Cold archive signed by' 'Joe Sixpack' prepare "${PKGFILE}" rm -rf rootdir/var/lib/apt/lists @@ -257,19 +258,14 @@ runtest2() { } runtest3() { - export APT_TESTS_DIGEST_ALGO="$1" - msgmsg "Running base test with digest $1" + echo "Debug::Acquire::gpgv::configdigest::truststate \"$1\";" > rootdir/etc/apt/apt.conf.d/truststate + msgmsg "Running base test with $1 digest" runtest2 - DELETEFILE="InRelease" - msgmsg "Running test with deletion of $DELETEFILE and digest $1" - runtest - - DELETEFILE="Release.gpg" - msgmsg "Running test with deletion of $DELETEFILE and digest $1" - runtest - - unset APT_TESTS_DIGEST_ALGO + for DELETEFILE in 'InRelease' 'Release.gpg'; do + msgmsg "Running test with deletion of $DELETEFILE and $1 digest" + runtest "$DELETEFILE" + done } # diable some protection by default and ensure we still do the verification @@ -278,17 +274,50 @@ cat > rootdir/etc/apt/apt.conf.d/weaken-security < rootdir/etc/apt/apt.conf.d/truststate +runfailure() { + for DELETEFILE in 'InRelease' 'Release.gpg'; do + msgmsg 'Cold archive signed by' 'Joe Sixpack' + prepare "${PKGFILE}" + rm -rf rootdir/var/lib/apt/lists + signreleasefiles 'Joe Sixpack' + find aptarchive/ -name "$DELETEFILE" -delete + testfailure aptget update --no-allow-insecure-repositories -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::gpgv=1 + testsuccess grep 'The following signatures were invalid' rootdir/tmp/testfailure.output + testnopackage 'apt' + testwarning aptget update --allow-insecure-repositories -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::gpgv=1 + failaptold + + msgmsg 'Cold archive signed by' 'Marvin Paranoid' + prepare "${PKGFILE}" + rm -rf rootdir/var/lib/apt/lists + signreleasefiles 'Marvin Paranoid' + find aptarchive/ -name "$DELETEFILE" -delete + testfailure aptget update --no-allow-insecure-repositories -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::gpgv=1 + testnopackage 'apt' + updatewithwarnings '^W: .* NO_PUBKEY' + testsuccessequal "$(cat "${PKGFILE}") +" aptcache show apt + failaptold + done +} +runfailure + +msgmsg "Running test with gpgv-untrusted digest" +export APT_TESTS_DIGEST_ALGO='MD5' +runfailure -- cgit v1.2.3