From b934870c4f893be28eb1537910c0aadce6dd6e09 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 16 Aug 2018 23:36:41 +0200 Subject: Report (soon) worthless keys if gpg uses fpr for GOODSIG gpgs DETAILS documentation file declares that GOODSIG could report keyid or fingerprint since gpg2, but for the time being it is still keyid only. Who knows if that will ever change as that feels like an interface break with dangerous security implications, but lets be better safe than sorry especially as the code dealing with signed-by keyids is prepared for this already. This code is rewritten still to have them all use the same code for this type of problem. --- methods/gpgv.cc | 59 ++++++++++++++++----------------------- test/integration/test-method-gpgv | 25 +++++++++++++++++ 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 8de15c48a..84b8c3e59 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -94,7 +94,10 @@ struct Signer { 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 + // VALIDSIG reports a fingerprint (40 = 24 + 16), GOODSIG can be longid (16) or + // fingerprint according to documentation in DETAILS.gz + if (goodsig.length() == 40 + strlen("GOODSIG ")) + return validsig.compare(0, 40, goodsig, strlen("GOODSIG "), 40) == 0; return validsig.compare(24, 16, goodsig, strlen("GOODSIG "), 16) == 0; } @@ -254,46 +257,32 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, if (keyIsID == true) { if (Debug == true) - std::clog << "GoodSigs needs to be limited to keyid " << key << std::endl; - bool foundGood = false; - for (auto const &k: VectorizeString(key, ',')) + std::clog << "GoodSigs needs to be limited to keyid(s) " << key << std::endl; + auto const limitedTo = VectorizeString(key, ','); + std::vector filteredGood; + for (auto &&good: GoodSigners) { - if (std::find(ValidSigners.begin(), ValidSigners.end(), k) == ValidSigners.end()) - continue; - // we look for GOODSIG here as well as an expired sig is a valid sig as well (but not a good one) - std::string const goodfingerprint = "GOODSIG " + k; - std::string const goodlongkeyid = "GOODSIG " + k.substr(24, 16); - foundGood = std::find(GoodSigners.begin(), GoodSigners.end(), goodfingerprint) != GoodSigners.end(); if (Debug == true) - std::clog << "Key " << k << " is valid sig, is " << goodfingerprint << " also a good one? " << (foundGood ? "yes" : "no") << std::endl; - std::string goodsig; - if (foundGood == false) + std::clog << "Key " << good << " is good sig, is it also a valid and allowed one? "; + bool found = false; + for (auto const &l : limitedTo) { - foundGood = std::find(GoodSigners.begin(), GoodSigners.end(), goodlongkeyid) != GoodSigners.end(); - if (Debug == true) - std::clog << "Key " << k << " is valid sig, is " << goodlongkeyid << " also a good one? " << (foundGood ? "yes" : "no") << std::endl; - goodsig = goodlongkeyid; + if (IsTheSameKey(l, good) == false) + continue; + // GOODSIG might be "just" a longid, so we check VALIDSIG which is always a fingerprint + if (std::find(ValidSigners.begin(), ValidSigners.end(), l) == ValidSigners.end()) + continue; + found = true; + break; } + if (Debug) + std::clog << (found ? "yes" : "no") << "\n"; + if (found) + filteredGood.emplace_back(std::move(good)); else - goodsig = goodfingerprint; - if (foundGood == false) - continue; - std::copy(GoodSigners.begin(), GoodSigners.end(), std::back_insert_iterator >(NoPubKeySigners)); - GoodSigners.clear(); - GoodSigners.push_back(goodsig); - NoPubKeySigners.erase( - std::remove(NoPubKeySigners.begin(), - std::remove(NoPubKeySigners.begin(), NoPubKeySigners.end(), goodfingerprint), - goodlongkeyid), - NoPubKeySigners.end() - ); - break; - } - if (foundGood == false) - { - std::copy(GoodSigners.begin(), GoodSigners.end(), std::back_insert_iterator >(NoPubKeySigners)); - GoodSigners.clear(); + NoPubKeySigners.emplace_back(std::move(good)); } + GoodSigners = std::move(filteredGood); } int status; diff --git a/test/integration/test-method-gpgv b/test/integration/test-method-gpgv index 86559b7cb..5e00b1f13 100755 --- a/test/integration/test-method-gpgv +++ b/test/integration/test-method-gpgv @@ -40,6 +40,20 @@ testrun() { testgpgv 'Good signed with fingerprint' 'Good: GOODSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE,' '[GNUPG:] GOODSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE Joe Sixpack (APT Testcases Dummy) [GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2016-09-01 1472742625 0 4 0 1 11 00 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE' + testgpgv 'Untrusted signed with long keyid' 'Worthless: 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE,' '[GNUPG:] GOODSIG 5A90D141DBAC8DAE Joe Sixpack (APT Testcases Dummy) +[GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2016-09-01 1472742625 0 4 0 1 1 00 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE' + testsuccess grep '^\s\+Good:\s\+$' method.output + testgpgv 'Untrusted signed with fingerprint' 'Worthless: 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE,' '[GNUPG:] GOODSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE Joe Sixpack (APT Testcases Dummy) +[GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2016-09-01 1472742625 0 4 0 1 1 00 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE' + testsuccess grep '^\s\+Good:\s\+$' method.output + + testgpgv 'Weak signed with long keyid' 'Good: GOODSIG 5A90D141DBAC8DAE,' '[GNUPG:] GOODSIG 5A90D141DBAC8DAE Joe Sixpack (APT Testcases Dummy) +[GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2016-09-01 1472742625 0 4 0 1 2 00 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE' + testsuccess grep '^Message: Signature by key 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE uses weak digest algorithm (SHA1)$' method.output + testgpgv 'Weak signed with fingerprint' 'Good: GOODSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE,' '[GNUPG:] GOODSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE Joe Sixpack (APT Testcases Dummy) +[GNUPG:] VALIDSIG 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE 2016-09-01 1472742625 0 4 0 1 2 00 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE' + testsuccess grep '^Message: Signature by key 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE uses weak digest algorithm (SHA1)$' method.output + testgpgv 'No Pubkey with long keyid' 'NoPubKey: NO_PUBKEY E8525D47528144E2,' '[GNUPG:] ERRSIG E8525D47528144E2 1 11 00 1472744666 9 [GNUPG:] NO_PUBKEY E8525D47528144E2' testgpgv 'No Pubkey with fingerprint' 'NoPubKey: NO_PUBKEY DE66AECA9151AFA1877EC31DE8525D47528144E2,' '[GNUPG:] ERRSIG DE66AECA9151AFA1877EC31DE8525D47528144E2 1 11 00 1472744666 9 @@ -55,6 +69,7 @@ gpgvmethod() { echo '601 Configuration Config-Item: Debug::Acquire::gpgv=1 Config-Item: Dir::Bin::apt-key=./faked-apt-key +Config-Item: APT::Hashes::SHA1::Weak=true 600 URI Acquire URI: file:///dev/null @@ -67,6 +82,7 @@ gpgvmethod() { echo '601 Configuration Config-Item: Debug::Acquire::gpgv=1 Config-Item: Dir::Bin::apt-key=./faked-apt-key +Config-Item: APT::Hashes::SHA1::Weak=true 600 URI Acquire URI: file:///dev/null @@ -75,3 +91,12 @@ Signed-By: 34A8E9D18DB320F367E8EAA05A90D141DBAC8DAE ' | runapt "${METHODSDIR}/gpgv" } testrun + +testgpgv 'Good signed with long keyid but not signed-by key' 'NoPubKey: GOODSIG 4BC0A39C27CE74F9,' '[GNUPG:] GOODSIG 4BC0A39C27CE74F9 Rex Expired +[GNUPG:] VALIDSIG 891CC50E605796A0C6E733F74BC0A39C27CE74F9 2016-09-01 1472742625 0 4 0 1 11 00 891CC50E605796A0C6E733F74BC0A39C27CE74F9' +testsuccess grep '^\s\+Good:\s\+$' method.output +testsuccess grep 'verified because the public key is not available: GOODSIG' method.output +testgpgv 'Good signed with fingerprint' 'NoPubKey: GOODSIG 891CC50E605796A0C6E733F74BC0A39C27CE74F9,' '[GNUPG:] GOODSIG 891CC50E605796A0C6E733F74BC0A39C27CE74F9 Rex Expired +[GNUPG:] VALIDSIG 891CC50E605796A0C6E733F74BC0A39C27CE74F9 2016-09-01 1472742625 0 4 0 1 11 00 891CC50E605796A0C6E733F74BC0A39C27CE74F9' +testsuccess grep '^\s\+Good:\s\+$' method.output +testsuccess grep 'verified because the public key is not available: GOODSIG' method.output -- cgit v1.2.3