From ee427f308600a4a3a6f67a4a7835e1172605ba06 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 7 Mar 2022 11:53:27 +0100 Subject: gpgv: Fix legacy fallback on unavailable keys If a repository is signed with multiple keys, apt 2.4.0 would ignore the fallback result if some keys were still missing, causing signature verification to fail. Rework the logic such that when checking if fallback was "succesful", missing keys are ignored - it only matters if we managed to verify one key now, whether good or bad. Likewise, simplify the logic when to do the fallback: If there was a bad signature in trusted.gpg.d, do NOT fallback at all - this is a minor security issue, as a key in trusted.gpg.d could fail silently with a bad signature, and then a key in trusted.gpg might allow the signature to succeed (as trusted.gpg.d key is then missing). Only fallback if we are missing a good signature, and there are keys we have not yet checked. --- methods/gpgv.cc | 14 ++++++++++---- test/integration/test-method-gpgv-legacy-keyring | 8 ++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/methods/gpgv.cc b/methods/gpgv.cc index fdd8586b4..0d5707e2a 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -429,7 +429,14 @@ string GPGVMethod::VerifyGetSignersWithLegacy(const char *file, const char *outf string const msg = VerifyGetSigners(file, outfile, keyFpts, keyFiles, Signers); if (_error->PendingError()) return msg; - if (keyFiles.empty() && (Signers.Good.empty() || !Signers.Bad.empty() || !Signers.NoPubKey.empty())) + + // Bad signature always remains bad, no need to retry against trusted.gpg + if (!Signers.Bad.empty()) + return msg; + + // We do not have a key file pinned, did not find a good signature, but found + // missing keys - let's retry with trusted.gpg + if (keyFiles.empty() && Signers.Good.empty() && !Signers.NoPubKey.empty()) { std::vector legacyKeyFiles{_config->FindFile("Dir::Etc::trusted")}; if (legacyKeyFiles[0].empty()) @@ -437,14 +444,13 @@ string GPGVMethod::VerifyGetSignersWithLegacy(const char *file, const char *outf if (DebugEnabled()) std::clog << "Retrying against " << legacyKeyFiles[0] << "\n"; - // Retry against trusted.gpg SignersStorage legacySigners; string const legacyMsg = VerifyGetSigners(file, outfile, keyFpts, legacyKeyFiles, legacySigners); if (_error->PendingError()) return legacyMsg; - // Hooray, we found the key now - if (not(legacySigners.Good.empty() || !legacySigners.Bad.empty() || !legacySigners.NoPubKey.empty())) + // Hooray, we found a key apparently, something verified as good or bad + if (!legacySigners.Good.empty() || !legacySigners.Bad.empty()) { std::string warning; strprintf(warning, diff --git a/test/integration/test-method-gpgv-legacy-keyring b/test/integration/test-method-gpgv-legacy-keyring index 37a86529a..5af955cdf 100755 --- a/test/integration/test-method-gpgv-legacy-keyring +++ b/test/integration/test-method-gpgv-legacy-keyring @@ -25,3 +25,11 @@ testwarningequal "Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InReleas Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InRelease [1420 B] Reading package lists... W: file:${TMPWORKINGDIRECTORY}/aptarchive/dists/unstable/InRelease: Key is stored in legacy trusted.gpg keyring (${TMPWORKINGDIRECTORY}/rootdir/etc/apt/trusted.gpg), see the DEPRECATION section in apt-key(8) for details." aptget update -q + +# 2.4.0 regression: If the InRelease file was signed with two keys, fallback to trusted.gpg did not +# work: It ran the fallback, but then ignored the result, as keys were still missing. +signreleasefiles 'Joe Sixpack,Marvin Paranoid' +testwarningequal "Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InRelease [1867 B] +Get:1 file:${TMPWORKINGDIRECTORY}/aptarchive unstable InRelease [1867 B] +Reading package lists... +W: file:${TMPWORKINGDIRECTORY}/aptarchive/dists/unstable/InRelease: Key is stored in legacy trusted.gpg keyring (${TMPWORKINGDIRECTORY}/rootdir/etc/apt/trusted.gpg), see the DEPRECATION section in apt-key(8) for details." aptget update -q -omsg=with-two-signatures -- cgit v1.2.3-70-g09d2 From 55452afa1e8eb3b252f76e455b49df5883e0b811 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 7 Mar 2022 13:03:24 +0100 Subject: gpgv: Use Valid instead of Good to determine fallback Change the logic to use "Valid" instead of "Good" to determine whether we need to fallback and if fallback was successful. That means that if you have an expired key in trusted.gpg.d, and a non-expired in trusted.gpg, verification will now fail directly with the expired key in trusted.gpg.d and not try to fallback. Likewise, if the key in trusted.gpg is expired, this will now also be reported correctly again, instead of producing an error message that the key could not be found. --- methods/gpgv.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 0d5707e2a..b8d348484 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -436,7 +436,7 @@ string GPGVMethod::VerifyGetSignersWithLegacy(const char *file, const char *outf // We do not have a key file pinned, did not find a good signature, but found // missing keys - let's retry with trusted.gpg - if (keyFiles.empty() && Signers.Good.empty() && !Signers.NoPubKey.empty()) + if (keyFiles.empty() && Signers.Valid.empty() && !Signers.NoPubKey.empty()) { std::vector legacyKeyFiles{_config->FindFile("Dir::Etc::trusted")}; if (legacyKeyFiles[0].empty()) @@ -450,7 +450,7 @@ string GPGVMethod::VerifyGetSignersWithLegacy(const char *file, const char *outf if (_error->PendingError()) return legacyMsg; // Hooray, we found a key apparently, something verified as good or bad - if (!legacySigners.Good.empty() || !legacySigners.Bad.empty()) + if (!legacySigners.Valid.empty() || !legacySigners.Bad.empty()) { std::string warning; strprintf(warning, -- cgit v1.2.3-70-g09d2