From 2fac0dd5a7a62b67a869cd4c71c9d09159aaa31d Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 8 May 2016 19:46:34 +0200 Subject: [PATCH] gpgv: show always webportal error on NODATA gpg doesn't give use a UID on NODATA, which we were "expecting" (but not using for anything), but just an error number. Instead of collecting these as badsigners which will trigger a "invald signature" error with remarks like "NODATA 1" we instead adapt a message similar to the NODATA error of a clearsigned file (which is actually not reached anymore as we split them up, which fails with a NOSPLIT error, which uses the same general error message). In other words: Not a security relevant change, just a user experience improvement as we now point them to the most likely cause of the problem instead of saying "invalid signature" which would point them in the direction of the archive being broken (for everyone) instead. Closes: 823746 --- methods/gpgv.cc | 33 +++++++---- test/integration/framework | 5 +- test/integration/test-apt-key | 6 +- ...t-ubuntu-bug-346386-apt-get-update-paywall | 55 ++++++++++--------- 4 files changed, 58 insertions(+), 41 deletions(-) diff --git a/methods/gpgv.cc b/methods/gpgv.cc index b9fb09a8f..dd395d659 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -173,6 +173,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, std::vector ErrSigners; size_t buffersize = 0; char *buffer = NULL; + bool gotNODATA = false; while (1) { if (getline(&buffer, &buffersize, pipein) == -1) @@ -194,8 +195,8 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, ErrSigners.erase(std::remove_if(ErrSigners.begin(), ErrSigners.end(), [&](std::string const &errsig) { return errsig.compare(strlen("ERRSIG "), 16, buffer, sizeof(GNUPGNOPUBKEY), 16) == 0; }), ErrSigners.end()); } - else if (strncmp(buffer, GNUPGNODATA, sizeof(GNUPGBADSIG)-1) == 0) - PushEntryWithUID(BadSigners, buffer, Debug); + else if (strncmp(buffer, GNUPGNODATA, sizeof(GNUPGNODATA)-1) == 0) + gotNODATA = true; else if (strncmp(buffer, GNUPGEXPKEYSIG, sizeof(GNUPGEXPKEYSIG)-1) == 0) PushEntryWithUID(WorthlessSigners, buffer, Debug); else if (strncmp(buffer, GNUPGEXPSIG, sizeof(GNUPGEXPSIG)-1) == 0) @@ -293,10 +294,26 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, 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; + std::cerr << std::endl << " NODATA: " << (gotNODATA ? "yes" : "no") << std::endl; } - if (WEXITSTATUS(status) == 0) + if (WEXITSTATUS(status) == 112) + { + // acquire system checks for "NODATA" to generate GPG errors (the others are only warnings) + std::string errmsg; + //TRANSLATORS: %s is a single techy word like 'NODATA' + strprintf(errmsg, _("Clearsigned file isn't valid, got '%s' (does the network require authentication?)"), "NODATA"); + return errmsg; + } + else if (gotNODATA) + { + // acquire system checks for "NODATA" to generate GPG errors (the others are only warnings) + std::string errmsg; + //TRANSLATORS: %s is a single techy word like 'NODATA' + strprintf(errmsg, _("Signed file isn't valid, got '%s' (does the network require authentication?)"), "NODATA"); + return errmsg; + } + else if (WEXITSTATUS(status) == 0) { if (keyIsID) { @@ -316,14 +333,6 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, return _("At least one invalid signature was encountered."); else if (WEXITSTATUS(status) == 111) return _("Could not execute 'apt-key' to verify signature (is gnupg installed?)"); - else if (WEXITSTATUS(status) == 112) - { - // acquire system checks for "NODATA" to generate GPG errors (the others are only warnings) - std::string errmsg; - //TRANSLATORS: %s is a single techy word like 'NODATA' - strprintf(errmsg, _("Clearsigned file isn't valid, got '%s' (does the network require authentication?)"), "NODATA"); - return errmsg; - } else return _("Unknown error executing apt-key"); } diff --git a/test/integration/framework b/test/integration/framework index 1f4da97f0..ea577c04e 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1697,8 +1697,9 @@ testfailure() { local EXITCODE=$? if expr match "$1" '^apt.*' >/dev/null; then if [ "$1" = 'aptkey' ]; then - if grep -q -E " Can't check signature: " "$OUTPUT" || \ - grep -q -E " BAD signature from " "$OUTPUT"; then + if grep -q " Can't check signature: + BAD signature from + signature could not be verified" "$OUTPUT"; then msgpass else msgfailoutput "run failed with exitcode ${EXITCODE}, but no signature error" "$OUTPUT" "$@" diff --git a/test/integration/test-apt-key b/test/integration/test-apt-key index ddb9bf9d2..666136098 100755 --- a/test/integration/test-apt-key +++ b/test/integration/test-apt-key @@ -179,10 +179,14 @@ gpg: unchanged: 1' aptkey --fakeroot update cp -a keys/testcase-multikey.pub rootdir/etc/apt/trusted.gpg.d/multikey.gpg msgtest 'Test signing a file' 'with a key' echo 'Verify me. This is my signature.' > signature + echo 'lalalalala' > signature2 testsuccess --nomsg aptkey --quiet --keyring keys/marvinparanoid.pub --secret-keyring keys/marvinparanoid.sec --readonly \ adv --batch --yes --default-key 'Marvin' --armor --detach-sign --sign --output signature.gpg signature testsuccess test -s signature.gpg -a -s signature + msgtest 'Test verify a file' 'with no sig' + testfailure --nomsg aptkey --quiet --readonly --keyring keys/testcase-multikey.pub verify signature signature2 + for GPGV in '' 'gpgv' 'gpgv2'; do echo "APT::Key::GPGVCommand \"$GPGV\";" > rootdir/etc/apt/apt.conf.d/00gpgvcmd @@ -210,7 +214,6 @@ gpg: unchanged: 1' aptkey --fakeroot update testfailure --nomsg aptkey --quiet --readonly --keyid 'Kalnischkies' verify signature.gpg signature msgtest 'Test verify fails on' 'bad file' - echo 'lalalalala' > signature2 testfailure --nomsg aptkey --quiet --readonly verify signature.gpg signature2 done rm -f rootdir/etc/apt/apt.conf.d/00gpgvcmd @@ -257,7 +260,6 @@ gpg: unchanged: 1' aptkey --fakeroot update testfailure --nomsg aptkey --quiet --readonly --keyid 'Kalnischkies' verify signature.gpg signature msgtest 'Test verify fails on' 'bad doublesigned file' - echo 'lalalalala' > signature2 testfailure --nomsg aptkey --quiet --readonly verify signature.gpg signature2 done rm -f rootdir/etc/apt/apt.conf.d/00gpgvcmd diff --git a/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall b/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall index 07c13434c..a756b5df2 100755 --- a/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall +++ b/test/integration/test-ubuntu-bug-346386-apt-get-update-paywall @@ -13,7 +13,7 @@ insertsource 'unstable' 'unrelated' 'all' '1.0' 'stable' echo 'ni ni ni' > aptarchive/knights setupaptarchive -changetowebserver -o 'aptwebserver::overwrite::.*::filename=/knights' +changetowebserver -o 'aptwebserver::overwrite::.*InRelease::filename=/knights' -o 'aptwebserver::overwrite::.*::filename=/knights' msgtest 'Acquire test file from the webserver to check' 'overwrite' if downloadfile http://localhost:${APTHTTPPORT}/holygrail ./knights-talking >/dev/null; then @@ -34,34 +34,39 @@ ensure_n_canary_strings_in_dir() { test "$N" = "$EXPECTED_N" && msgpass || msgfail "Expected $EXPECTED_N canaries, got $N" } -LISTS='rootdir/var/lib/apt/lists' -rm -rf rootdir/var/lib/apt/lists -testfailure aptget update -testsuccess grep '^E:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output +runtests() { + LISTS='rootdir/var/lib/apt/lists' + rm -rf "$LISTS" + testfailure aptget update + testsuccess grep "$1" rootdir/tmp/testfailure.output -ensure_n_canary_strings_in_dir "$LISTS" 'ni ni ni' 0 -testequal 'lock + ensure_n_canary_strings_in_dir "$LISTS" 'ni ni ni' 0 + testequal 'lock partial' ls "$LISTS" -# and again with pre-existing files with "valid data" which should remain -for f in Release Release.gpg main_binary-amd64_Packages main_source_Sources; do - echo 'peng neee-wom' > "$LISTS/localhost:${APTHTTPPORT}_dists_stable_${f}" - chmod 644 "$LISTS/localhost:${APTHTTPPORT}_dists_stable_${f}" -done + # and again with pre-existing files with "valid data" which should remain + for f in Release Release.gpg main_binary-amd64_Packages main_source_Sources; do + echo 'peng neee-wom' > "$LISTS/localhost:${APTHTTPPORT}_dists_stable_${f}" + chmod 644 "$LISTS/localhost:${APTHTTPPORT}_dists_stable_${f}" + done -testfailure aptget update -testsuccess grep '^E:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output + testfailure aptget update + testsuccess grep "$1" rootdir/tmp/testfailure.output -ensure_n_canary_strings_in_dir "$LISTS" 'peng neee-wom' 4 -ensure_n_canary_strings_in_dir "$LISTS" 'ni ni ni' 0 + ensure_n_canary_strings_in_dir "$LISTS" 'peng neee-wom' 4 + ensure_n_canary_strings_in_dir "$LISTS" 'ni ni ni' 0 -# and now with a pre-existing InRelease file -echo 'peng neee-wom' > "$LISTS/localhost:${APTHTTPPORT}_dists_stable_InRelease" -chmod 644 "$LISTS/localhost:${APTHTTPPORT}_dists_stable_InRelease" -rm -f "$LISTS/localhost:${APTHTTPPORT}_dists_stable_Release" "$LISTS/localhost:${APTHTTPPORT}_dists_stable_Release.gpg" -msgtest 'excpected failure of' 'apt-get update' -testfailure aptget update -testsuccess grep '^E:.*Clearsigned file .*NOSPLIT.*' rootdir/tmp/testfailure.output + # and now with a pre-existing InRelease file + echo 'peng neee-wom' > "$LISTS/localhost:${APTHTTPPORT}_dists_stable_InRelease" + chmod 644 "$LISTS/localhost:${APTHTTPPORT}_dists_stable_InRelease" + rm -f "$LISTS/localhost:${APTHTTPPORT}_dists_stable_Release" "$LISTS/localhost:${APTHTTPPORT}_dists_stable_Release.gpg" + msgtest 'excpected failure of' 'apt-get update' + testfailure aptget update + testsuccess grep "$1" rootdir/tmp/testfailure.output -ensure_n_canary_strings_in_dir "$LISTS" 'peng neee-wom' 3 -ensure_n_canary_strings_in_dir "$LISTS" 'ni ni ni' 0 + ensure_n_canary_strings_in_dir "$LISTS" 'peng neee-wom' 3 + ensure_n_canary_strings_in_dir "$LISTS" 'ni ni ni' 0 +} +runtests '^E:.*Clearsigned file .*NOSPLIT.*' +webserverconfig 'aptwebserver::overwrite::.*InRelease::filename' '/404' +runtests '^E:.*Signed file .*NODATA.*' -- 2.45.2