From 9b046a0cf1bb50f352b000204fdc7c3c17f13d36 Mon Sep 17 00:00:00 2001 From: Paul Schaub Date: Thu, 1 Jul 2021 21:33:38 +0200 Subject: [PATCH] Change SymmetricEncryptionAlgorithmNegotiator to return the 'best' avail. alg --- .../SymmetricKeyAlgorithmNegotiator.java | 36 +++++++---- .../java/org/pgpainless/policy/Policy.java | 38 +++++++++--- .../SymmetricKeyAlgorithmNegotiatorTest.java | 62 +++++++++++++++++++ .../EncryptDecryptTest.java | 10 +++ 4 files changed, 124 insertions(+), 22 deletions(-) diff --git a/pgpainless-core/src/main/java/org/pgpainless/algorithm/negotiation/SymmetricKeyAlgorithmNegotiator.java b/pgpainless-core/src/main/java/org/pgpainless/algorithm/negotiation/SymmetricKeyAlgorithmNegotiator.java index e4bcb4dc..62fd65cf 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/algorithm/negotiation/SymmetricKeyAlgorithmNegotiator.java +++ b/pgpainless-core/src/main/java/org/pgpainless/algorithm/negotiation/SymmetricKeyAlgorithmNegotiator.java @@ -17,7 +17,7 @@ package org.pgpainless.algorithm.negotiation; import java.util.ArrayList; import java.util.Collections; -import java.util.Comparator; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -53,8 +53,8 @@ public interface SymmetricKeyAlgorithmNegotiator { return override; } + // Count score (occurrences) of each algorithm Map supportWeight = new LinkedHashMap<>(); - for (Set keyPreferences : preferences) { for (SymmetricKeyAlgorithm preferred : keyPreferences) { if (supportWeight.containsKey(preferred)) { @@ -65,21 +65,33 @@ public interface SymmetricKeyAlgorithmNegotiator { } } - List scoreboard = new ArrayList<>(supportWeight.keySet()); - // Sort scoreboard by descending popularity - Collections.sort(scoreboard, new Comparator() { - @Override - public int compare(SymmetricKeyAlgorithm t0, SymmetricKeyAlgorithm t1) { - return -supportWeight.get(t0).compareTo(supportWeight.get(t1)); + // Pivot the score map + Map> byScore = new HashMap<>(); + for (SymmetricKeyAlgorithm algorithm : supportWeight.keySet()) { + int score = supportWeight.get(algorithm); + List withSameScore = byScore.get(score); + if (withSameScore == null) { + withSameScore = new ArrayList<>(); + byScore.put(score, withSameScore); } - }); + withSameScore.add(algorithm); + } - for (SymmetricKeyAlgorithm mostWanted : scoreboard) { - if (policy.isAcceptable(mostWanted)) { - return mostWanted; + List scores = new ArrayList<>(byScore.keySet()); + + // Sort map and iterate from highest to lowest score + Collections.sort(scores); + for (int i = scores.size() - 1; i >= 0; i--) { + int score = scores.get(i); + List withSameScore = byScore.get(score); + // Select best algorithm + SymmetricKeyAlgorithm best = policy.selectBest(withSameScore); + if (best != null) { + return best; } } + // If no algorithm is acceptable, choose fallback return policy.getDefaultSymmetricKeyAlgorithm(); } }; 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 97ef898a..1c2e8d45 100644 --- a/pgpainless-core/src/main/java/org/pgpainless/policy/Policy.java +++ b/pgpainless-core/src/main/java/org/pgpainless/policy/Policy.java @@ -231,14 +231,14 @@ public final class Policy { public static SymmetricKeyAlgorithmPolicy defaultSymmetricKeyEncryptionAlgorithmPolicy() { return new SymmetricKeyAlgorithmPolicy(SymmetricKeyAlgorithm.AES_256, Arrays.asList( // Reject: Unencrypted, IDEA, TripleDES, CAST5 - SymmetricKeyAlgorithm.BLOWFISH, - SymmetricKeyAlgorithm.AES_128, - SymmetricKeyAlgorithm.AES_192, SymmetricKeyAlgorithm.AES_256, + SymmetricKeyAlgorithm.AES_192, + SymmetricKeyAlgorithm.AES_128, + SymmetricKeyAlgorithm.BLOWFISH, SymmetricKeyAlgorithm.TWOFISH, - SymmetricKeyAlgorithm.CAMELLIA_128, + SymmetricKeyAlgorithm.CAMELLIA_256, SymmetricKeyAlgorithm.CAMELLIA_192, - SymmetricKeyAlgorithm.CAMELLIA_256 + SymmetricKeyAlgorithm.CAMELLIA_128 )); } @@ -251,16 +251,34 @@ public final class Policy { return new SymmetricKeyAlgorithmPolicy(SymmetricKeyAlgorithm.AES_256, Arrays.asList( // Reject: Unencrypted, IDEA, TripleDES SymmetricKeyAlgorithm.CAST5, - SymmetricKeyAlgorithm.BLOWFISH, - SymmetricKeyAlgorithm.AES_128, - SymmetricKeyAlgorithm.AES_192, SymmetricKeyAlgorithm.AES_256, + SymmetricKeyAlgorithm.AES_192, + SymmetricKeyAlgorithm.AES_128, + SymmetricKeyAlgorithm.BLOWFISH, SymmetricKeyAlgorithm.TWOFISH, - SymmetricKeyAlgorithm.CAMELLIA_128, + SymmetricKeyAlgorithm.CAMELLIA_256, SymmetricKeyAlgorithm.CAMELLIA_192, - SymmetricKeyAlgorithm.CAMELLIA_256 + SymmetricKeyAlgorithm.CAMELLIA_128 )); } + + /** + * Select the best acceptable algorithm from the options list. + * The best algorithm is the first algorithm we encounter in our list of acceptable algorithms that + * is also contained in the list of options. + * + * + * @param options list of algorithm options + * @return best + */ + public SymmetricKeyAlgorithm selectBest(List options) { + for (SymmetricKeyAlgorithm acceptable : acceptableSymmetricKeyAlgorithms) { + if (options.contains(acceptable)) { + return acceptable; + } + } + return null; + } } public static final class HashAlgorithmPolicy { diff --git a/pgpainless-core/src/test/java/org/pgpainless/algorithm/SymmetricKeyAlgorithmNegotiatorTest.java b/pgpainless-core/src/test/java/org/pgpainless/algorithm/SymmetricKeyAlgorithmNegotiatorTest.java index 3167f557..6263c193 100644 --- a/pgpainless-core/src/test/java/org/pgpainless/algorithm/SymmetricKeyAlgorithmNegotiatorTest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/algorithm/SymmetricKeyAlgorithmNegotiatorTest.java @@ -76,4 +76,66 @@ public class SymmetricKeyAlgorithmNegotiatorTest { // AES 192 is most popular assertEquals(SymmetricKeyAlgorithm.AES_192, byPopularity.negotiate(policy, null, preferences)); } + + @Test + public void byPopularityIgnoresRejectedAlgorithms() { + List> preferences = new ArrayList<>(); + + preferences.add(new LinkedHashSet(){{ + add(SymmetricKeyAlgorithm.CAMELLIA_128); + add(SymmetricKeyAlgorithm.CAMELLIA_192); // <- rejected + add(SymmetricKeyAlgorithm.AES_256); // <- accepted + }}); + preferences.add(new LinkedHashSet(){{ + add(SymmetricKeyAlgorithm.CAMELLIA_128); + add(SymmetricKeyAlgorithm.CAMELLIA_192); // <- rejected + }}); + preferences.add(new LinkedHashSet(){{ + add(SymmetricKeyAlgorithm.CAMELLIA_192); // <- rejected + add(SymmetricKeyAlgorithm.AES_256); // <- accepted + }}); + + // AES 192 is most popular + assertEquals(SymmetricKeyAlgorithm.AES_256, byPopularity.negotiate(policy, null, preferences)); + } + + @Test + public void byPopularityChoosesFallbackWhenNoAlgIsAcceptable() { + List> preferences = new ArrayList<>(); + + preferences.add(new LinkedHashSet(){{ + add(SymmetricKeyAlgorithm.CAMELLIA_128); + add(SymmetricKeyAlgorithm.CAMELLIA_192); + }}); + preferences.add(new LinkedHashSet(){{ + add(SymmetricKeyAlgorithm.CAMELLIA_128); + add(SymmetricKeyAlgorithm.CAMELLIA_192); + }}); + preferences.add(new LinkedHashSet(){{ + add(SymmetricKeyAlgorithm.CAMELLIA_192); + add(SymmetricKeyAlgorithm.BLOWFISH); + }}); + + // AES 192 is most popular + assertEquals(SymmetricKeyAlgorithm.CAMELLIA_256, byPopularity.negotiate(policy, null, preferences)); + } + + @Test + public void byPopularitySelectsBestOnDraw() { + List> preferences = new ArrayList<>(); + + // Create draw between AES 128 and AES 256 + // The recipients prefer AES 128 first, but we prioritize our policies order + preferences.add(new LinkedHashSet(){{ + add(SymmetricKeyAlgorithm.AES_128); + add(SymmetricKeyAlgorithm.AES_192); + add(SymmetricKeyAlgorithm.AES_256); + }}); + preferences.add(new LinkedHashSet(){{ + add(SymmetricKeyAlgorithm.AES_128); + add(SymmetricKeyAlgorithm.AES_256); + }}); + + assertEquals(SymmetricKeyAlgorithm.AES_256, byPopularity.negotiate(policy, null, preferences)); + } } diff --git a/pgpainless-core/src/test/java/org/pgpainless/encryption_signing/EncryptDecryptTest.java b/pgpainless-core/src/test/java/org/pgpainless/encryption_signing/EncryptDecryptTest.java index 9e8c1bba..50ba10b7 100644 --- a/pgpainless-core/src/test/java/org/pgpainless/encryption_signing/EncryptDecryptTest.java +++ b/pgpainless-core/src/test/java/org/pgpainless/encryption_signing/EncryptDecryptTest.java @@ -35,6 +35,7 @@ import org.bouncycastle.openpgp.PGPPublicKeyRing; import org.bouncycastle.openpgp.PGPSecretKeyRing; import org.bouncycastle.openpgp.PGPSignature; import org.bouncycastle.util.io.Streams; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; @@ -56,6 +57,7 @@ import org.pgpainless.key.generation.type.rsa.RsaLength; import org.pgpainless.key.protection.SecretKeyRingProtector; import org.pgpainless.key.protection.UnprotectedKeysProtector; import org.pgpainless.key.util.KeyRingUtils; +import org.pgpainless.policy.Policy; import org.pgpainless.util.ArmoredOutputStreamFactory; public class EncryptDecryptTest { @@ -71,6 +73,14 @@ public class EncryptDecryptTest { "Unfold the imagined happiness that both\n" + "Receive in either by this dear encounter."; + @BeforeEach + public void setDefaultPolicy() { + PGPainless.getPolicy().setSymmetricKeyEncryptionAlgorithmPolicy( + Policy.SymmetricKeyAlgorithmPolicy.defaultSymmetricKeyEncryptionAlgorithmPolicy()); + PGPainless.getPolicy().setSymmetricKeyDecryptionAlgorithmPolicy( + Policy.SymmetricKeyAlgorithmPolicy.defaultSymmetricKeyDecryptionAlgorithmPolicy()); + } + @ParameterizedTest @MethodSource("org.pgpainless.util.TestUtil#provideImplementationFactories") public void freshKeysRsaToElGamalTest(ImplementationFactory implementationFactory)