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

diff --git a/methods/gpgv.cc b/methods/gpgv.cc
index 70207e6150ff52061ed47e9e23a021aaa8ab83e8..b6e0fa7bda40d1901becd00f114c83c0553b1b69 100644 (file)
--- 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 897ae3bfe66f87b047e8eaf5d2cb9a4ba823b1ad..fc59c64503e650f5c4234c4ce02f4698c429c373 100644 (file)
--- 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 41661f88d07152595d9c2b8315c1eab5655386a5..54483ba9a6feb924d8d7d91bea2babf2eb5310eb 100755 (executable)
--- 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'