properly check for "all good sigs are weak"

Using erase(pos) is invalid in our case here as pos must be a valid and
derefenceable iterator, which isn't the case for an end-iterator (like
if we had no good signature).
The problem runs deeper still through as VALIDSIG is a keyid while
GOODSIG is just a longid so comparing them will always fail.

Closes: 818910

3 files changed, 67 insertions, 44 deletions

diff --git a/methods/gpgv.cc b/methods/gpgv.cc
index 70207e615..b6e0fa7bd 100644
--- a/methods/gpgv.cc
+++ b/methods/gpgv.cc
@@ -189,8 +189,6 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile,
          while (*p && isxdigit(*p))
             p++;
          *p = 0;
-         if (Debug == true)
-            std::clog << "Got VALIDSIG, key ID: " << sig << std::endl;
          // Reject weak digest algorithms
          Digest digest = FindDigest(tokens[7]);
          switch (digest.state) {
@@ -198,14 +196,20 @@ 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.
             SoonWorthlessSigners.push_back({string(sig), digest.name});
+	    if (Debug == true)
+	       std::clog << "Got weak VALIDSIG, key ID: " << sig << std::endl;
             break;
          case Digest::State::Untrusted:
             // 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)));
+	    if (Debug == true)
+	       std::clog << "Got untrusted VALIDSIG, key ID: " << sig << std::endl;
             break;
          case Digest::State::Trusted:
+	    if (Debug == true)
+	       std::clog << "Got trusted VALIDSIG, key ID: " << sig << std::endl;
             break;
          }
 
@@ -302,13 +306,14 @@ bool GPGVMethod::URIAcquire(std::string const &Message, FetchItem *Itm)
                                  GoodSigners, BadSigners, WorthlessSigners,
                                  SoonWorthlessSigners, NoPubKeySigners);
 
