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.

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<std::string> 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<std::vector<std::string> >(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<std::vector<std::string> >(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) <joe@example.org>
 [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) <joe@example.org>
+[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) <joe@example.org>
+[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) <joe@example.org>
+[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) <joe@example.org>
+[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 <rex@example.org>
+[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 <rex@example.org>
+[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