From ffdbd2149162f8d1f1877f623a1a0e47ad62ec90 Mon Sep 17 00:00:00 2001 From: Paul Schaub Date: Fri, 11 Mar 2022 18:23:40 +0100 Subject: [PATCH] Implement configuration option for SignerUserId subpacket verification level. By default we ignore SignerUserId subpackets on signatures. This behavior can be changed by calling Policy.setSignerUserIdValidationLevel(). Right now, STRICT and DISABLED are available as options, but it may make sense to implement another option PARTIALLY, which will accept signatures made by key with user-id 'A ' but where the sig contains a signer user id of value 'foo@bar' for example. --- .../java/org/pgpainless/policy/Policy.java | 44 +++++++++++ .../consumer/CertificateValidator.java | 2 +- .../WrongSignerUserIdTest.java | 78 +++++++++++++------ 3 files changed, 99 insertions(+), 25 deletions(-) rename pgpainless-core/src/test/java/{investigations => org/pgpainless/decryption_verification}/WrongSignerUserIdTest.java (69%) diff --git a/pgpainless-core/src/main/java/org/pgpainless/policy/Policy.java b/pgpainless-core/src/main/java/org/pgpainless/policy/Policy.java index 85948546..58d6f6a2 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/policy/Policy.java +++ b/pgpainless-core/src/main/java/org/pgpainless/policy/Policy.java @@ -42,6 +42,25 @@ public final class Policy { private AlgorithmSuite keyGenerationAlgorithmSuite = AlgorithmSuite.getDefaultAlgorithmSuite(); + // Signers User-ID is soon to be deprecated. + private SignerUserIdValidationLevel signerUserIdValidationLevel = SignerUserIdValidationLevel.DISABLED; + + public enum SignerUserIdValidationLevel { + /** + * PGPainless will verify {@link org.bouncycastle.bcpg.sig.SignerUserID} subpackets in signatures strictly. + * This means, that signatures with Signer's User-ID subpackets containing a value that does not match the signer key's + * user-id exactly, will be rejected. + * E.g. Signer's user-id "alice@pgpainless.org", User-ID: "Alice <alice@pgpainless.org>" does not + * match exactly and is therefore rejected. + */ + STRICT, + + /** + * PGPainless will ignore {@link org.bouncycastle.bcpg.sig.SignerUserID} subpackets on signature. + */ + DISABLED + } + Policy() { } @@ -468,4 +487,29 @@ public final class Policy { public void setKeyGenerationAlgorithmSuite(@Nonnull AlgorithmSuite algorithmSuite) { this.keyGenerationAlgorithmSuite = algorithmSuite; } + + /** + * Return the level of validation PGPainless shall do on {@link org.bouncycastle.bcpg.sig.SignerUserID} subpackets. + * By default, this value is {@link SignerUserIdValidationLevel#DISABLED}. + * + * @return the level of validation + */ + public SignerUserIdValidationLevel getSignerUserIdValidationLevel() { + return signerUserIdValidationLevel; + } + + /** + * Specify, how {@link org.bouncycastle.bcpg.sig.SignerUserID} subpackets on signatures shall be validated. + * + * @param signerUserIdValidationLevel level of verification PGPainless shall do on + * {@link org.bouncycastle.bcpg.sig.SignerUserID} subpackets. + * @return policy instance + */ + public Policy setSignerUserIdValidationLevel(SignerUserIdValidationLevel signerUserIdValidationLevel) { + if (signerUserIdValidationLevel == null) { + throw new NullPointerException("SignerUserIdValidationLevel cannot be null."); + } + this.signerUserIdValidationLevel = signerUserIdValidationLevel; + return this; + } } diff --git a/pgpainless-core/src/main/java/org/pgpainless/signature/consumer/CertificateValidator.java b/pgpainless-core/src/main/java/org/pgpainless/signature/consumer/CertificateValidator.java index 93d12fc5..65a1a41d 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/signature/consumer/CertificateValidator.java +++ b/pgpainless-core/src/main/java/org/pgpainless/signature/consumer/CertificateValidator.java @@ -143,7 +143,7 @@ public final class CertificateValidator { // Specific signer user-id SignerUserID signerUserID = SignatureSubpacketsUtil.getSignerUserID(signature); - if (signerUserID != null) { + if (signerUserID != null && policy.getSignerUserIdValidationLevel() == Policy.SignerUserIdValidationLevel.STRICT) { List signerUserIdSigs = userIdSignatures.get(signerUserID.getID()); if (signerUserIdSigs == null || signerUserIdSigs.isEmpty()) { throw new SignatureValidationException("Signature was allegedly made by user-id '" + signerUserID.getID() + diff --git a/pgpainless-core/src/test/java/investigations/WrongSignerUserIdTest.java b/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/WrongSignerUserIdTest.java similarity index 69% rename from pgpainless-core/src/test/java/investigations/WrongSignerUserIdTest.java rename to pgpainless-core/src/test/java/org/pgpainless/decryption_verification/WrongSignerUserIdTest.java index ccccdb2d..b3ecabc8 100644 --- a/pgpainless-core/src/test/java/investigations/WrongSignerUserIdTest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/decryption_verification/WrongSignerUserIdTest.java @@ -2,9 +2,11 @@ // // SPDX-License-Identifier: Apache-2.0 -package investigations; +package org.pgpainless.decryption_verification; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -34,17 +36,17 @@ import org.bouncycastle.openpgp.operator.bc.BcPGPContentSignerBuilder; import org.bouncycastle.openpgp.operator.bc.BcPGPDataEncryptorBuilder; import org.bouncycastle.openpgp.operator.bc.BcPublicKeyKeyEncryptionMethodGenerator; import org.bouncycastle.util.io.Streams; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Test; import org.pgpainless.PGPainless; import org.pgpainless.algorithm.HashAlgorithm; -import org.pgpainless.decryption_verification.ConsumerOptions; -import org.pgpainless.decryption_verification.DecryptionStream; -import org.pgpainless.exception.SignatureValidationException; import org.pgpainless.key.protection.UnlockSecretKey; +import org.pgpainless.policy.Policy; import org.pgpainless.util.Passphrase; public class WrongSignerUserIdTest { - private static final String CERT = "-----BEGIN PGP PRIVATE KEY BLOCK-----\n" + + private static final String KEY = "-----BEGIN PGP PRIVATE KEY BLOCK-----\n" + " Comment: Alice's OpenPGP Transferable Secret Key\n" + " Comment: https://www.ietf.org/id/draft-bre-openpgp-samples-01.html\n" + "\n" + @@ -63,13 +65,54 @@ public class WrongSignerUserIdTest { " -----END PGP PRIVATE KEY BLOCK-----"; private static final String USER_ID = "Alice Lovelace "; - public static void main(String[] args) throws Exception { - WrongSignerUserIdTest test = new WrongSignerUserIdTest(); - test.execute(); + @Test + public void verificationSucceedsWithDisabledCheck() throws PGPException, IOException { + executeTest(false, true); } - public void execute() throws PGPException, IOException { - PGPSecretKeyRing secretKeys = PGPainless.readKeyRing().secretKeyRing(CERT); + @Test + public void verificationFailsWithEnabledCheck() throws PGPException, IOException { + executeTest(true, false); + } + + @AfterAll + public static void resetDefault() { + PGPainless.getPolicy().setSignerUserIdValidationLevel(Policy.SignerUserIdValidationLevel.DISABLED); + } + + public void executeTest(boolean enableCheck, boolean expectSucessfulVerification) throws IOException, PGPException { + PGPainless.getPolicy().setSignerUserIdValidationLevel(enableCheck ? Policy.SignerUserIdValidationLevel.STRICT : Policy.SignerUserIdValidationLevel.DISABLED); + PGPSecretKeyRing secretKeys = PGPainless.readKeyRing().secretKeyRing(KEY); + assertEquals(USER_ID, secretKeys.getPublicKey().getUserIDs().next()); + + String messageWithWrongUserId = generateTestMessage(secretKeys); + verifyTestMessage(messageWithWrongUserId, secretKeys, expectSucessfulVerification); + } + + private void verifyTestMessage(String messageWithWrongUserId, PGPSecretKeyRing secretKeys, boolean expectSuccessfulVerification) throws IOException, PGPException { + PGPPublicKeyRing certificate = PGPainless.extractCertificate(secretKeys); + + DecryptionStream decryptionStream = PGPainless.decryptAndOrVerify().onInputStream( + new ByteArrayInputStream(messageWithWrongUserId.getBytes(StandardCharsets.UTF_8))) + .withOptions(new ConsumerOptions() + .addDecryptionKey(secretKeys) + .addVerificationCert(certificate)); + + ByteArrayOutputStream out = new ByteArrayOutputStream(); + Streams.pipeAll(decryptionStream, out); + + decryptionStream.close(); + OpenPgpMetadata metadata = decryptionStream.getResult(); + + if (expectSuccessfulVerification) { + assertTrue(metadata.isVerified()); + } else { + assertFalse(metadata.isVerified()); + } + + } + + private String generateTestMessage(PGPSecretKeyRing secretKeys) throws PGPException, IOException { PGPPublicKeyRing certificate = PGPainless.extractCertificate(secretKeys); assertEquals(USER_ID, certificate.getPublicKey().getUserIDs().next()); @@ -126,19 +169,6 @@ public class WrongSignerUserIdTest { encStream.close(); armorOut.close(); - try { - DecryptionStream decryptionStream = PGPainless.decryptAndOrVerify().onInputStream( - new ByteArrayInputStream(cipherText.toByteArray())) - .withOptions(new ConsumerOptions() - .addDecryptionKey(secretKeys) - .addVerificationCert(certificate)); - - ByteArrayOutputStream out = new ByteArrayOutputStream(); - Streams.pipeAll(decryptionStream, out); - - decryptionStream.close(); - } catch (SignatureValidationException e) { - // expected - } + return cipherText.toString(); } }