From e57c111010b61eb19f4a3788cbf330d654db7b73 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 19 Jan 2018 18:00:29 +0100 Subject: [PATCH 1/3] extract getRsaOperationPayload method --- .../operations/PsoDecryptTokenOp.java | 24 ++++++++++++------- .../SecurityTokenConnectionCompatTest.java | 13 +++++----- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/operations/PsoDecryptTokenOp.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/operations/PsoDecryptTokenOp.java index 3a2a8307d..4fde33d14 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/operations/PsoDecryptTokenOp.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/operations/PsoDecryptTokenOp.java @@ -25,6 +25,7 @@ import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; import android.support.annotation.NonNull; +import android.support.annotation.VisibleForTesting; import javax.crypto.Cipher; import javax.crypto.NoSuchPaddingException; @@ -43,7 +44,6 @@ import org.sufficientlysecure.keychain.securitytoken.CardException; import org.sufficientlysecure.keychain.securitytoken.CommandApdu; import org.sufficientlysecure.keychain.securitytoken.ECKeyFormat; import org.sufficientlysecure.keychain.securitytoken.KeyFormat; -import org.sufficientlysecure.keychain.securitytoken.KeyType; import org.sufficientlysecure.keychain.securitytoken.ResponseApdu; import org.sufficientlysecure.keychain.securitytoken.SecurityTokenConnection; @@ -84,14 +84,7 @@ public class PsoDecryptTokenOp { } private byte[] decryptSessionKeyRsa(byte[] encryptedSessionKeyMpi) throws IOException { - int mpiLength = getMpiLength(encryptedSessionKeyMpi); - if (mpiLength != encryptedSessionKeyMpi.length - 2) { - throw new IOException("Malformed RSA session key!"); - } - - byte[] psoDecipherPayload = new byte[mpiLength + 1]; - psoDecipherPayload[0] = (byte) 0x00; // RSA Padding Indicator Byte - System.arraycopy(encryptedSessionKeyMpi, 2, psoDecipherPayload, 1, mpiLength); + byte[] psoDecipherPayload = getRsaOperationPayload(encryptedSessionKeyMpi); CommandApdu command = connection.getCommandFactory().createDecipherCommand(psoDecipherPayload); ResponseApdu response = connection.communicate(command); @@ -103,6 +96,19 @@ public class PsoDecryptTokenOp { return response.getData(); } + @VisibleForTesting + public byte[] getRsaOperationPayload(byte[] encryptedSessionKeyMpi) throws IOException { + int mpiLength = getMpiLength(encryptedSessionKeyMpi); + if (mpiLength != encryptedSessionKeyMpi.length - 2) { + throw new IOException("Malformed RSA session key!"); + } + + byte[] psoDecipherPayload = new byte[mpiLength + 1]; + psoDecipherPayload[0] = 0x00; // RSA Padding Indicator Byte + System.arraycopy(encryptedSessionKeyMpi, 2, psoDecipherPayload, 1, mpiLength); + return psoDecipherPayload; + } + private byte[] decryptSessionKeyEcdh(byte[] encryptedSessionKeyMpi, ECKeyFormat eckf, CanonicalizedPublicKey publicKey) throws IOException { int mpiLength = getMpiLength(encryptedSessionKeyMpi); diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnectionCompatTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnectionCompatTest.java index fc368dc00..c5289f32e 100644 --- a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnectionCompatTest.java +++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnectionCompatTest.java @@ -19,7 +19,6 @@ package org.sufficientlysecure.keychain.securitytoken; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import org.bouncycastle.util.encoders.Hex; @@ -28,8 +27,10 @@ import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.sufficientlysecure.keychain.KeychainTestRunner; +import org.sufficientlysecure.keychain.securitytoken.operations.PsoDecryptTokenOp; import static junit.framework.Assert.assertEquals; +import static org.mockito.Mockito.mock; @RunWith(KeychainTestRunner.class) @@ -52,7 +53,7 @@ public class SecurityTokenConnectionCompatTest { */ @Test - public void testPrePostEquals() { + public void testPrePostEquals() throws Exception { List preApdus = decryptPre_ee8cd38(); List postApdus = decryptNow(); @@ -80,11 +81,9 @@ public class SecurityTokenConnectionCompatTest { return apduData; } - public List decryptNow() { - int mpiLength = ((((encryptedSessionKey[0] & 0xff) << 8) + (encryptedSessionKey[1] & 0xff)) + 7) / 8; - byte[] psoDecipherPayload = new byte[mpiLength + 1]; - psoDecipherPayload[0] = (byte) 0x00; - System.arraycopy(encryptedSessionKey, 2, psoDecipherPayload, 1, mpiLength); + public List decryptNow() throws Exception { + PsoDecryptTokenOp psoDecryptTokenOp = PsoDecryptTokenOp.create(mock(SecurityTokenConnection.class)); + byte[] psoDecipherPayload = psoDecryptTokenOp.getRsaOperationPayload(encryptedSessionKey); CommandApdu command = openPgpCommandApduFactory.createDecipherCommand(psoDecipherPayload); List chainedApdus = openPgpCommandApduFactory.createChainedApdus(command); From bccc20ea322ab0ef35f8f115e2f9bc0a92897ae8 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 19 Jan 2018 17:58:34 +0100 Subject: [PATCH 2/3] Reduce max apdu length, for compatibility --- .../keychain/securitytoken/OpenPgpCommandApduFactory.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java index 5fa4f93e9..c3a364e1a 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java @@ -28,7 +28,9 @@ import org.bouncycastle.util.encoders.Hex; public class OpenPgpCommandApduFactory { - private static final int MAX_APDU_NC = 255; + // The spec allows 255, but for compatibility with non-compliant tokens we use 254 here + // See https://github.com/open-keychain/open-keychain/issues/2049 + private static final int MAX_APDU_NC = 254; private static final int MAX_APDU_NC_EXT = 65535; private static final int MAX_APDU_NE = 256; From 1c8cc99c681a435254a1326949da33a88a532b57 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 19 Jan 2018 17:58:53 +0100 Subject: [PATCH 3/3] Don't send NE value for decryption This is slightly more compliant to spec. OpenPGP-Applet implementations I've looked at don't seem to care, but for some reason this still improves compatibility. See https://github.com/open-keychain/open-keychain/issues/2049 --- .../keychain/securitytoken/OpenPgpCommandApduFactory.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java index c3a364e1a..1ffbb72f7 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java @@ -136,8 +136,7 @@ public class OpenPgpCommandApduFactory { @NonNull public CommandApdu createDecipherCommand(byte[] data) { - return CommandApdu.create(CLA, INS_PERFORM_SECURITY_OPERATION, P1_PSO_DECIPHER, P2_PSO_DECIPHER, data, - MAX_APDU_NE_EXT); + return CommandApdu.create(CLA, INS_PERFORM_SECURITY_OPERATION, P1_PSO_DECIPHER, P2_PSO_DECIPHER, data); } @NonNull