From a0a18a88d76df85001b8b1f18808a1d707d0672a Mon Sep 17 00:00:00 2001 From: Gregor Johannson Date: Thu, 21 Feb 2019 21:43:38 +0200 Subject: [PATCH] #153: Fix introduced LGTM alert. Remove DAPProperties setters. Cipher back to Signature. Swap -token to -token-key. --- .../java/pro/javacard/gp/DAPProperties.java | 18 +++---------- ...mentHandler.java => DMTokenGenerator.java} | 23 +++++++++------- .../javacard/gp/GPCommandLineInterface.java | 5 ++-- src/main/java/pro/javacard/gp/GPTool.java | 27 ++++++++++--------- .../java/pro/javacard/gp/GlobalPlatform.java | 13 +++++---- ...Handler.java => TestDMTokenGenerator.java} | 6 ++--- 6 files changed, 44 insertions(+), 48 deletions(-) rename src/main/java/pro/javacard/gp/{DelegatedManagementHandler.java => DMTokenGenerator.java} (80%) rename src/test/java/pro/javacard/gp/{TestDelegatedManagementHandler.java => TestDMTokenGenerator.java} (84%) diff --git a/src/main/java/pro/javacard/gp/DAPProperties.java b/src/main/java/pro/javacard/gp/DAPProperties.java index a1a5ce0e..717727a2 100644 --- a/src/main/java/pro/javacard/gp/DAPProperties.java +++ b/src/main/java/pro/javacard/gp/DAPProperties.java @@ -7,7 +7,6 @@ import static pro.javacard.gp.GPCommandLineInterface.OPT_DAP_DOMAIN; import static pro.javacard.gp.GPCommandLineInterface.OPT_TO; -import static pro.javacard.gp.GPTool.fail; public class DAPProperties { private AID targetDomain = null; @@ -18,6 +17,9 @@ public DAPProperties(OptionSet args, GlobalPlatform gp) throws CardException, GP // Override target and check for DAP if (args.has(OPT_TO)) { targetDomain = AID.fromString(args.valueOf(OPT_TO)); + if (gp.getRegistry().getDomain(targetDomain) == null) { + throw new GPException("Specified target domain is invalid: " + targetDomain); + } if (gp.getRegistry().getDomain(targetDomain).getPrivileges().has(GPRegistryEntry.Privilege.DAPVerification)) required = true; } @@ -33,7 +35,7 @@ public DAPProperties(OptionSet args, GlobalPlatform gp) throws CardException, GP dapDomain = AID.fromString(args.valueOf(OPT_DAP_DOMAIN)); GPRegistryEntry.Privileges p = gp.getRegistry().getDomain(dapDomain).getPrivileges(); if (!(p.has(GPRegistryEntry.Privilege.DAPVerification) || p.has(GPRegistryEntry.Privilege.MandatedDAPVerification))) { - fail("Specified DAP domain does not have (Mandated)DAPVerification privilege: " + p.toString()); + throw new GPException("Specified DAP domain does not have (Mandated)DAPVerification privilege: " + p.toString()); } } } @@ -42,23 +44,11 @@ public AID getTargetDomain() { return targetDomain; } - public void setTargetDomain(AID targetDomain) { - this.targetDomain = targetDomain; - } - public AID getDapDomain() { return dapDomain; } - public void setDapDomain(AID dapDomain) { - this.dapDomain = dapDomain; - } - public boolean isRequired() { return required; } - - public void setRequired(boolean required) { - this.required = required; - } } diff --git a/src/main/java/pro/javacard/gp/DelegatedManagementHandler.java b/src/main/java/pro/javacard/gp/DMTokenGenerator.java similarity index 80% rename from src/main/java/pro/javacard/gp/DelegatedManagementHandler.java rename to src/main/java/pro/javacard/gp/DMTokenGenerator.java index 72180af0..fdf8772e 100644 --- a/src/main/java/pro/javacard/gp/DelegatedManagementHandler.java +++ b/src/main/java/pro/javacard/gp/DMTokenGenerator.java @@ -3,17 +3,18 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import javax.crypto.Cipher; import javax.smartcardio.CommandAPDU; import java.io.ByteArrayOutputStream; import java.security.PrivateKey; +import java.security.Signature; + +public class DMTokenGenerator { + private static final Logger logger = LoggerFactory.getLogger(DMTokenGenerator.class); + private static final String acceptedSignatureAlgorithm = "SHA1withRSA"; -public class DelegatedManagementHandler { - private static final Logger logger = LoggerFactory.getLogger(DelegatedManagementHandler.class); - private static final String acceptedSignatureAlgorithm = "RSA/ECB/PKCS1Padding"; private PrivateKey key; - public DelegatedManagementHandler(PrivateKey key) { + public DMTokenGenerator(PrivateKey key) { this.key = key; } @@ -56,13 +57,17 @@ private static byte[] getTokenData(CommandAPDU apdu) { private static byte[] signData(PrivateKey privateKey, byte[] apduData) { try { - Cipher cipher = Cipher.getInstance(acceptedSignatureAlgorithm); - cipher.init(Cipher.ENCRYPT_MODE, privateKey); - - return cipher.doFinal(apduData); + Signature signature = Signature.getInstance(acceptedSignatureAlgorithm); + signature.initSign(privateKey); + signature.update(apduData); + return signature.sign(); } catch (Exception e) { throw new RuntimeException("Could not create signature with instance " + acceptedSignatureAlgorithm, e); } } + public boolean hasKey() { + return key != null; + } + } diff --git a/src/main/java/pro/javacard/gp/GPCommandLineInterface.java b/src/main/java/pro/javacard/gp/GPCommandLineInterface.java index 3f51afde..cf970ad0 100644 --- a/src/main/java/pro/javacard/gp/GPCommandLineInterface.java +++ b/src/main/java/pro/javacard/gp/GPCommandLineInterface.java @@ -104,7 +104,8 @@ abstract class GPCommandLineInterface { protected final static String OPT_ACR_DELETE = "acr-delete"; protected final static String OPT_ACR_RULE = "acr-rule"; protected final static String OPT_ACR_CERT_HASH = "acr-hash"; - protected final static String OPT_TOKEN = "token"; + protected final static String OPT_TOKEN_KEY = "token-key"; + // TODO - include token "as is" with -token protected static OptionSet parseArguments(String[] argv) throws IOException { OptionSet args = null; @@ -161,7 +162,7 @@ protected static OptionSet parseArguments(String[] argv) throws IOException { parser.accepts(OPT_STORE_DATA_BLOB, "STORE DATA blob").withRequiredArg().describedAs("data"); parser.accepts(OPT_STORE_DATA, "Send STORE DATA commands").withRequiredArg().describedAs("data"); - parser.accepts(OPT_TOKEN, "Path to private key used in Delegated Management token generation").withRequiredArg().describedAs("path"); + parser.accepts(OPT_TOKEN_KEY, "Path to private key used in Delegated Management token generation").withRequiredArg().describedAs("path"); parser.accepts(OPT_MAKE_DEFAULT, "Make AID the default").withRequiredArg().describedAs("AID"); parser.accepts(OPT_RENAME_ISD, "Rename ISD").withRequiredArg().describedAs("new AID"); diff --git a/src/main/java/pro/javacard/gp/GPTool.java b/src/main/java/pro/javacard/gp/GPTool.java index d4c00945..c441dbbc 100644 --- a/src/main/java/pro/javacard/gp/GPTool.java +++ b/src/main/java/pro/javacard/gp/GPTool.java @@ -237,7 +237,11 @@ public static void main(String[] argv) throws Exception { // FIXME: would like to get AID from oracle as well. } - gp.setTokenKey(privateKeyOrNull(args)); + if (args.has(OPT_TOKEN_KEY)) { + gp.setDMTokenGenerator(new DMTokenGenerator(getRSAPrivateKey(args.valueOf(OPT_TOKEN_KEY).toString()))); + } else { + gp.setDMTokenGenerator(new DMTokenGenerator(null)); + } // Don't do sanity checks, just run asked commands if (args.has(OPT_FORCE)) @@ -860,8 +864,6 @@ private static void calculateDapPropertiesAndLoadCap(OptionSet args, GlobalPlatf // Do nothing. Here for findbugs } throw e; - } catch (CardException e) { - throw e; } } @@ -875,18 +877,17 @@ private static void loadCapAccordingToDapRequirement(OptionSet args, GlobalPlatf } } - private static PrivateKey privateKeyOrNull(OptionSet args) { - if (args.has(OPT_TOKEN)) { - try (FileInputStream fin = new FileInputStream(new File(args.valueOf(OPT_TOKEN).toString()))) { - PrivateKey key = GPCrypto.pem2PrivateKey(fin); - if (key instanceof RSAPrivateKey) { - return key; - } - } catch (Exception e) { - throw new RuntimeException("Could not extract RSA private key from supplied path"); + private static PrivateKey getRSAPrivateKey(String path) { + try (FileInputStream fin = new FileInputStream(new File(path))) { + PrivateKey key = GPCrypto.pem2PrivateKey(fin); + if (key instanceof RSAPrivateKey) { + return key; + } else { + throw new RuntimeException("Supplied key at path is not instance of RSAPrivateKey"); } + } catch (Exception e) { + throw new RuntimeException("Could not extract RSAPrivateKey from supplied path"); } - return null; } // FIXME: get rid diff --git a/src/main/java/pro/javacard/gp/GlobalPlatform.java b/src/main/java/pro/javacard/gp/GlobalPlatform.java index 5dea6f68..0d908e66 100644 --- a/src/main/java/pro/javacard/gp/GlobalPlatform.java +++ b/src/main/java/pro/javacard/gp/GlobalPlatform.java @@ -104,7 +104,7 @@ public class GlobalPlatform extends CardChannel implements AutoCloseable { private SecureChannelWrapper wrapper = null; private CardChannel channel; private GPRegistry registry = null; - private PrivateKey tokenKey; + private DMTokenGenerator tokenGenerator; private boolean dirty = true; // True if registry is dirty. /** @@ -235,8 +235,8 @@ public void setSpec(GPSpec spec) { this.spec = spec; } - public void setTokenKey(PrivateKey key) { - this.tokenKey = key; + public void setDMTokenGenerator(DMTokenGenerator tokenGenerator) { + this.tokenGenerator = tokenGenerator; } public AID getAID() { @@ -550,12 +550,11 @@ private ResponseAPDU transmitLV(CommandAPDU command) throws CardException { } private ResponseAPDU transmitDM(CommandAPDU command) throws CardException { - if (tokenKey == null && command.getINS() == INS_DELETE) { + if (!tokenGenerator.hasKey() && command.getINS() == INS_DELETE) { //Only add token bytes to INS_DELETE if key exists for token calculation, since token is optional return transmitLV(command); } - DelegatedManagementHandler dmHandler = new DelegatedManagementHandler(tokenKey); - command = dmHandler.applyToken(command); + command = tokenGenerator.applyToken(command); return transmitLV(command); } @@ -625,7 +624,7 @@ private void loadCapFile(CAPFile cap, AID targetDomain, boolean includeDebug, bo ByteArrayOutputStream loadBlock = new ByteArrayOutputStream(); try { // Add DAP block, if signature present - if (dap != null) { + if (dap != null && dapDomain != null) { loadBlock.write(0xE2); loadBlock.write(GPUtils.encodeLength(dapDomain.getLength() + dap.length + GPUtils.encodeLength(dap.length).length + 3)); // two tags, two lengths FIXME: proper size loadBlock.write(0x4F); diff --git a/src/test/java/pro/javacard/gp/TestDelegatedManagementHandler.java b/src/test/java/pro/javacard/gp/TestDMTokenGenerator.java similarity index 84% rename from src/test/java/pro/javacard/gp/TestDelegatedManagementHandler.java rename to src/test/java/pro/javacard/gp/TestDMTokenGenerator.java index 0cf9e90f..d138fccf 100644 --- a/src/test/java/pro/javacard/gp/TestDelegatedManagementHandler.java +++ b/src/test/java/pro/javacard/gp/TestDMTokenGenerator.java @@ -13,7 +13,7 @@ import static pro.javacard.gp.GlobalPlatform.CLA_GP; import static pro.javacard.gp.GlobalPlatform.INS_INSTALL; -public class TestDelegatedManagementHandler { +public class TestDMTokenGenerator { private PrivateKey key; @@ -29,7 +29,7 @@ public void setUp() { @Test public void testApplyToken() { CommandAPDU command = new CommandAPDU(CLA_GP, INS_INSTALL, 0x02, 0x00, new byte[]{0}); - DelegatedManagementHandler dmHandler = new DelegatedManagementHandler(key); + DMTokenGenerator dmHandler = new DMTokenGenerator(key); command = dmHandler.applyToken(command); Assert.assertTrue(command.getData().length > 1); } @@ -37,7 +37,7 @@ public void testApplyToken() { @Test public void testApplyEmptyToken() { CommandAPDU command = new CommandAPDU(CLA_GP, INS_INSTALL, 0x02, 0x00, new byte[]{0}); - DelegatedManagementHandler dmHandler = new DelegatedManagementHandler(null); + DMTokenGenerator dmHandler = new DMTokenGenerator(null); command = dmHandler.applyToken(command); Assert.assertArrayEquals(command.getData(), new byte[]{0, 0}); }