Merge branch 'master' of github.com:open-keychain/open-keychain

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java

@@ -25,6 +25,8 @@ import android.net.Uri;
 import android.os.IBinder;
 import android.os.ParcelFileDescriptor;
 import android.os.Parcelable;
+import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
 import android.text.TextUtils;
 
 import org.openintents.openpgp.IOpenPgpService;
@@ -239,10 +241,8 @@ public class OpenPgpService extends RemoteService {
                 PendingIntent.FLAG_CANCEL_CURRENT);
     }
 
-    private Intent signImpl(Intent data, ParcelFileDescriptor input,
+    private Intent signImpl(Intent data, InputStream inputStream,
-                            ParcelFileDescriptor output, boolean cleartextSign) {
+                            OutputStream outputStream, boolean cleartextSign) {
-        InputStream is = null;
-        OutputStream os = null;
         try {
             boolean asciiArmor = cleartextSign || data.getBooleanExtra(OpenPgpApi.EXTRA_REQUEST_ASCII_ARMOR, true);
 
@@ -278,14 +278,13 @@ public class OpenPgpService extends RemoteService {
             }
 
             // Get Input- and OutputStream from ParcelFileDescriptor
-            is = new ParcelFileDescriptor.AutoCloseInputStream(input);
+            if (!cleartextSign) {
-            if (cleartextSign) {
                 // output stream only needed for cleartext signatures,
                 // detached signatures are returned as extra
-                os = new ParcelFileDescriptor.AutoCloseOutputStream(output);
+                outputStream = null;
             }
-            long inputLength = is.available();
+            long inputLength = inputStream.available();
-            InputData inputData = new InputData(is, inputLength);
+            InputData inputData = new InputData(inputStream, inputLength);
 
             CryptoInputParcel inputParcel = CryptoInputParcelCacheService.getCryptoInputParcel(this, data);
             if (inputParcel == null) {
@@ -299,7 +298,7 @@ public class OpenPgpService extends RemoteService {
 
             // execute PGP operation!
             PgpSignEncryptOperation pse = new PgpSignEncryptOperation(this, new ProviderHelper(getContext()), null);
-            PgpSignEncryptResult pgpResult = pse.execute(pseInput, inputParcel, inputData, os);
+            PgpSignEncryptResult pgpResult = pse.execute(pseInput, inputParcel, inputData, outputStream);
 
             if (pgpResult.isPending()) {
 
@@ -331,28 +330,11 @@ public class OpenPgpService extends RemoteService {
                     new OpenPgpError(OpenPgpError.GENERIC_ERROR, e.getMessage()));
             result.putExtra(OpenPgpApi.RESULT_CODE, OpenPgpApi.RESULT_CODE_ERROR);
             return result;
-        } finally {
-            if (is != null) {
-                try {
-                    is.close();
-                } catch (IOException e) {
-                    Log.e(Constants.TAG, "IOException when closing InputStream", e);
-                }
-            }
-            if (os != null) {
-                try {
-                    os.close();
-                } catch (IOException e) {
-                    Log.e(Constants.TAG, "IOException when closing OutputStream", e);
-                }
-            }
         }
     }
 
-    private Intent encryptAndSignImpl(Intent data, ParcelFileDescriptor input,
+    private Intent encryptAndSignImpl(Intent data, InputStream inputStream,
-                                      ParcelFileDescriptor output, boolean sign) {
+                                      OutputStream outputStream, boolean sign) {
-        InputStream is = null;
-        OutputStream os = null;
         try {
             boolean asciiArmor = data.getBooleanExtra(OpenPgpApi.EXTRA_REQUEST_ASCII_ARMOR, true);
             String originalFilename = data.getStringExtra(OpenPgpApi.EXTRA_ORIGINAL_FILENAME);
@@ -384,13 +366,9 @@ public class OpenPgpService extends RemoteService {
                 }
             }
 
-            // build InputData and write into OutputStream
+            // TODO this is not correct!
-            // Get Input- and OutputStream from ParcelFileDescriptor
+            long inputLength = inputStream.available();
-            is = new ParcelFileDescriptor.AutoCloseInputStream(input);
+            InputData inputData = new InputData(inputStream, inputLength, originalFilename);
-            os = new ParcelFileDescriptor.AutoCloseOutputStream(output);
-
-            long inputLength = is.available();
-            InputData inputData = new InputData(is, inputLength, originalFilename);
 
             PgpSignEncryptInputParcel pseInput = new PgpSignEncryptInputParcel();
             pseInput.setEnableAsciiArmorOutput(asciiArmor)
@@ -456,7 +434,7 @@ public class OpenPgpService extends RemoteService {
             PgpSignEncryptOperation op = new PgpSignEncryptOperation(this, new ProviderHelper(getContext()), null);
 
             // execute PGP operation!
-            PgpSignEncryptResult pgpResult = op.execute(pseInput, inputParcel, inputData, os);
+            PgpSignEncryptResult pgpResult = op.execute(pseInput, inputParcel, inputData, outputStream);
 
             if (pgpResult.isPending()) {
                 RequiredInputParcel requiredInput = pgpResult.getRequiredInputParcel();
@@ -483,37 +461,15 @@ public class OpenPgpService extends RemoteService {
                     new OpenPgpError(OpenPgpError.GENERIC_ERROR, e.getMessage()));
             result.putExtra(OpenPgpApi.RESULT_CODE, OpenPgpApi.RESULT_CODE_ERROR);
             return result;
-        } finally {
-            if (is != null) {
-                try {
-                    is.close();
-                } catch (IOException e) {
-                    Log.e(Constants.TAG, "IOException when closing InputStream", e);
-                }
-            }
-            if (os != null) {
-                try {
-                    os.close();
-                } catch (IOException e) {
-                    Log.e(Constants.TAG, "IOException when closing OutputStream", e);
-                }
-            }
         }
     }
 
-    private Intent decryptAndVerifyImpl(Intent data, ParcelFileDescriptor inputDescriptor,
+    private Intent decryptAndVerifyImpl(Intent data, InputStream inputStream,
-                                        ParcelFileDescriptor output, boolean decryptMetadataOnly) {
+                                        OutputStream outputStream, boolean decryptMetadataOnly) {
-        InputStream inputStream = null;
-        OutputStream outputStream = null;
         try {
-            // Get Input- and OutputStream from ParcelFileDescriptor
-            inputStream = new ParcelFileDescriptor.AutoCloseInputStream(inputDescriptor);
-
             // output is optional, e.g., for verifying detached signatures
-            if (decryptMetadataOnly || output == null) {
+            if (decryptMetadataOnly) {
                 outputStream = null;
-            } else {
-                outputStream = new ParcelFileDescriptor.AutoCloseOutputStream(output);
             }
 
             String currentPkg = getCurrentCallingPackage();
@@ -539,6 +495,7 @@ public class OpenPgpService extends RemoteService {
 
             PgpDecryptVerifyOperation op = new PgpDecryptVerifyOperation(this, mProviderHelper, null);
 
+            // TODO this is not correct!
             long inputLength = inputStream.available();
             InputData inputData = new InputData(inputStream, inputLength);
 
@@ -605,6 +562,7 @@ public class OpenPgpService extends RemoteService {
                         // case RESULT_NOT_ENCRYPTED, but a signature, fallback to deprecated signatureOnly variable
                         if (decryptionResult.getResult() == OpenPgpDecryptionResult.RESULT_NOT_ENCRYPTED
                                 && signatureResult.getResult() != OpenPgpSignatureResult.RESULT_NO_SIGNATURE) {
+                            // noinspection deprecation, TODO
                             signatureResult.setSignatureOnly(true);
                         }
 
@@ -666,33 +624,14 @@ public class OpenPgpService extends RemoteService {
             result.putExtra(OpenPgpApi.RESULT_ERROR, new OpenPgpError(OpenPgpError.GENERIC_ERROR, e.getMessage()));
             result.putExtra(OpenPgpApi.RESULT_CODE, OpenPgpApi.RESULT_CODE_ERROR);
             return result;
-        } finally {
-            if (inputStream != null) {
-                try {
-                    inputStream.close();
-                } catch (IOException e) {
-                    Log.e(Constants.TAG, "IOException when closing InputStream", e);
-                }
-            }
-            if (outputStream != null) {
-                try {
-                    outputStream.close();
-                } catch (IOException e) {
-                    Log.e(Constants.TAG, "IOException when closing OutputStream", e);
-                }
-            }
         }
 
     }
 
-    private Intent getKeyImpl(Intent data, ParcelFileDescriptor output) {
+    private Intent getKeyImpl(Intent data, OutputStream outputStream) {
         try {
             long masterKeyId = data.getLongExtra(OpenPgpApi.EXTRA_KEY_ID, 0);
 
-            // output is optional, for getting the key
-            OutputStream outputStream =
-                    (output != null) ? new ParcelFileDescriptor.AutoCloseOutputStream(output) : null;
-
             try {
                 // try to find key, throws NotFoundException if not in db!
                 CanonicalizedPublicKeyRing keyRing =
@@ -846,7 +785,7 @@ public class OpenPgpService extends RemoteService {
             OpenPgpError error = new OpenPgpError
                     (OpenPgpError.INCOMPATIBLE_API_VERSIONS, "Incompatible API versions!\n"
                             + "used API version: " + data.getIntExtra(OpenPgpApi.EXTRA_API_VERSION, -1) + "\n"
-                            + "supported API versions: " + supportedVersions.toString());
+                            + "supported API versions: " + supportedVersions);
             result.putExtra(OpenPgpApi.RESULT_ERROR, error);
             result.putExtra(OpenPgpApi.RESULT_CODE, OpenPgpApi.RESULT_CODE_ERROR);
             return result;
@@ -875,68 +814,88 @@ public class OpenPgpService extends RemoteService {
         return mBinder;
     }
 
+    @Nullable
+    protected Intent executeInternal(
+            @NonNull Intent data,
+            @Nullable ParcelFileDescriptor input,
+            @Nullable ParcelFileDescriptor output) {
+
+        OutputStream outputStream =
+                (output != null) ? new ParcelFileDescriptor.AutoCloseOutputStream(output) : null;
+        InputStream inputStream =
+                (input != null) ? new ParcelFileDescriptor.AutoCloseInputStream(input) : null;
 
-    protected Intent executeInternal(Intent data, ParcelFileDescriptor input, ParcelFileDescriptor output) {
         try {
-            Intent errorResult = checkRequirements(data);
+            return executeInternalWithStreams(data, inputStream, outputStream);
-            if (errorResult != null) {
-                return errorResult;
-            }
-
-            String action = data.getAction();
-            switch (action) {
-                case OpenPgpApi.ACTION_CLEARTEXT_SIGN: {
-                    return signImpl(data, input, output, true);
-                }
-                case OpenPgpApi.ACTION_SIGN: {
-                    // DEPRECATED: same as ACTION_CLEARTEXT_SIGN
-                    Log.w(Constants.TAG, "You are using a deprecated API call, please use ACTION_CLEARTEXT_SIGN instead of ACTION_SIGN!");
-                    return signImpl(data, input, output, true);
-                }
-                case OpenPgpApi.ACTION_DETACHED_SIGN: {
-                    return signImpl(data, input, output, false);
-                }
-                case OpenPgpApi.ACTION_ENCRYPT: {
-                    return encryptAndSignImpl(data, input, output, false);
-                }
-                case OpenPgpApi.ACTION_SIGN_AND_ENCRYPT: {
-                    return encryptAndSignImpl(data, input, output, true);
-                }
-                case OpenPgpApi.ACTION_DECRYPT_VERIFY: {
-                    return decryptAndVerifyImpl(data, input, output, false);
-                }
-                case OpenPgpApi.ACTION_DECRYPT_METADATA: {
-                    return decryptAndVerifyImpl(data, input, output, true);
-                }
-                case OpenPgpApi.ACTION_GET_SIGN_KEY_ID: {
-                    return getSignKeyIdImpl(data);
-                }
-                case OpenPgpApi.ACTION_GET_KEY_IDS: {
-                    return getKeyIdsImpl(data);
-                }
-                case OpenPgpApi.ACTION_GET_KEY: {
-                    return getKeyImpl(data, output);
-                }
-                default: {
-                    return null;
-                }
-            }
         } finally {
             // always close input and output file descriptors even in error cases
-            if (input != null) {
+            if (inputStream != null) {
                 try {
-                    input.close();
+                    inputStream.close();
                 } catch (IOException e) {
                     Log.e(Constants.TAG, "IOException when closing input ParcelFileDescriptor", e);
                 }
             }
-            if (output != null) {
+            if (outputStream != null) {
                 try {
-                    output.close();
+                    outputStream.close();
                 } catch (IOException e) {
                     Log.e(Constants.TAG, "IOException when closing output ParcelFileDescriptor", e);
                 }
             }
         }
     }
+
+    @Nullable
+    protected Intent executeInternalWithStreams(
+            @NonNull Intent data,
+            @Nullable InputStream inputStream,
+            @Nullable OutputStream outputStream) {
+
+        Intent errorResult = checkRequirements(data);
+        if (errorResult != null) {
+            return errorResult;
+        }
+
+        String action = data.getAction();
+        switch (action) {
+            case OpenPgpApi.ACTION_CLEARTEXT_SIGN: {
+                return signImpl(data, inputStream, outputStream, true);
+            }
+            case OpenPgpApi.ACTION_SIGN: {
+                // DEPRECATED: same as ACTION_CLEARTEXT_SIGN
+                Log.w(Constants.TAG, "You are using a deprecated API call, please use ACTION_CLEARTEXT_SIGN instead of ACTION_SIGN!");
+                return signImpl(data, inputStream, outputStream, true);
+            }
+            case OpenPgpApi.ACTION_DETACHED_SIGN: {
+                return signImpl(data, inputStream, outputStream, false);
+            }
+            case OpenPgpApi.ACTION_ENCRYPT: {
+                return encryptAndSignImpl(data, inputStream, outputStream, false);
+            }
+            case OpenPgpApi.ACTION_SIGN_AND_ENCRYPT: {
+                return encryptAndSignImpl(data, inputStream, outputStream, true);
+            }
+            case OpenPgpApi.ACTION_DECRYPT_VERIFY: {
+                return decryptAndVerifyImpl(data, inputStream, outputStream, false);
+            }
+            case OpenPgpApi.ACTION_DECRYPT_METADATA: {
+                return decryptAndVerifyImpl(data, inputStream, outputStream, true);
+            }
+            case OpenPgpApi.ACTION_GET_SIGN_KEY_ID: {
+                return getSignKeyIdImpl(data);
+            }
+            case OpenPgpApi.ACTION_GET_KEY_IDS: {
+                return getKeyIdsImpl(data);
+            }
+            case OpenPgpApi.ACTION_GET_KEY: {
+                return getKeyImpl(data, outputStream);
+            }
+            default: {
+                return null;
+            }
+        }
+
+    }
+
 }

OpenKeychain/src/test/java/org/sufficientlysecure/keychain/pgp/PgpEncryptDecryptTest.java

@@ -46,6 +46,7 @@ import org.spongycastle.bcpg.PacketTags;
 import org.spongycastle.bcpg.PublicKeyEncSessionPacket;
 import org.spongycastle.bcpg.sig.KeyFlags;
 import org.spongycastle.jce.provider.BouncyCastleProvider;
+import org.spongycastle.openpgp.PGPKeyFlags;
 import org.sufficientlysecure.keychain.WorkaroundBuildConfig;
 import org.sufficientlysecure.keychain.operations.results.DecryptVerifyResult;
 import org.sufficientlysecure.keychain.operations.results.OperationResult.LogType;
@@ -56,6 +57,7 @@ import org.sufficientlysecure.keychain.provider.ProviderHelper;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel.Algorithm;
 import org.sufficientlysecure.keychain.service.SaveKeyringParcel.ChangeUnlockParcel;
+import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyChange;
 import org.sufficientlysecure.keychain.service.input.CryptoInputParcel;
 import org.sufficientlysecure.keychain.service.input.RequiredInputParcel.RequiredInputType;
 import org.sufficientlysecure.keychain.support.KeyringTestingHelper;
@@ -558,8 +560,10 @@ public class PgpEncryptDecryptTest {
 
         String plaintext = "dies ist ein plaintext ☭" + TestingUtils.genPassphrase(true);
 
+        byte[] ciphertext;
+        long encKeyId1;
+
         { // encrypt data with key
-            byte[] ciphertext;
 
             ByteArrayOutputStream out = new ByteArrayOutputStream();
             ByteArrayInputStream in = new ByteArrayInputStream(plaintext.getBytes());
@@ -589,7 +593,7 @@ public class PgpEncryptDecryptTest {
             Packet p;
             p = new BCPGInputStream(new ByteArrayInputStream(enc1.buf)).readPacket();
             Assert.assertTrue("first packet must be session packet", p instanceof PublicKeyEncSessionPacket);
-            long encKeyId1 = ((PublicKeyEncSessionPacket) p).getKeyID();
+            encKeyId1 = ((PublicKeyEncSessionPacket) p).getKeyID();
 
             p = new BCPGInputStream(new ByteArrayInputStream(enc2.buf)).readPacket();
             Assert.assertTrue("second packet must be session packet", p instanceof PublicKeyEncSessionPacket);
@@ -604,6 +608,48 @@ public class PgpEncryptDecryptTest {
 
         }
 
+        { // strip first encrypted subkey, decryption should skip it
+
+            SaveKeyringParcel parcel = new SaveKeyringParcel(mStaticRing1.getMasterKeyId(), mStaticRing1.getFingerprint());
+            parcel.mChangeSubKeys.add(new SubkeyChange(encKeyId1, true, false));
+            UncachedKeyRing modified = PgpKeyOperationTest.applyModificationWithChecks(parcel, mStaticRing1,
+                    new ArrayList<RawPacket>(), new ArrayList<RawPacket>(),
+                    new CryptoInputParcel(new Date(), mKeyPhrase1));
+
+            ProviderHelper providerHelper = new ProviderHelper(RuntimeEnvironment.application);
+            providerHelper.saveSecretKeyRing(modified, new ProgressScaler());
+
+            PgpDecryptVerifyOperation op = new PgpDecryptVerifyOperation(RuntimeEnvironment.application,
+                    new ProviderHelper(RuntimeEnvironment.application), null);
+            PgpDecryptVerifyInputParcel input = new PgpDecryptVerifyInputParcel(ciphertext);
+            DecryptVerifyResult result = op.execute(input, new CryptoInputParcel(mKeyPhrase1));
+
+            Assert.assertTrue("decryption must succeed", result.success());
+            Assert.assertTrue("decryption must have skipped first key",
+                    result.getLog().containsType(LogType.MSG_DC_ASKIP_UNAVAILABLE));
+        }
+
+        { // change flags of second encrypted subkey, decryption should skip it
+
+            SaveKeyringParcel parcel = new SaveKeyringParcel(mStaticRing1.getMasterKeyId(), mStaticRing1.getFingerprint());
+            parcel.mChangeSubKeys.add(new SubkeyChange(encKeyId1, PGPKeyFlags.CAN_CERTIFY, null));
+            UncachedKeyRing modified = PgpKeyOperationTest.applyModificationWithChecks(parcel, mStaticRing1,
+                    new ArrayList<RawPacket>(), new ArrayList<RawPacket>(),
+                    new CryptoInputParcel(new Date(), mKeyPhrase1));
+
+            ProviderHelper providerHelper = new ProviderHelper(RuntimeEnvironment.application);
+            providerHelper.saveSecretKeyRing(modified, new ProgressScaler());
+
+            PgpDecryptVerifyOperation op = new PgpDecryptVerifyOperation(RuntimeEnvironment.application,
+                    new ProviderHelper(RuntimeEnvironment.application), null);
+            PgpDecryptVerifyInputParcel input = new PgpDecryptVerifyInputParcel(ciphertext);
+            DecryptVerifyResult result = op.execute(input, new CryptoInputParcel(mKeyPhrase1));
+
+            Assert.assertTrue("decryption must succeed", result.success());
+            Assert.assertTrue("decryption must have skipped first key",
+                    result.getLog().containsType(LogType.MSG_DC_ASKIP_BAD_FLAGS));
+        }
+
         { // revoke first encryption subkey of keyring in database
             SaveKeyringParcel parcel = new SaveKeyringParcel(mStaticRing1.getMasterKeyId(), mStaticRing1.getFingerprint());
             parcel.mRevokeSubKeys.add(KeyringTestingHelper.getSubkeyId(mStaticRing1, 2));
@@ -616,7 +662,6 @@ public class PgpEncryptDecryptTest {
         }
 
         { // encrypt to this keyring, make sure it's not encrypted to the revoked subkey
-            byte[] ciphertext;
 
             ByteArrayOutputStream out = new ByteArrayOutputStream();
             ByteArrayInputStream in = new ByteArrayInputStream(plaintext.getBytes());