-
-   // Check if there are any good signers that are not soon worthless
-   std::vector<std::string> NotWarnAboutSigners(GoodSigners);
-   for (auto const & Signer : SoonWorthlessSigners)
-      NotWarnAboutSigners.erase(std::remove(NotWarnAboutSigners.begin(), NotWarnAboutSigners.end(), "GOODSIG " + Signer.key));
-   // If all signers are soon worthless, report them.
-   if (NotWarnAboutSigners.empty()) {
+   // 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;
+		  });
+	    }))
+   {
       for (auto const & Signer : SoonWorthlessSigners)
          // TRANSLATORS: The second %s is the reason and is untranslated for repository owners.
          Warning(_("Signature by key %s uses weak digest algorithm (%s)"), Signer.key.c_str(), Signer.note.c_str());
diff --git a/test/integration/framework b/test/integration/framework
index 897ae3bfe..fc59c6450 100644
--- a/test/integration/framework
+++ b/test/integration/framework
@@ -1074,7 +1074,7 @@ signreleasefiles() {
 	local SIGNER="${1:-Joe Sixpack}"
 	local REPODIR="${2:-aptarchive}"
 	local KEY="keys/$(echo "$SIGNER" | tr 'A-Z' 'a-z' | sed 's# ##g')"
-	local GPG="aptkey --quiet --keyring ${KEY}.pub --secret-keyring ${KEY}.sec --readonly adv --batch --yes --digest-algo SHA512"
+	local GPG="aptkey --quiet --keyring ${KEY}.pub --secret-keyring ${KEY}.sec --readonly adv --batch --yes --digest-algo ${APT_TESTS_DIGEST_ALGO:-SHA512}"
 	msgninfo "\tSign archive with $SIGNER key $KEY… "
 	local REXKEY='keys/rexexpired'
 	local SECEXPIREBAK="${REXKEY}.sec.bak"
diff --git a/test/integration/test-releasefile-verification b/test/integration/test-releasefile-verification
index 41661f88d..54483ba9a 100755
--- a/test/integration/test-releasefile-verification
+++ b/test/integration/test-releasefile-verification
@@ -97,166 +97,181 @@ updatewithwarnings() {
 }
 
 runtest() {
+	msgmsg 'Cold archive signed by' 'Joe Sixpack'
 	prepare "${PKGFILE}"
 	rm -rf rootdir/var/lib/apt/lists
 	signreleasefiles 'Joe Sixpack'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Cold archive signed by' 'Joe Sixpack'
-	testsuccess aptget update
+	successfulaptgetupdate
 	testsuccessequal "$(cat "${PKGFILE}")
 " aptcache show apt
 	installaptold
 
+	msgmsg 'Good warm archive signed by' 'Joe Sixpack'
 	prepare "${PKGFILE}-new"
 	signreleasefiles 'Joe Sixpack'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Good warm archive signed by' 'Joe Sixpack'
-	testsuccess aptget update
+	successfulaptgetupdate
 	testsuccessequal "$(cat "${PKGFILE}-new")
 " aptcache show apt
 	installaptnew
 
+	msgmsg 'Cold archive signed by' 'Rex Expired'
 	prepare "${PKGFILE}"
 	rm -rf rootdir/var/lib/apt/lists
 	cp keys/rexexpired.pub rootdir/etc/apt/trusted.gpg.d/rexexpired.gpg
 	signreleasefiles 'Rex Expired'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Cold archive signed by' 'Rex Expired'
 	updatewithwarnings '^W: .* KEYEXPIRED'
 	testsuccessequal "$(cat "${PKGFILE}")
 " aptcache show apt
 	failaptold
 	rm rootdir/etc/apt/trusted.gpg.d/rexexpired.gpg
 
+	msgmsg 'Cold archive signed by' 'Marvin Paranoid'
 	prepare "${PKGFILE}"
 	rm -rf rootdir/var/lib/apt/lists
 	signreleasefiles 'Marvin Paranoid'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Cold archive signed by' 'Marvin Paranoid'
 	updatewithwarnings '^W: .* NO_PUBKEY'
 	testsuccessequal "$(cat "${PKGFILE}")
 " aptcache show apt
 	failaptold
 
+	msgmsg 'Bad warm archive signed by' 'Joe Sixpack'
 	prepare "${PKGFILE}-new"
 	signreleasefiles 'Joe Sixpack'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Bad warm archive signed by' 'Joe Sixpack'
-	testsuccess aptget update
+	successfulaptgetupdate
 	testsuccessequal "$(cat "${PKGFILE}-new")
 " aptcache show apt
 	installaptnew
 
-
+	msgmsg 'Cold archive signed by' 'Joe Sixpack'
 	prepare "${PKGFILE}"
 	rm -rf rootdir/var/lib/apt/lists
 	signreleasefiles 'Joe Sixpack'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Cold archive signed by' 'Joe Sixpack'
-	testsuccess aptget update
+	successfulaptgetupdate
 	testsuccessequal "$(cat "${PKGFILE}")
 " aptcache show apt
 	installaptold
 
+	msgmsg 'Good warm archive signed by' 'Marvin Paranoid'
 	prepare "${PKGFILE}-new"
 	signreleasefiles 'Marvin Paranoid'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Good warm archive signed by' 'Marvin Paranoid'
 	updatewithwarnings '^W: .* NO_PUBKEY'
 	testsuccessequal "$(cat "${PKGFILE}")
 " aptcache show apt
 	installaptold
 
+	msgmsg 'Good warm archive signed by' 'Rex Expired'
 	prepare "${PKGFILE}-new"
 	cp keys/rexexpired.pub rootdir/etc/apt/trusted.gpg.d/rexexpired.gpg
 	signreleasefiles 'Rex Expired'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Good warm archive signed by' 'Rex Expired'
 	updatewithwarnings '^W: .* KEYEXPIRED'
 	testsuccessequal "$(cat "${PKGFILE}")
 " aptcache show apt
 	installaptold
 	rm rootdir/etc/apt/trusted.gpg.d/rexexpired.gpg
 
+	msgmsg 'Good warm archive signed by' 'Joe Sixpack'
 	prepare "${PKGFILE}-new"
 	signreleasefiles
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Good warm archive signed by' 'Joe Sixpack'
-	testsuccess aptget update
+	successfulaptgetupdate
 	testsuccessequal "$(cat "${PKGFILE}-new")
 " aptcache show apt
 	installaptnew
 
+	msgmsg 'Cold archive signed by good keyring' 'Marvin Paranoid'
 	prepare "${PKGFILE}"
 	rm -rf rootdir/var/lib/apt/lists
 	signreleasefiles 'Marvin Paranoid'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Cold archive signed by good keyring' 'Marvin Paranoid'
 	local MARVIN="$(readlink -f keys/marvinparanoid.pub)"
 	sed -i "s#^\(deb\(-src\)\?\) #\1 [signed-by=$MARVIN] #" rootdir/etc/apt/sources.list.d/*
-	testsuccess aptget update -o Debug::pkgAcquire::Worker=1
+	successfulaptgetupdate
 	testsuccessequal "$(cat "${PKGFILE}")
 " aptcache show apt
 	installaptold
 
+	msgmsg 'Cold archive signed by bad keyring' 'Joe Sixpack'
 	rm -rf rootdir/var/lib/apt/lists
 	signreleasefiles 'Joe Sixpack'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Cold archive signed by bad keyring' 'Joe Sixpack'
 	updatewithwarnings '^W: .* NO_PUBKEY'
 
 	sed -i "s#^\(deb\(-src\)\?\) \[signed-by=$MARVIN\] #\1 #" rootdir/etc/apt/sources.list.d/*
 	local MARVIN="$(aptkey --keyring $MARVIN finger | grep 'Key fingerprint' | cut -d'=' -f 2 | tr -d ' ')"
 
+	msgmsg 'Cold archive signed by good keyid' 'Marvin Paranoid'
 	prepare "${PKGFILE}"
 	rm -rf rootdir/var/lib/apt/lists
 	signreleasefiles 'Marvin Paranoid'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Cold archive signed by good keyid' 'Marvin Paranoid'
 	sed -i "s#^\(deb\(-src\)\?\) #\1 [signed-by=$MARVIN] #" rootdir/etc/apt/sources.list.d/*
 	cp keys/marvinparanoid.pub rootdir/etc/apt/trusted.gpg.d/marvinparanoid.gpg
-	testsuccess aptget update -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::gpgv=1
+	successfulaptgetupdate
 	testsuccessequal "$(cat "${PKGFILE}")
 " aptcache show apt
 	installaptold
 	rm -f rootdir/etc/apt/trusted.gpg.d/marvinparanoid.gpg
 
+	msgmsg 'Cold archive signed by bad keyid' 'Joe Sixpack'
 	rm -rf rootdir/var/lib/apt/lists
 	signreleasefiles 'Joe Sixpack'
 	find aptarchive/ -name "$DELETEFILE" -delete
-	msgmsg 'Cold archive signed by bad keyid' 'Joe Sixpack'
 	updatewithwarnings '^W: .* be verified because the public key is not available: .*'
 
 	sed -i "s#^\(deb\(-src\)\?\) \[signed-by=$MARVIN\] #\1 #" rootdir/etc/apt/sources.list.d/*
 }
 
 runtest2() {
+	msgmsg 'Cold archive signed by' 'Joe Sixpack'
 	prepare "${PKGFILE}"
 	rm -rf rootdir/var/lib/apt/lists
 	signreleasefiles 'Joe Sixpack'
-	msgmsg 'Cold archive signed by' 'Joe Sixpack'
-	testsuccess aptget update
+	successfulaptgetupdate
 
 	# New .deb but now an unsigned archive. For example MITM to circumvent
 	# package verification.
+	msgmsg 'Warm archive signed by' 'nobody'
 	prepare "${PKGFILE}-new"
 	find aptarchive/ -name InRelease -delete
 	find aptarchive/ -name Release.gpg -delete
-	msgmsg 'Warm archive signed by' 'nobody'
 	updatewithwarnings 'W: .* no longer signed.'
 	testsuccessequal "$(cat "${PKGFILE}-new")
 " aptcache show apt
 	failaptnew
 
 	# Unsigned archive from the beginning must also be detected.
-	rm -rf rootdir/var/lib/apt/lists
 	msgmsg 'Cold archive signed by' 'nobody'
+	rm -rf rootdir/var/lib/apt/lists
 	updatewithwarnings 'W: .* is not signed.'
 	testsuccessequal "$(cat "${PKGFILE}-new")
 " aptcache show apt
 	failaptnew
 }
 
+runtest3() {
+	export APT_TESTS_DIGEST_ALGO="$1"
+	msgmsg "Running base test with digest $1"
+	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
+}
+
 # diable some protection by default and ensure we still do the verification
 # correctly
 cat > rootdir/etc/apt/apt.conf.d/weaken-security <<EOF
@@ -264,13 +279,16 @@ Acquire::AllowInsecureRepositories "1";
 Acquire::AllowDowngradeToInsecureRepositories "1";
 EOF
 
-msgmsg "Running base test"
-runtest2
-
-DELETEFILE="InRelease"
-msgmsg "Running test with deletion of $DELETEFILE"
-runtest
+# an all-round good hash
+successfulaptgetupdate() {
+	testsuccess aptget update -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::gpgv=1
+}
+runtest3 'SHA512'
 
-DELETEFILE="Release.gpg"
-msgmsg "Running test with deletion of $DELETEFILE"
-runtest
+# a hash we consider weak and therefore warn about
+rm -f rootdir/etc/apt/apt.conf.d/no-sha1
+successfulaptgetupdate() {
+	testwarning aptget update -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::gpgv=1
+	testsuccess grep 'uses weak digest algorithm' rootdir/tmp/testwarning.output
+}
+runtest3 'SHA1'