Follow some of coorus recommendations: better selection of algo whitelist, ignore recipients preferred algos

4 files changed, 53 insertions, 39 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java
index 04013e9ed..b063e1e74 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java
@@ -660,7 +660,6 @@ public abstract class OperationResult implements Parcelable {
         MSG_PSE_COMPRESSING (LogLevel.DEBUG, R.string.msg_pse_compressing),
         MSG_PSE_ENCRYPTING (LogLevel.DEBUG, R.string.msg_pse_encrypting),
         MSG_PSE_ERROR_BAD_PASSPHRASE (LogLevel.ERROR, R.string.msg_pse_error_bad_passphrase),
-        MSG_PSE_ERROR_HASH_ALGO (LogLevel.ERROR, R.string.msg_pse_error_hash_algo),
         MSG_PSE_ERROR_IO (LogLevel.ERROR, R.string.msg_pse_error_io),
         MSG_PSE_ERROR_SIGN_KEY(LogLevel.ERROR, R.string.msg_pse_error_sign_key),
         MSG_PSE_ERROR_KEY_SIGN (LogLevel.ERROR, R.string.msg_pse_error_key_sign),
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
index 4aa05c9f8..23f02ac08 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
@@ -177,16 +177,6 @@ public class CanonicalizedSecretKey extends CanonicalizedPublicKey {
         return true;
     }
 
-    /**
-     * Returns a list of all supported hash algorithms.
-     */
-    public ArrayList<Integer> getSupportedHashAlgorithms() {
-        // TODO: intersection between preferred hash algos of this key and PgpConstants.PREFERRED_HASH_ALGORITHMS
-        // choose best algo
-
-        return new ArrayList<>();
-    }
-
     private PGPContentSignerBuilder getContentSignerBuilder(int hashAlgo,
             Map<ByteBuffer,byte[]> signedHashes) {
         if (mPrivateKeyState == PRIVATE_KEY_STATE_DIVERT_TO_CARD) {
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpConstants.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpConstants.java
index 7c009921d..7162a3366 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpConstants.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpConstants.java
@@ -29,6 +29,8 @@ import java.util.HashSet;
  * - ECC: 224 bit
  * - Symmetric: 3TDEA
  * - Digital Signature (hash A): SHA-224 - SHA-512
+ *
+ * Many decisions are based on https://gist.github.com/coruus/68a8c65571e2b4225a69
  */
 public class PgpConstants {
 
@@ -39,26 +41,36 @@ public class PgpConstants {
 //    }
     // https://tools.ietf.org/html/rfc6637#section-13
 
+    /*
+        PgpDecryptVerify: Secure Algorithms Whitelist
+        all other algorithms will be rejected with OpenPgpDecryptionResult.RESULT_INSECURE
 
-    // PgpDecryptVerify: Secure Algorithms Whitelist
-    // all other algorithms will be rejected with OpenPgpDecryptionResult.RESULT_INSECURE
+        No broken ciphers or ciphers with key length smaller than 128 bit are allowed!
+     */
     public static HashSet<Integer> sSymmetricAlgorithmsWhitelist = new HashSet<>();
     static {
         sSymmetricAlgorithmsWhitelist.add(SymmetricKeyAlgorithmTags.AES_256);
         sSymmetricAlgorithmsWhitelist.add(SymmetricKeyAlgorithmTags.AES_192);
         sSymmetricAlgorithmsWhitelist.add(SymmetricKeyAlgorithmTags.AES_128);
-        sSymmetricAlgorithmsWhitelist.add(SymmetricKeyAlgorithmTags.TWOFISH);
+        sSymmetricAlgorithmsWhitelist.add(SymmetricKeyAlgorithmTags.TWOFISH); // 128 bit
     }
 
     // all other algorithms will be rejected with OpenPgpSignatureResult.RESULT_INVALID_INSECURE
     public static HashSet<Integer> sHashAlgorithmsWhitelist = new HashSet<>();
     static {
-        sHashAlgorithmsWhitelist.add(HashAlgorithmTags.SHA256);
         sHashAlgorithmsWhitelist.add(HashAlgorithmTags.SHA512);
         sHashAlgorithmsWhitelist.add(HashAlgorithmTags.SHA384);
+        /*
+            TODO: SHA256 and SHA224 are still allowed even though
+            coruus advises against it, to enable better backward compatibility
+
+            coruus:
+            Implementations MUST NOT sign SHA-224 hashes. They SHOULD NOT accept signatures over SHA-224 hashes.
+            ((collision resistance of 112-bits))
+            Implementations SHOULD NOT sign SHA-256 hashes. They MUST NOT default to signing SHA-256 hashes.
+         */
+        sHashAlgorithmsWhitelist.add(HashAlgorithmTags.SHA256);
         sHashAlgorithmsWhitelist.add(HashAlgorithmTags.SHA224);
-        sHashAlgorithmsWhitelist.add(HashAlgorithmTags.SHA1);
-        sHashAlgorithmsWhitelist.add(HashAlgorithmTags.RIPEMD160);
     }
 
     /*
@@ -70,15 +82,15 @@ public class PgpConstants {
             SymmetricKeyAlgorithmTags.AES_256,
             SymmetricKeyAlgorithmTags.AES_192,
             SymmetricKeyAlgorithmTags.AES_128,
-            SymmetricKeyAlgorithmTags.TWOFISH
     };
 
-    // NOTE: some implementations do not support SHA512, thus we choose SHA256 as default (Mailvelope?)
+    /*
+        coorus:
+        Implementations SHOULD use SHA-512 for RSA or DSA signatures. They SHOULD NOT use SHA-384.
+        ((cite to affine padding attacks; unproven status of RSA-PKCSv15))
+     */
     public static final int[] PREFERRED_HASH_ALGORITHMS = new int[]{
-            HashAlgorithmTags.SHA256,
             HashAlgorithmTags.SHA512,
-            HashAlgorithmTags.SHA384,
-            HashAlgorithmTags.SHA224,
     };
 
     /*
@@ -89,19 +101,41 @@ public class PgpConstants {
      */
     public static final int[] PREFERRED_COMPRESSION_ALGORITHMS = new int[]{
             CompressionAlgorithmTags.ZIP,
-            CompressionAlgorithmTags.ZLIB,
-            CompressionAlgorithmTags.BZIP2
     };
 
-    public static final int CERTIFY_HASH_ALGO = HashAlgorithmTags.SHA256;
+    public static final int CERTIFY_HASH_ALGO = HashAlgorithmTags.SHA512;
+
 
+    /*
+    Always use AES-256! Ignore the preferred encryption algos of the recipient!
+
+    coorus:
+    Implementations SHOULD ignore the symmetric algorithm preferences of a recipient's public key;
+    in particular, implementations MUST NOT choose an algorithm forbidden by this
+    document because a recipient prefers it.
 
+    NEEDCITE downgrade attacks on TLS, other protocols
+     */
     public static final int DEFAULT_SYMMETRIC_ALGORITHM = SymmetricKeyAlgorithmTags.AES_256;
     public interface OpenKeychainSymmetricKeyAlgorithmTags extends SymmetricKeyAlgorithmTags {
         int USE_DEFAULT = -1;
     }
 
-    public static final int DEFAULT_HASH_ALGORITHM = HashAlgorithmTags.SHA256;
+    /*
+    Always use SHA-512! Ignore the preferred hash algos of the recipient!
+
+    coorus:
+    Implementations MUST ignore the hash algorithm preferences of a recipient when signing
+    a message to a recipient. The difficulty of forging a signature under a given key,
+    using generic attacks on hash functions, is the difficulty of the weakest hash signed by that key.
+
+    Implementations MUST default to using SHA-512 for RSA signatures,
+
+    and either SHA-512 or the matched instance of SHA-2 for ECDSA signatures.
+    TODO: Ed25519
+    CITE: zooko's hash function table CITE: distinguishers on SHA-256
+     */
+    public static final int DEFAULT_HASH_ALGORITHM = HashAlgorithmTags.SHA512;
     public interface OpenKeychainHashAlgorithmTags extends HashAlgorithmTags {
         int USE_DEFAULT = -1;
     }
@@ -130,9 +164,9 @@ public class PgpConstants {
      * OpenKeychain: 0x90
      */
     public static final int SECRET_KEY_ENCRYPTOR_S2K_COUNT = 0x90;
-    public static final int SECRET_KEY_ENCRYPTOR_HASH_ALGO = HashAlgorithmTags.SHA256;
+    public static final int SECRET_KEY_ENCRYPTOR_HASH_ALGO = HashAlgorithmTags.SHA512;
     public static final int SECRET_KEY_ENCRYPTOR_SYMMETRIC_ALGO = SymmetricKeyAlgorithmTags.AES_256;
-    public static final int SECRET_KEY_BINDING_SIGNATURE_HASH_ALGO = HashAlgorithmTags.SHA256;
+    public static final int SECRET_KEY_BINDING_SIGNATURE_HASH_ALGO = HashAlgorithmTags.SHA512;
     // NOTE: only SHA1 is supported for key checksum calculations in OpenPGP,
     // see http://tools.ietf.org/html/rfc488 0#section-5.5.3
     public static final int SECRET_KEY_SIGNATURE_CHECKSUM_HASH_ALGO = HashAlgorithmTags.SHA1;
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java
index a2c5fa30d..8701c27dc 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java
@@ -226,17 +226,10 @@ public class PgpSignEncryptOperation extends BaseOperation {
                 return new PgpSignEncryptResult(PgpSignEncryptResult.RESULT_ERROR, log);
             }
 
-            // Use preferred hash algo
+            // Use requested hash algo
             int requestedAlgorithm = input.getSignatureHashAlgorithm();
-            ArrayList<Integer> supported = signingKey.getSupportedHashAlgorithms();
             if (requestedAlgorithm == PgpConstants.OpenKeychainHashAlgorithmTags.USE_DEFAULT) {
                 input.setSignatureHashAlgorithm(PgpConstants.DEFAULT_HASH_ALGORITHM);
-                // TODO
-                // get most preferred
-//                input.setSignatureHashAlgorithm(supported.get(0));
-            } else if (!supported.contains(requestedAlgorithm)) {
-                log.add(LogType.MSG_PSE_ERROR_HASH_ALGO, indent);
-                return new PgpSignEncryptResult(PgpSignEncryptResult.RESULT_ERROR, log);
             }
         }
         updateProgress(R.string.progress_preparing_streams, 2, 100);
@@ -245,11 +238,9 @@ public class PgpSignEncryptOperation extends BaseOperation {
         PGPEncryptedDataGenerator cPk = null;
         if (enableEncryption) {
 
-            // Use preferred encryption algo
+            // Use requested encryption algo
             int algo = input.getSymmetricEncryptionAlgorithm();
             if (algo == PgpConstants.OpenKeychainSymmetricKeyAlgorithmTags.USE_DEFAULT) {
-                // get most preferred
-                // TODO: get from recipients
                 algo = PgpConstants.DEFAULT_SYMMETRIC_ALGORITHM;
             }
             // has Integrity packet enabled!