From 013d07a0791e790793246cdc5884e682b44a11be Mon Sep 17 00:00:00 2001 From: Michele Balistreri Date: Wed, 28 Nov 2018 17:22:49 +0300 Subject: [PATCH 1/7] redesign export key command --- APPLICATION.MD | 26 ++++---- build.gradle | 2 +- .../java/im/status/wallet/WalletApplet.java | 64 ++++++++++++++----- .../im/status/wallet/WalletAppletTest.java | 54 ++++------------ 4 files changed, 77 insertions(+), 69 deletions(-) diff --git a/APPLICATION.MD b/APPLICATION.MD index 3dd2fec..17bdbdb 100644 --- a/APPLICATION.MD +++ b/APPLICATION.MD @@ -6,10 +6,12 @@ Version numbers are in the form major.minor. An major revision increment indicat compared to the previous released version. This is version 2.0 of the specs (unreleased). ### Changes since 1.2 +* **BREAKING** Completely redefined the EXPORT KEY command * **BREAKING** Removed assisted key derivation * **BREAKING** Removed plain data signing, now only 32-byte long hashes can be signed * Added internal key generation (GENERATE KEY) * Added the ability to customize the NDEF response (SET NDEF) +* Added DUPLICATE KEY command ## Overview @@ -410,16 +412,18 @@ will work even if no PIN authentication has been performed. An empty sequence me * CLA = 0x80 * INS = 0xC2 -* P1 = key path index +* P1 = derivation options * P2 = export options * Response SW = 0x9000 on success, 0x6A86 if P1 or P2 are wrong +* Data = a sequence of 32-bit integers (empty if P1=0x00) * Response Data = key pair template -* Preconditions: Secure Channel must be opened, user PIN must be verified, the current key path must match the one of - the key selected through P1 +* Response SW = 0x9000 on success, 0x6985 if the private key cannot be exported, 0x6A80 if the path is malformed +* Preconditions: Secure Channel must be opened, user PIN must be verified P1: -0x00 = Any key -0x01 = Non-wallet key +0x00 = Current key +0x01 = Derive +0x02 = Derive and make current P2: 0x00 = private and public key @@ -430,10 +434,10 @@ Response Data format: - Tag 0x80 = ECC public key component - Tag 0x81 = ECC private key component (if P2=0x00) -This command exports the current public and private key if and only if the current key path matches the conditions -dictated by the given P1 parameter. This currently allows exporting any key whose last path component has a key index -with the two most significant bits set (that is, matches the 0xc0000000 mask). No key in this range should be used for -wallet accounts, even as intermediate path component. These keys are meant for client-specific use such as the Whisper key. +This command exports the requested public and private key. The public key can be always exported (P2=0x01), but the +private key (P2=0x00) can be exported if and only if the requested key path is in the +[EIP-1581](https://eips.ethereum.org/EIPS/eip-1581) subtree. -The special index 0x00 indicates any path, which means the current key will always be exported regardless of its actual -path. This works however only in combination with P2=0x01, so only the public key will be exported. +The P1 parameter indicates how to the derive the desired key. P1 = 0x00 indicates that the current key must be exported, +and no derivation will be performed. P1 = 0x01 derives the path given in the data field without changing the current +path of the card. P1 = 0x02 derives the path but also changes the current path of the card. \ No newline at end of file diff --git a/build.gradle b/build.gradle index 633f862..8d6915e 100644 --- a/build.gradle +++ b/build.gradle @@ -42,7 +42,7 @@ dependencies { testCompile('org.web3j:core:2.3.1') testCompile('org.bitcoinj:bitcoinj-core:0.14.5') testCompile("org.bouncycastle:bcprov-jdk15on:1.58") - testCompile("com.github.status-im:hardwallet-lite-sdk:f64cefd") + testCompile("com.github.status-im:hardwallet-lite-sdk:75d9f54") testCompile("org.junit.jupiter:junit-jupiter-api:5.1.1") testRuntime("org.junit.jupiter:junit-jupiter-engine:5.1.1") } diff --git a/src/main/java/im/status/wallet/WalletApplet.java b/src/main/java/im/status/wallet/WalletApplet.java index 60b7e45..2c18853 100644 --- a/src/main/java/im/status/wallet/WalletApplet.java +++ b/src/main/java/im/status/wallet/WalletApplet.java @@ -63,8 +63,9 @@ public class WalletApplet extends Applet { static final byte DUPLICATE_KEY_P1_EXPORT = 0x02; static final byte DUPLICATE_KEY_P1_IMPORT = 0x03; - static final byte EXPORT_KEY_P1_ANY = 0x00; - static final byte EXPORT_KEY_P1_HIGH = 0x01; + static final byte EXPORT_KEY_P1_CURRENT = 0x00; + static final byte EXPORT_KEY_P1_DERIVE = 0x01; + static final byte EXPORT_KEY_P1_DERIVE_AND_MAKE_CURRENT = 0x02; static final byte EXPORT_KEY_P2_PRIVATE_AND_PUBLIC = 0x00; static final byte EXPORT_KEY_P2_PUBLIC_ONLY = 0x01; @@ -84,7 +85,7 @@ public class WalletApplet extends Applet { static final byte TLV_UID = (byte) 0x8F; static final byte TLV_KEY_UID = (byte) 0x8E; - private static final byte EXPORT_KEY_HIGH_MASK = (byte) 0xc0; + static final byte[] EIP_1581_PREFIX = { (byte) 0x80, 0x00, 0x00, 0x2B, (byte) 0x80, 0x00, 0x00, 0x3C, (byte) 0x80, 0x00, 0x06, 0x2D}; private OwnerPIN pin; private OwnerPIN puk; @@ -749,13 +750,28 @@ public class WalletApplet extends Applet { byte[] apduBuffer = apdu.getBuffer(); short len = secureChannel.preprocessAPDU(apduBuffer); - if (!((pin.isValidated() || (pinlessPathLen > 0)) && isExtended)) { + if (!((pin.isValidated() || (pinlessPathLen > 0)))) { ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); } boolean isReset = apduBuffer[ISO7816.OFFSET_P1] == DERIVE_P1_SOURCE_MASTER; boolean fromParent = apduBuffer[ISO7816.OFFSET_P1] == DERIVE_P1_SOURCE_PARENT; + doDerive(apduBuffer, len, isReset, fromParent); + } + + /** + * Internal derivation function, called by DERIVE KEY and EXPORT KEY + * @param apduBuffer the APDU buffer + * @param len the APDU len + * @param isReset start deriving from master + * @param fromParent start deriving from parent + */ + private void doDerive(byte[] apduBuffer, short len, boolean isReset, boolean fromParent) { + if (!isExtended) { + ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); + } + if (fromParent && !parentValid) { ISOException.throwIt(ISO7816.SW_WRONG_P1P2); } @@ -1138,16 +1154,13 @@ public class WalletApplet extends Applet { } /** - * Processes the EXPORT KEY command. Requires an open secure channel and the PIN to be verified. The P1 parameter is - * an index to which key must be exported from the list of exportable ones. At the moment only the Whisper key with - * key path m/1/1 is exportable. The key is exported only if the current key path matches the key path of the key to - * be exported. The public key of the current path can always be exported with P1=0x00 and P2=0x01. + * Processes the EXPORT KEY command. Requires an open secure channel and the PIN to be verified. * * @param apdu the JCRE-owned APDU object. */ private void exportKey(APDU apdu) { byte[] apduBuffer = apdu.getBuffer(); - secureChannel.preprocessAPDU(apduBuffer); + short dataLen = secureChannel.preprocessAPDU(apduBuffer); if (!pin.isValidated() || !privateKey.isInitialized()) { ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); @@ -1167,22 +1180,39 @@ public class WalletApplet extends Applet { return; } + byte[] exportPath; + short exportPathOff; + short exportPathLen; + boolean derive = false; + boolean makeCurrent = false; + switch (apduBuffer[ISO7816.OFFSET_P1]) { - case EXPORT_KEY_P1_ANY: - if (!publicOnly) { - ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); - } + case EXPORT_KEY_P1_CURRENT: + exportPath = keyPath; + exportPathOff = (short) 0; + exportPathLen = keyPathLen; break; - case EXPORT_KEY_P1_HIGH: - if (keyPathLen < 4 || ((((byte)(keyPath[(byte)(keyPathLen - 4)] & EXPORT_KEY_HIGH_MASK)) != EXPORT_KEY_HIGH_MASK))){ - ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); - } + case EXPORT_KEY_P1_DERIVE_AND_MAKE_CURRENT: + makeCurrent = true; + case EXPORT_KEY_P1_DERIVE: + derive = true; + exportPath = apduBuffer; + exportPathOff = ISO7816.OFFSET_CDATA; + exportPathLen = dataLen; break; default: ISOException.throwIt(ISO7816.SW_INCORRECT_P1P2); return; } + if (!publicOnly && ((exportPathLen < (short)(((short) EIP_1581_PREFIX.length) + 8)) || (Util.arrayCompare(EIP_1581_PREFIX, (short) 0, exportPath, exportPathOff, (short) EIP_1581_PREFIX.length) != 0))) { + ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); + } + + if (makeCurrent) { + doDerive(apduBuffer, dataLen, true, false); + } + short off = SecureChannel.SC_OUT_OFFSET; apduBuffer[off++] = TLV_KEY_TEMPLATE; diff --git a/src/test/java/im/status/wallet/WalletAppletTest.java b/src/test/java/im/status/wallet/WalletAppletTest.java index 23ce89a..b380b8b 100644 --- a/src/test/java/im/status/wallet/WalletAppletTest.java +++ b/src/test/java/im/status/wallet/WalletAppletTest.java @@ -737,7 +737,7 @@ public class WalletAppletTest { assertEquals(0x9000, response.getSW()); byte[] keyUID = response.getData(); - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_ANY, true); + response = cmdSet.exportCurrentKey(true); assertEquals(0x9000, response.getSW()); byte[] pubKey = response.getData(); @@ -945,13 +945,13 @@ public class WalletAppletTest { new Random().nextBytes(chainCode); // Security condition violation: SecureChannel not open - ResponseAPDU response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); + ResponseAPDU response = cmdSet.exportCurrentKey(true); assertEquals(0x6985, response.getSW()); cmdSet.autoOpenSecureChannel(); // Security condition violation: PIN not verified - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); + response = cmdSet.exportCurrentKey(true); assertEquals(0x6985, response.getSW()); response = cmdSet.verifyPIN("000000"); @@ -960,61 +960,35 @@ public class WalletAppletTest { assertEquals(0x9000, response.getSW()); // Security condition violation: current key is not exportable - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); + response = cmdSet.exportCurrentKey(false); assertEquals(0x6985, response.getSW()); - response = cmdSet.deriveKey(new byte[] { (byte) 0x80, 0x00, 0x00, 0x2c}, WalletApplet.DERIVE_P1_SOURCE_MASTER); + response = cmdSet.deriveKey(new byte[] {(byte) 0x80, 0x00, 0x00, 0x2B, (byte) 0x80, 0x00, 0x00, 0x3C, (byte) 0x80, 0x00, 0x06, 0x2c, (byte) 0x00, 0x00, 0x00, 0x00, (byte) 0x00, 0x00, 0x00, 0x00}, WalletApplet.DERIVE_P1_SOURCE_MASTER); assertEquals(0x9000, response.getSW()); - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); - assertEquals(0x6985, response.getSW()); - response = cmdSet.deriveKey(new byte[] { (byte) 0x80, 0x00, 0x00, 0x3c}, WalletApplet.DERIVE_P1_SOURCE_CURRENT); - assertEquals(0x9000, response.getSW()); - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); - assertEquals(0x6985, response.getSW()); - response = cmdSet.deriveKey(new byte[] { (byte) 0x80, 0x00, 0x00, 0x00}, WalletApplet.DERIVE_P1_SOURCE_CURRENT); - assertEquals(0x9000, response.getSW()); - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); - assertEquals(0x6985, response.getSW()); - response = cmdSet.deriveKey(new byte[] { (byte) 0x00, 0x00, 0x00, 0x00}, WalletApplet.DERIVE_P1_SOURCE_CURRENT); - assertEquals(0x9000, response.getSW()); - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); + response = cmdSet.exportCurrentKey(false); assertEquals(0x6985, response.getSW()); - - // Export current public key (wrong P2) - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_ANY, false); + response = cmdSet.deriveKey(new byte[] {(byte) 0x80, 0x00, 0x00, 0x2B, (byte) 0x80, 0x00, 0x00, 0x3C, (byte) 0x80, 0x00, 0x06, 0x2D, (byte) 0x00, 0x00, 0x00, 0x00}, WalletApplet.DERIVE_P1_SOURCE_MASTER); + assertEquals(0x9000, response.getSW()); + response = cmdSet.exportCurrentKey(false); assertEquals(0x6985, response.getSW()); + // Export current public key - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_ANY, true); + response = cmdSet.exportCurrentKey(true); assertEquals(0x9000, response.getSW()); byte[] keyTemplate = response.getData(); - verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002c, 0x8000003c, 0x80000000, 0x00000000 }, true); - - response = cmdSet.deriveKey(new byte[] {(byte) 0xC0, 0x00, 0x00, 0x01}, WalletApplet.DERIVE_P1_SOURCE_CURRENT); - assertEquals(0x9000, response.getSW()); - - // Wrong P1 - response = cmdSet.exportKey((byte) 3, false); - assertEquals(0x6a86, response.getSW()); + verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002b, 0x8000003c, 0x8000062d, 0x00000000 }, true); // Correct - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); + response = cmdSet.exportKey(new byte[] {(byte) 0x80, 0x00, 0x00, 0x2B, (byte) 0x80, 0x00, 0x00, 0x3C, (byte) 0x80, 0x00, 0x06, 0x2D, (byte) 0x00, 0x00, 0x00, 0x00, (byte) 0x00, 0x00, 0x00, 0x00}, true,false); assertEquals(0x9000, response.getSW()); keyTemplate = response.getData(); - verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002c, 0x8000003c, 0x80000000, 0x00000000, 0xC0000001 }, false); - - // Correct public only - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, true); - assertEquals(0x9000, response.getSW()); - keyTemplate = response.getData(); - verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002c, 0x8000003c, 0x80000000, 0x00000000, 0xC0000001 }, true); + verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002b, 0x8000003c, 0x8000062d, 0x00000000, 0x00000000 }, false); // Reset response = cmdSet.deriveKey(new byte[] {}, WalletApplet.DERIVE_P1_SOURCE_MASTER); assertEquals(0x9000, response.getSW()); - response = cmdSet.exportKey(WalletApplet.EXPORT_KEY_P1_HIGH, false); - assertEquals(0x6985, response.getSW()); } @Test From 852a7d9f482847b07add66495c1d6bd65cbb3155 Mon Sep 17 00:00:00 2001 From: Michele Balistreri Date: Wed, 28 Nov 2018 17:44:29 +0300 Subject: [PATCH 2/7] remove reference to old usage mode --- APPLICATION.MD | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/APPLICATION.MD b/APPLICATION.MD index 17bdbdb..c8c0101 100644 --- a/APPLICATION.MD +++ b/APPLICATION.MD @@ -262,8 +262,7 @@ signing sessions, if any. Unless a DERIVE KEY is sent, a subsequent SIGN command * INS = 0xD1 * P1 = derivation options * P2 = 0x00 -* Data = a sequence of 32-bit integers (most significant byte first). Empty if the master key must be used. On assisted - derivation contains a public key when P2 = 0x02. +* Data = a sequence of 32-bit integers (most significant byte first). Empty if the master key must be used. * Response SW = 0x9000 on success, 0x6A80 if the format is invalid, 0x6984 if one of the components in the path generates an invalid key, 0x6B00 if derivation from parent keys is selected but no valid parent key is cached. * Preconditions: Secure Channel must be opened, user PIN must be verified (if no PIN-less key is defined), an extended From 863ac1d6cf5fe076e27e4b8c3cf211a75a687c1e Mon Sep 17 00:00:00 2001 From: Michele Balistreri Date: Thu, 29 Nov 2018 19:22:14 +0300 Subject: [PATCH 3/7] avoid deriving public when not needed, commit only final result --- src/main/java/im/status/wallet/Crypto.java | 58 +++--- src/main/java/im/status/wallet/SECP256k1.java | 26 ++- .../java/im/status/wallet/WalletApplet.java | 194 +++++++++++------- 3 files changed, 167 insertions(+), 111 deletions(-) diff --git a/src/main/java/im/status/wallet/Crypto.java b/src/main/java/im/status/wallet/Crypto.java index ff15820..405bcee 100644 --- a/src/main/java/im/status/wallet/Crypto.java +++ b/src/main/java/im/status/wallet/Crypto.java @@ -12,8 +12,9 @@ import javacardx.crypto.Cipher; public class Crypto { final static public short AES_BLOCK_SIZE = 16; - final static private short KEY_SECRET_SIZE = 32; - final static private short KEY_DERIVATION_INPUT_SIZE = 37; + final static short KEY_SECRET_SIZE = 32; + final static short KEY_PUB_SIZE = 65; + final static short KEY_DERIVATION_SCRATCH_SIZE = 37; final static private short HMAC_OUT_SIZE = 64; final static private byte[] MAX_S = { (byte) 0x7F, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0x5D, (byte) 0x57, (byte) 0x6E, (byte) 0x73, (byte) 0x57, (byte) 0xA4, (byte) 0x50, (byte) 0x1D, (byte) 0xDF, (byte) 0xE9, (byte) 0x2F, (byte) 0x46, (byte) 0x68, (byte) 0x1B, (byte) 0x20, (byte) 0xA0 }; @@ -22,7 +23,6 @@ public class Crypto { final static private byte HMAC_IPAD = (byte) 0x36; final static private byte HMAC_OPAD = (byte) 0x5c; final static private short HMAC_BLOCK_SIZE = (short) 128; - final static private short HMAC_BLOCK_OFFSET = (short) KEY_DERIVATION_INPUT_SIZE + HMAC_OUT_SIZE; final static private byte[] KEY_BITCOIN_SEED = {'B', 'i', 't', 'c', 'o', 'i', 'n', ' ', 's', 'e', 'e', 'd'}; @@ -38,7 +38,7 @@ public class Crypto { private AESKey tmpAES256; - private byte[] tmp; + private byte[] hmacBlock; Crypto() { random = RandomData.getInstance(RandomData.ALG_SECURE_RANDOM); @@ -49,18 +49,14 @@ public class Crypto { tmpAES256 = (AESKey) KeyBuilder.buildKey(KeyBuilder.TYPE_AES_TRANSIENT_DESELECT, KeyBuilder.LENGTH_AES_256, false); - short blockSize; - try { - blockSize = 0; hmacSHA512 = Signature.getInstance(Signature.ALG_HMAC_SHA_512, false); hmacKey = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC_TRANSIENT_DESELECT, KEY_SECRET_SIZE, false); } catch (CryptoException e) { hmacSHA512 = null; - blockSize = HMAC_BLOCK_SIZE; + hmacBlock = JCSystem.makeTransientByteArray(HMAC_BLOCK_SIZE, JCSystem.CLEAR_ON_RESET); } - tmp = JCSystem.makeTransientByteArray((short) (HMAC_BLOCK_OFFSET + blockSize), JCSystem.CLEAR_ON_RESET); } public short oneShotAES(byte mode, byte[] src, short sOff, short sLen, byte[] dst, short dOff, byte[] key, short keyOff) { @@ -69,6 +65,10 @@ public class Crypto { return aesCbcIso9797m2.doFinal(src, (short) (sOff + AES_BLOCK_SIZE), sLen, dst, dOff); } + boolean bip32IsHardened(byte[] i, short iOff) { + return (i[iOff] & (byte) 0x80) == (byte) 0x80; + } + /** * Derives a private key according to the algorithm defined in BIP32. The BIP32 specifications define some checks * to be performed on the derived keys. In the very unlikely event that these checks fail this key is not considered @@ -76,43 +76,33 @@ public class Crypto { * * @param i the buffer containing the key path element (a 32-bit big endian integer) * @param iOff the offset in the buffer - * @param privateKey the parent private key - * @param publicKey the parent public key - * @param chain the chain code - * @param chainOff the offset in the chain code buffer * @return true if successful, false otherwise */ - boolean bip32CKDPriv(byte[] i, short iOff, ECPrivateKey privateKey, ECPublicKey publicKey, byte[] chain, short chainOff) { - short off = 0; + boolean bip32CKDPriv(byte[] i, short iOff, byte[] scratch, short scratchOff, byte[] data, short dataOff, byte[] output, short outOff) { + short off = scratchOff; - if ((i[iOff] & (byte) 0x80) == (byte) 0x80) { - tmp[off++] = 0; - off += privateKey.getS(tmp, off); + if (bip32IsHardened(i, iOff)) { + scratch[off++] = 0; + off = Util.arrayCopyNonAtomic(data, dataOff, scratch, off, KEY_SECRET_SIZE); } else { - off = (short) (publicKey.getW(tmp, (short) 0) - 1); - tmp[0] = ((tmp[off] & 1) != 0 ? (byte) 0x03 : (byte) 0x02); - off = (short) ((short) (off / 2) + 1); + scratch[off++] = ((data[(short) (dataOff + KEY_SECRET_SIZE + KEY_SECRET_SIZE + KEY_PUB_SIZE - 1)] & 1) != 0 ? (byte) 0x03 : (byte) 0x02); + off = Util.arrayCopyNonAtomic(data, (short) (dataOff + KEY_SECRET_SIZE + KEY_SECRET_SIZE + 1), scratch, off, KEY_SECRET_SIZE); } - off = Util.arrayCopyNonAtomic(i, iOff, tmp, off, (short) 4); + off = Util.arrayCopyNonAtomic(i, iOff, hmacBlock, off, (short) 4); - hmacSHA512(chain, chainOff, KEY_SECRET_SIZE, tmp, (short) 0, off, tmp, off); + hmacSHA512(data, (short)(dataOff + KEY_SECRET_SIZE), KEY_SECRET_SIZE, scratch, scratchOff, off, output, outOff); - if (ucmp256(tmp, off, SECP256k1.SECP256K1_R, (short) 0) >= 0) { + if (ucmp256(output, outOff, SECP256k1.SECP256K1_R, (short) 0) >= 0) { return false; } - privateKey.getS(tmp, (short) 0); + addm256(output, outOff, data, dataOff, SECP256k1.SECP256K1_R, (short) 0, output, outOff); - addm256(tmp, off, tmp, (short) 0, SECP256k1.SECP256K1_R, (short) 0, tmp, off); - - if (isZero256(tmp, off)) { + if (isZero256(output, outOff)) { return false; } - privateKey.setS(tmp, off, (short) KEY_SECRET_SIZE); - Util.arrayCopy(tmp, (short)(off + KEY_SECRET_SIZE), chain, chainOff, (short) KEY_SECRET_SIZE); - return true; } @@ -177,13 +167,13 @@ public class Crypto { hmacSHA512.sign(in, inOff, inLen, out, outOff); } else { for (byte i = 0; i < 2; i++) { - Util.arrayFillNonAtomic(tmp, HMAC_BLOCK_OFFSET, HMAC_BLOCK_SIZE, (i == 0 ? HMAC_IPAD : HMAC_OPAD)); + Util.arrayFillNonAtomic(hmacBlock, (short) 0, HMAC_BLOCK_SIZE, (i == 0 ? HMAC_IPAD : HMAC_OPAD)); for (short j = 0; j < keyLen; j++) { - tmp[(short)(HMAC_BLOCK_OFFSET + j)] ^= key[(short)(keyOff + j)]; + hmacBlock[j] ^= key[(short)(keyOff + j)]; } - sha512.update(tmp, HMAC_BLOCK_OFFSET, HMAC_BLOCK_SIZE); + sha512.update(hmacBlock, (short) 0, HMAC_BLOCK_SIZE); if (i == 0) { sha512.doFinal(in, inOff, inLen, out, outOff); diff --git a/src/main/java/im/status/wallet/SECP256k1.java b/src/main/java/im/status/wallet/SECP256k1.java index 8fe7ffb..ccae43c 100644 --- a/src/main/java/im/status/wallet/SECP256k1.java +++ b/src/main/java/im/status/wallet/SECP256k1.java @@ -3,6 +3,7 @@ package im.status.wallet; import javacard.security.ECKey; import javacard.security.ECPrivateKey; import javacard.security.KeyAgreement; +import javacard.security.KeyBuilder; /** * Utility methods to work with the SECP256k1 curve. This class is not meant to be instantiated, but its init method @@ -47,17 +48,22 @@ public class SECP256k1 { static final byte SECP256K1_K = (byte)0x01; + static final short SECP256K1_KEY_SIZE = 256; + private static final byte ALG_EC_SVDP_DH_PLAIN_XY = 6; // constant from JavaCard 3.0.5 + private KeyAgreement ecPointMultiplier; private Crypto crypto; + private ECPrivateKey tmpECPrivateKey; /** * Allocates objects needed by this class. Must be invoked during the applet installation exactly 1 time. */ SECP256k1(Crypto crypto) { this.crypto = crypto; - ecPointMultiplier = KeyAgreement.getInstance(ALG_EC_SVDP_DH_PLAIN_XY, false); + this.ecPointMultiplier = KeyAgreement.getInstance(ALG_EC_SVDP_DH_PLAIN_XY, false); + this.tmpECPrivateKey = (ECPrivateKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PRIVATE, SECP256K1_KEY_SIZE, false); } /** @@ -87,6 +93,24 @@ public class SECP256k1 { return multiplyPoint(privateKey, SECP256K1_G, (short) 0, (short) SECP256K1_G.length, pubOut, pubOff); } + + /** + * Derives the public key from the given private key and outputs it in the pubOut buffer. This is done by multiplying + * the private key by the G point of the curve. + * + * @param privateKey the private key + * @param pubOut the output buffer for the public key + * @param pubOff the offset in pubOut + * @return the length of the public key + */ + short derivePublicKey(byte[] privateKey, short privOff, byte[] pubOut, short pubOff) { + tmpECPrivateKey.setS(privateKey, privOff, (short)(SECP256K1_KEY_SIZE/8)); + short res = derivePublicKey(tmpECPrivateKey, pubOut, pubOff); + // Unfortunately our current card does not support EC transient keys + tmpECPrivateKey.clearKey(); + return res; + } + /** * Multiplies a scalar in the form of a private key by the given point. Internally uses a special version of EC-DH * supported since JavaCard 3.0.5 which outputs both X and Y in their uncompressed form. diff --git a/src/main/java/im/status/wallet/WalletApplet.java b/src/main/java/im/status/wallet/WalletApplet.java index 2c18853..a81ad5e 100644 --- a/src/main/java/im/status/wallet/WalletApplet.java +++ b/src/main/java/im/status/wallet/WalletApplet.java @@ -34,7 +34,6 @@ public class WalletApplet extends Applet { static final byte PAIRING_MAX_CLIENT_COUNT = 5; static final byte UID_LENGTH = 16; - static final short EC_KEY_SIZE = 256; static final short CHAIN_CODE_SIZE = 32; static final short KEY_UID_LENGTH = 32; static final short BIP39_SEED_SIZE = CHAIN_CODE_SIZE * 2; @@ -100,7 +99,6 @@ public class WalletApplet extends Applet { private ECPublicKey parentPublicKey; private ECPrivateKey parentPrivateKey; private byte[] parentChainCode; - private boolean parentValid; private ECPublicKey publicKey; private ECPrivateKey privateKey; @@ -152,14 +150,14 @@ public class WalletApplet extends Applet { uid = new byte[UID_LENGTH]; crypto.random.generateData(uid, (short) 0, UID_LENGTH); - masterPublic = (ECPublicKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PUBLIC, EC_KEY_SIZE, false); - masterPrivate = (ECPrivateKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PRIVATE, EC_KEY_SIZE, false); + masterPublic = (ECPublicKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PUBLIC, SECP256k1.SECP256K1_KEY_SIZE, false); + masterPrivate = (ECPrivateKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PRIVATE, SECP256k1.SECP256K1_KEY_SIZE, false); - parentPublicKey = (ECPublicKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PUBLIC, EC_KEY_SIZE, false); - parentPrivateKey = (ECPrivateKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PRIVATE, EC_KEY_SIZE, false); + parentPublicKey = (ECPublicKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PUBLIC, SECP256k1.SECP256K1_KEY_SIZE, false); + parentPrivateKey = (ECPrivateKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PRIVATE, SECP256k1.SECP256K1_KEY_SIZE, false); - publicKey = (ECPublicKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PUBLIC, EC_KEY_SIZE, false); - privateKey = (ECPrivateKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PRIVATE, EC_KEY_SIZE, false); + publicKey = (ECPublicKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PUBLIC, SECP256k1.SECP256K1_KEY_SIZE, false); + privateKey = (ECPrivateKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PRIVATE, SECP256k1.SECP256K1_KEY_SIZE, false); masterChainCode = new byte[CHAIN_CODE_SIZE]; parentChainCode = new byte[CHAIN_CODE_SIZE]; @@ -628,9 +626,10 @@ public class WalletApplet extends Applet { * Resets the status of the keys. This method must be called immediately before committing the transaction where key * manipulation has happened to be sure that the state is always consistent. */ - private void resetKeyStatus(boolean toParent) { - parentValid = false; - keyPathLen = toParent ? (short) (keyPathLen - 4) : 0; + private void resetKeyStatus() { + parentPrivateKey.clearKey(); + parentPublicKey.clearKey(); + keyPathLen = 0; } /** @@ -687,7 +686,7 @@ public class WalletApplet extends Applet { ISOException.throwIt(ISO7816.SW_WRONG_DATA); } - resetKeyStatus(false); + resetKeyStatus(); JCSystem.commitTransaction(); } @@ -719,7 +718,7 @@ public class WalletApplet extends Applet { masterPublic.setW(apduBuffer, (short) 0, pubLen); publicKey.setW(apduBuffer, (short) 0, pubLen); - resetKeyStatus(false); + resetKeyStatus(); JCSystem.commitTransaction(); } @@ -754,96 +753,140 @@ public class WalletApplet extends Applet { ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); } - boolean isReset = apduBuffer[ISO7816.OFFSET_P1] == DERIVE_P1_SOURCE_MASTER; - boolean fromParent = apduBuffer[ISO7816.OFFSET_P1] == DERIVE_P1_SOURCE_PARENT; - - doDerive(apduBuffer, len, isReset, fromParent); + doDerive(apduBuffer, len, apduBuffer[ISO7816.OFFSET_P1], true); } /** * Internal derivation function, called by DERIVE KEY and EXPORT KEY - * @param apduBuffer the APDU buffer + * @param apduBuffer the APDU buffer * @param len the APDU len - * @param isReset start deriving from master - * @param fromParent start deriving from parent + * @param source derivation source + * @param makeCurrent whether the results should be saved or not */ - private void doDerive(byte[] apduBuffer, short len, boolean isReset, boolean fromParent) { + private void doDerive(byte[] apduBuffer, short len, byte source, boolean makeCurrent) { if (!isExtended) { ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); } - if (fromParent && !parentValid) { - ISOException.throwIt(ISO7816.SW_WRONG_P1P2); + short newPathLen; + short pathLenOff; + ECPublicKey sourcePub; + ECPrivateKey sourcePriv; + byte[] sourceChain; + + switch (source) { + case DERIVE_P1_SOURCE_MASTER: + if (len == 0) { + resetToMaster(apduBuffer); + return; + } + + newPathLen = len; + sourcePriv = masterPrivate; + sourcePub = masterPublic; + sourceChain = masterChainCode; + pathLenOff = 0; + break; + case DERIVE_P1_SOURCE_PARENT: + if (!parentPublicKey.isInitialized()) { + ISOException.throwIt(ISO7816.SW_WRONG_P1P2); + } + + if (len == 0) { + ISOException.throwIt(ISO7816.SW_WRONG_DATA); + } + + newPathLen = (short) (keyPathLen + len - 4); + sourcePriv = parentPrivateKey; + sourcePub = parentPublicKey; + sourceChain = parentChainCode; + pathLenOff = (short) (keyPathLen - 4); + break; + case DERIVE_P1_SOURCE_CURRENT: + if (len == 0) { + ISOException.throwIt(ISO7816.SW_WRONG_DATA); + } + + newPathLen = (short) (keyPathLen + len); + sourcePriv = privateKey; + sourcePub = publicKey; + sourceChain = chainCode; + pathLenOff = keyPathLen; + break; + default: + ISOException.throwIt(ISO7816.SW_INCORRECT_P1P2); + return; } - if (((short) (len % 4) != 0) || ((short)(len + (isReset ? 0 : keyPathLen)) > keyPath.length)) { + if (((short) (len % 4) != 0) || (newPathLen > keyPath.length)) { ISOException.throwIt(ISO7816.SW_WRONG_DATA); } - short chainEnd = (short) (ISO7816.OFFSET_CDATA + len); + short scratchOff = (short) (ISO7816.OFFSET_CDATA + len); + short dataOff = (short) (scratchOff + Crypto.KEY_DERIVATION_SCRATCH_SIZE); - if (isReset || fromParent) { - resetKeys(fromParent, apduBuffer, chainEnd); + short pubKeyOff = (short) (dataOff + sourcePriv.getS(apduBuffer, dataOff)); + pubKeyOff = Util.arrayCopyNonAtomic(sourceChain, (short)0, apduBuffer, pubKeyOff, CHAIN_CODE_SIZE); + short outputOff = (short) (pubKeyOff + Crypto.KEY_PUB_SIZE); + + if (!crypto.bip32IsHardened(apduBuffer, ISO7816.OFFSET_CDATA)) { + sourcePub.getW(apduBuffer, pubKeyOff); + } else { + apduBuffer[pubKeyOff] = 0; } - for (short i = ISO7816.OFFSET_CDATA; i < chainEnd; i += 4) { - JCSystem.beginTransaction(); + for (short i = ISO7816.OFFSET_CDATA; i < scratchOff; i += 4) { + Util.arrayCopyNonAtomic(apduBuffer, outputOff, apduBuffer, dataOff, (short) (Crypto.KEY_SECRET_SIZE + CHAIN_CODE_SIZE)); - copyKeys(privateKey, publicKey, chainCode, parentPrivateKey, parentPublicKey, parentChainCode, apduBuffer, chainEnd); - - if (!crypto.bip32CKDPriv(apduBuffer, i, privateKey, publicKey, chainCode, (short) 0)) { - ISOException.throwIt(ISO7816.SW_DATA_INVALID); + if ((i > ISO7816.OFFSET_CDATA) && !crypto.bip32IsHardened(apduBuffer, i)) { + secp256k1.derivePublicKey(apduBuffer, dataOff, apduBuffer, pubKeyOff); + } else { + apduBuffer[pubKeyOff] = 0; } - Util.arrayCopy(apduBuffer, i, keyPath, keyPathLen, (short) 4); + if (!crypto.bip32CKDPriv(apduBuffer, i, apduBuffer, scratchOff, apduBuffer, dataOff, apduBuffer, outputOff)) { + ISOException.throwIt(ISO7816.SW_DATA_INVALID); + } + } - short pubLen = secp256k1.derivePublicKey(privateKey, apduBuffer, chainEnd); - publicKey.setW(apduBuffer, chainEnd, pubLen); - keyPathLen += 4; - parentValid = true; + if (makeCurrent) { + JCSystem.beginTransaction(); + parentPrivateKey.setS(apduBuffer, dataOff, Crypto.KEY_SECRET_SIZE); + Util.arrayCopy(apduBuffer, (short)(dataOff + Crypto.KEY_SECRET_SIZE), parentChainCode, (short) 0, CHAIN_CODE_SIZE); + + if (apduBuffer[pubKeyOff] == 0x04) { + parentPublicKey.setW(apduBuffer, pubKeyOff, Crypto.KEY_PUB_SIZE); + } else { + secp256k1.derivePublicKey(parentPrivateKey, apduBuffer, scratchOff); + parentPublicKey.setW(apduBuffer, scratchOff, Crypto.KEY_PUB_SIZE); + } + + parentPublicKey.setW(apduBuffer, pubKeyOff, Crypto.KEY_PUB_SIZE); + + privateKey.setS(apduBuffer, outputOff, Crypto.KEY_SECRET_SIZE); + Util.arrayCopy(apduBuffer, (short)(outputOff + Crypto.KEY_SECRET_SIZE), chainCode, (short) 0, CHAIN_CODE_SIZE); + secp256k1.derivePublicKey(privateKey, apduBuffer, scratchOff); + publicKey.setW(apduBuffer, scratchOff, Crypto.KEY_PUB_SIZE); + + Util.arrayCopy(apduBuffer, ISO7816.OFFSET_CDATA, keyPath, pathLenOff, len); + keyPathLen = newPathLen; JCSystem.commitTransaction(); } } /** - * Resets the current key and key path to the parent or master key. A transaction is used to make sure this all - * happens at once. This method is called internally by the deriveKey method. + * Resets to master key * - * @param toParent resets to the parent key - * @param buffer a buffer which can be overwritten (currently the APDU buffer) - * @param offset the offset at which the buffer is free + * @param apduBuffer the APDU buffer */ - private void resetKeys(boolean toParent, byte[] buffer, short offset) { - ECPrivateKey srcPrivKey = toParent ? parentPrivateKey : masterPrivate; - ECPublicKey srcPubKey = toParent ? parentPublicKey : masterPublic; - byte[] srcChainCode = toParent ? parentChainCode : masterChainCode; - - JCSystem.beginTransaction(); - copyKeys(srcPrivKey, srcPubKey, srcChainCode, privateKey, publicKey, chainCode, buffer, offset); - resetKeyStatus(toParent); - JCSystem.commitTransaction(); - } - - /** - * Copys a key set to another one. Requires a transient buffer which can be overwritten. - * - * @param srcPrivate source private key - * @param srcPublic source public key - * @param srcChain source chain code - * @param dstPrivate destination private key - * @param dstPublic destination public key - * @param dstChain destination chain code - * @param buffer tmp buffer - * @param offset tmp buffer offset - */ - private void copyKeys(ECPrivateKey srcPrivate, ECPublicKey srcPublic, byte[] srcChain, ECPrivateKey dstPrivate, ECPublicKey dstPublic, byte[] dstChain, byte[] buffer, short offset) { - short pubOff = (short) (offset + srcPrivate.getS(buffer, offset)); - short pubLen = srcPublic.getW(buffer, pubOff); - - Util.arrayCopy(srcChain, (short) 0, dstChain, (short) 0, CHAIN_CODE_SIZE); - dstPrivate.setS(buffer, offset, CHAIN_CODE_SIZE); - dstPublic.setW(buffer, pubOff, pubLen); + private void resetToMaster(byte[] apduBuffer) { + resetKeyStatus(); + masterPrivate.getS(apduBuffer, ISO7816.OFFSET_CDATA); + privateKey.setS(apduBuffer, ISO7816.OFFSET_CDATA, Crypto.KEY_SECRET_SIZE); + masterPublic.getW(apduBuffer, ISO7816.OFFSET_CDATA); + publicKey.setW(apduBuffer, ISO7816.OFFSET_CDATA, Crypto.KEY_PUB_SIZE); + Util.arrayCopyNonAtomic(masterChainCode, (short) 0, chainCode, (short) 0, CHAIN_CODE_SIZE); } /** @@ -941,7 +984,6 @@ public class WalletApplet extends Applet { keyPathLen = 0; pinlessPathLen = 0; - parentValid = false; isExtended = false; privateKey.clearKey(); publicKey.clearKey(); @@ -1209,8 +1251,8 @@ public class WalletApplet extends Applet { ISOException.throwIt(ISO7816.SW_CONDITIONS_NOT_SATISFIED); } - if (makeCurrent) { - doDerive(apduBuffer, dataLen, true, false); + if (derive) { + doDerive(apduBuffer, dataLen, DERIVE_P1_SOURCE_MASTER, makeCurrent); } short off = SecureChannel.SC_OUT_OFFSET; From b7f9df63835c9758c40ccb63abb634bca58fd960 Mon Sep 17 00:00:00 2001 From: Michele Balistreri Date: Fri, 30 Nov 2018 09:44:39 +0300 Subject: [PATCH 4/7] fix some errors --- src/main/java/im/status/wallet/Crypto.java | 4 +-- .../java/im/status/wallet/WalletApplet.java | 27 ++++++++++--------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/main/java/im/status/wallet/Crypto.java b/src/main/java/im/status/wallet/Crypto.java index 405bcee..e36d113 100644 --- a/src/main/java/im/status/wallet/Crypto.java +++ b/src/main/java/im/status/wallet/Crypto.java @@ -89,9 +89,9 @@ public class Crypto { off = Util.arrayCopyNonAtomic(data, (short) (dataOff + KEY_SECRET_SIZE + KEY_SECRET_SIZE + 1), scratch, off, KEY_SECRET_SIZE); } - off = Util.arrayCopyNonAtomic(i, iOff, hmacBlock, off, (short) 4); + off = Util.arrayCopyNonAtomic(i, iOff, scratch, off, (short) 4); - hmacSHA512(data, (short)(dataOff + KEY_SECRET_SIZE), KEY_SECRET_SIZE, scratch, scratchOff, off, output, outOff); + hmacSHA512(data, (short)(dataOff + KEY_SECRET_SIZE), KEY_SECRET_SIZE, scratch, scratchOff, (short)(off - scratchOff), output, outOff); if (ucmp256(output, outOff, SECP256k1.SECP256K1_R, (short) 0) >= 0) { return false; diff --git a/src/main/java/im/status/wallet/WalletApplet.java b/src/main/java/im/status/wallet/WalletApplet.java index a81ad5e..56a27fd 100644 --- a/src/main/java/im/status/wallet/WalletApplet.java +++ b/src/main/java/im/status/wallet/WalletApplet.java @@ -120,6 +120,8 @@ public class WalletApplet extends Applet { private byte[] duplicationEncKey; private short expectedEntropy; + private byte[] derivationOutput; + /** * Invoked during applet installation. Creates an instance of this class. The installation parameters are passed in * the given buffer. @@ -175,6 +177,8 @@ public class WalletApplet extends Applet { duplicationEncKey = new byte[(short)(KeyBuilder.LENGTH_AES_256/8)]; expectedEntropy = -1; + derivationOutput = JCSystem.makeTransientByteArray((short) (Crypto.KEY_SECRET_SIZE + CHAIN_CODE_SIZE), JCSystem.CLEAR_ON_RESET); + register(bArray, (short) (bOffset + 1), bArray[bOffset]); } @@ -826,8 +830,7 @@ public class WalletApplet extends Applet { short dataOff = (short) (scratchOff + Crypto.KEY_DERIVATION_SCRATCH_SIZE); short pubKeyOff = (short) (dataOff + sourcePriv.getS(apduBuffer, dataOff)); - pubKeyOff = Util.arrayCopyNonAtomic(sourceChain, (short)0, apduBuffer, pubKeyOff, CHAIN_CODE_SIZE); - short outputOff = (short) (pubKeyOff + Crypto.KEY_PUB_SIZE); + pubKeyOff = Util.arrayCopyNonAtomic(sourceChain, (short) 0, apduBuffer, pubKeyOff, CHAIN_CODE_SIZE); if (!crypto.bip32IsHardened(apduBuffer, ISO7816.OFFSET_CDATA)) { sourcePub.getW(apduBuffer, pubKeyOff); @@ -836,15 +839,17 @@ public class WalletApplet extends Applet { } for (short i = ISO7816.OFFSET_CDATA; i < scratchOff; i += 4) { - Util.arrayCopyNonAtomic(apduBuffer, outputOff, apduBuffer, dataOff, (short) (Crypto.KEY_SECRET_SIZE + CHAIN_CODE_SIZE)); + if (i > ISO7816.OFFSET_CDATA) { + Util.arrayCopyNonAtomic(derivationOutput, (short) 0, apduBuffer, dataOff, (short) (Crypto.KEY_SECRET_SIZE + CHAIN_CODE_SIZE)); - if ((i > ISO7816.OFFSET_CDATA) && !crypto.bip32IsHardened(apduBuffer, i)) { - secp256k1.derivePublicKey(apduBuffer, dataOff, apduBuffer, pubKeyOff); - } else { - apduBuffer[pubKeyOff] = 0; + if (!crypto.bip32IsHardened(apduBuffer, i)) { + secp256k1.derivePublicKey(apduBuffer, dataOff, apduBuffer, pubKeyOff); + } else { + apduBuffer[pubKeyOff] = 0; + } } - if (!crypto.bip32CKDPriv(apduBuffer, i, apduBuffer, scratchOff, apduBuffer, dataOff, apduBuffer, outputOff)) { + if (!crypto.bip32CKDPriv(apduBuffer, i, apduBuffer, scratchOff, apduBuffer, dataOff, derivationOutput, (short) 0)) { ISOException.throwIt(ISO7816.SW_DATA_INVALID); } } @@ -862,10 +867,8 @@ public class WalletApplet extends Applet { parentPublicKey.setW(apduBuffer, scratchOff, Crypto.KEY_PUB_SIZE); } - parentPublicKey.setW(apduBuffer, pubKeyOff, Crypto.KEY_PUB_SIZE); - - privateKey.setS(apduBuffer, outputOff, Crypto.KEY_SECRET_SIZE); - Util.arrayCopy(apduBuffer, (short)(outputOff + Crypto.KEY_SECRET_SIZE), chainCode, (short) 0, CHAIN_CODE_SIZE); + privateKey.setS(derivationOutput, (short) 0, Crypto.KEY_SECRET_SIZE); + Util.arrayCopy(derivationOutput, Crypto.KEY_SECRET_SIZE, chainCode, (short) 0, CHAIN_CODE_SIZE); secp256k1.derivePublicKey(privateKey, apduBuffer, scratchOff); publicKey.setW(apduBuffer, scratchOff, Crypto.KEY_PUB_SIZE); From a0c15da432b468a4ad7d5d5ee8ccf6266787142d Mon Sep 17 00:00:00 2001 From: Michele Balistreri Date: Fri, 30 Nov 2018 09:55:49 +0300 Subject: [PATCH 5/7] add better exception handling --- .../java/im/status/wallet/WalletApplet.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/main/java/im/status/wallet/WalletApplet.java b/src/main/java/im/status/wallet/WalletApplet.java index 56a27fd..a77fb21 100644 --- a/src/main/java/im/status/wallet/WalletApplet.java +++ b/src/main/java/im/status/wallet/WalletApplet.java @@ -266,11 +266,11 @@ public class WalletApplet extends Applet { break; } } catch(ISOException sw) { - if (shouldRespond(apdu) && (sw.getReason() != ISO7816.SW_SECURITY_STATUS_NOT_SATISFIED)) { - secureChannel.respond(apdu, (short) 0, sw.getReason()); - } else { - throw sw; - } + handleException(apdu, sw.getReason()); + } catch (CryptoException ce) { + handleException(apdu, (short)(ISO7816.SW_UNKNOWN | ce.getReason())); + } catch (Exception e) { + handleException(apdu, ISO7816.SW_UNKNOWN); } if (shouldRespond(apdu)) { @@ -278,6 +278,14 @@ public class WalletApplet extends Applet { } } + private void handleException(APDU apdu, short sw) { + if (shouldRespond(apdu) && (sw != ISO7816.SW_SECURITY_STATUS_NOT_SATISFIED)) { + secureChannel.respond(apdu, (short) 0, sw); + } else { + ISOException.throwIt(sw); + } + } + /** * Processes the init command, this is invoked only if the applet has not yet been personalized with secrets. * From cd26da75d30cbf7897d7c9e224df257126647e88 Mon Sep 17 00:00:00 2001 From: Michele Balistreri Date: Fri, 30 Nov 2018 10:01:29 +0300 Subject: [PATCH 6/7] fix usage of uinitialized keys --- src/main/java/im/status/wallet/SECP256k1.java | 6 ++---- src/main/java/im/status/wallet/WalletApplet.java | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/main/java/im/status/wallet/SECP256k1.java b/src/main/java/im/status/wallet/SECP256k1.java index ccae43c..d6dbb1e 100644 --- a/src/main/java/im/status/wallet/SECP256k1.java +++ b/src/main/java/im/status/wallet/SECP256k1.java @@ -64,6 +64,7 @@ public class SECP256k1 { this.crypto = crypto; this.ecPointMultiplier = KeyAgreement.getInstance(ALG_EC_SVDP_DH_PLAIN_XY, false); this.tmpECPrivateKey = (ECPrivateKey) KeyBuilder.buildKey(KeyBuilder.TYPE_EC_FP_PRIVATE, SECP256K1_KEY_SIZE, false); + setCurveParameters(tmpECPrivateKey); } /** @@ -105,10 +106,7 @@ public class SECP256k1 { */ short derivePublicKey(byte[] privateKey, short privOff, byte[] pubOut, short pubOff) { tmpECPrivateKey.setS(privateKey, privOff, (short)(SECP256K1_KEY_SIZE/8)); - short res = derivePublicKey(tmpECPrivateKey, pubOut, pubOff); - // Unfortunately our current card does not support EC transient keys - tmpECPrivateKey.clearKey(); - return res; + return derivePublicKey(tmpECPrivateKey, pubOut, pubOff); } /** diff --git a/src/main/java/im/status/wallet/WalletApplet.java b/src/main/java/im/status/wallet/WalletApplet.java index a77fb21..6bff517 100644 --- a/src/main/java/im/status/wallet/WalletApplet.java +++ b/src/main/java/im/status/wallet/WalletApplet.java @@ -640,7 +640,7 @@ public class WalletApplet extends Applet { */ private void resetKeyStatus() { parentPrivateKey.clearKey(); - parentPublicKey.clearKey(); + secp256k1.setCurveParameters(parentPrivateKey); keyPathLen = 0; } @@ -800,7 +800,7 @@ public class WalletApplet extends Applet { pathLenOff = 0; break; case DERIVE_P1_SOURCE_PARENT: - if (!parentPublicKey.isInitialized()) { + if (!parentPrivateKey.isInitialized()) { ISOException.throwIt(ISO7816.SW_WRONG_P1P2); } From a7da5983c2c0f64c757bce4d0a222ec03d39ed43 Mon Sep 17 00:00:00 2001 From: Michele Balistreri Date: Fri, 30 Nov 2018 10:33:12 +0300 Subject: [PATCH 7/7] fix EXPORT KEY without make current --- APPLICATION.MD | 7 +++- .../java/im/status/wallet/WalletApplet.java | 30 ++++++++++--- .../im/status/wallet/WalletAppletTest.java | 42 ++++++++++++++----- 3 files changed, 60 insertions(+), 19 deletions(-) diff --git a/APPLICATION.MD b/APPLICATION.MD index c8c0101..3026235 100644 --- a/APPLICATION.MD +++ b/APPLICATION.MD @@ -430,7 +430,7 @@ P2: Response Data format: - Tag 0xA1 = keypair template - - Tag 0x80 = ECC public key component + - Tag 0x80 = ECC public key component (could be omitted) - Tag 0x81 = ECC private key component (if P2=0x00) This command exports the requested public and private key. The public key can be always exported (P2=0x01), but the @@ -439,4 +439,7 @@ private key (P2=0x00) can be exported if and only if the requested key path is i The P1 parameter indicates how to the derive the desired key. P1 = 0x00 indicates that the current key must be exported, and no derivation will be performed. P1 = 0x01 derives the path given in the data field without changing the current -path of the card. P1 = 0x02 derives the path but also changes the current path of the card. \ No newline at end of file +path of the card. P1 = 0x02 derives the path but also changes the current path of the card. + +If the private key is being exported, the card could omit exporting the public key for performance reason. The public +key can then be calculate off-card if needed. \ No newline at end of file diff --git a/src/main/java/im/status/wallet/WalletApplet.java b/src/main/java/im/status/wallet/WalletApplet.java index 6bff517..b755805 100644 --- a/src/main/java/im/status/wallet/WalletApplet.java +++ b/src/main/java/im/status/wallet/WalletApplet.java @@ -1270,16 +1270,34 @@ public class WalletApplet extends Applet { apduBuffer[off++] = TLV_KEY_TEMPLATE; off++; - apduBuffer[off++] = TLV_PUB_KEY; - off++; - short len = publicKey.getW(apduBuffer, off); - apduBuffer[(short)(off - 1)] = (byte) len; - off += len; + + short len; + + if (!derive || makeCurrent) { + apduBuffer[off++] = TLV_PUB_KEY; + off++; + len = publicKey.getW(apduBuffer, off); + apduBuffer[(short) (off - 1)] = (byte) len; + off += len; + } else if (publicOnly) { + apduBuffer[off++] = TLV_PUB_KEY; + off++; + len = secp256k1.derivePublicKey(derivationOutput, (short) 0, apduBuffer, off); + apduBuffer[(short) (off - 1)] = (byte) len; + off += len; + } if (!publicOnly) { apduBuffer[off++] = TLV_PRIV_KEY; off++; - len = privateKey.getS(apduBuffer, off); + + if (!derive || makeCurrent) { + len = privateKey.getS(apduBuffer, off); + } else { + Util.arrayCopyNonAtomic(derivationOutput, (short) 0, apduBuffer, off, Crypto.KEY_SECRET_SIZE); + len = Crypto.KEY_SECRET_SIZE; + } + apduBuffer[(short) (off - 1)] = (byte) len; off += len; } diff --git a/src/test/java/im/status/wallet/WalletAppletTest.java b/src/test/java/im/status/wallet/WalletAppletTest.java index b380b8b..53f2e72 100644 --- a/src/test/java/im/status/wallet/WalletAppletTest.java +++ b/src/test/java/im/status/wallet/WalletAppletTest.java @@ -978,13 +978,28 @@ public class WalletAppletTest { response = cmdSet.exportCurrentKey(true); assertEquals(0x9000, response.getSW()); byte[] keyTemplate = response.getData(); - verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002b, 0x8000003c, 0x8000062d, 0x00000000 }, true); + verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002b, 0x8000003c, 0x8000062d, 0x00000000 }, true, false); - // Correct + // Derive & Make current response = cmdSet.exportKey(new byte[] {(byte) 0x80, 0x00, 0x00, 0x2B, (byte) 0x80, 0x00, 0x00, 0x3C, (byte) 0x80, 0x00, 0x06, 0x2D, (byte) 0x00, 0x00, 0x00, 0x00, (byte) 0x00, 0x00, 0x00, 0x00}, true,false); assertEquals(0x9000, response.getSW()); keyTemplate = response.getData(); - verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002b, 0x8000003c, 0x8000062d, 0x00000000, 0x00000000 }, false); + verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002b, 0x8000003c, 0x8000062d, 0x00000000, 0x00000000 }, false, false); + + // Derive without making current + response = cmdSet.exportKey(new byte[] {(byte) 0x80, 0x00, 0x00, 0x2B, (byte) 0x80, 0x00, 0x00, 0x3C, (byte) 0x80, 0x00, 0x06, 0x2D, (byte) 0x00, 0x00, 0x00, 0x00, (byte) 0x00, 0x00, 0x00, 0x01}, false,false); + assertEquals(0x9000, response.getSW()); + keyTemplate = response.getData(); + verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002b, 0x8000003c, 0x8000062d, 0x00000000, 0x00000001 }, false, true); + response = cmdSet.getStatus(WalletApplet.GET_STATUS_P1_KEY_PATH); + assertEquals(0x9000, response.getSW()); + assertArrayEquals(new byte[] {(byte) 0x80, 0x00, 0x00, 0x2B, (byte) 0x80, 0x00, 0x00, 0x3C, (byte) 0x80, 0x00, 0x06, 0x2D, (byte) 0x00, 0x00, 0x00, 0x00, (byte) 0x00, 0x00, 0x00, 0x00}, response.getData()); + + // Export current + response = cmdSet.exportCurrentKey(false); + assertEquals(0x9000, response.getSW()); + keyTemplate = response.getData(); + verifyExportedKey(keyTemplate, keyPair, chainCode, new int[] { 0x8000002b, 0x8000003c, 0x8000062d, 0x00000000, 0x00000000 }, false, false); // Reset response = cmdSet.deriveKey(new byte[] {}, WalletApplet.DERIVE_P1_SOURCE_MASTER); @@ -1289,19 +1304,24 @@ public class WalletAppletTest { } } - private void verifyExportedKey(byte[] keyTemplate, KeyPair keyPair, byte[] chainCode, int[] path, boolean publicOnly) { + private void verifyExportedKey(byte[] keyTemplate, KeyPair keyPair, byte[] chainCode, int[] path, boolean publicOnly, boolean noPubKey) { ECKey key = deriveKey(keyPair, chainCode, path).decompress(); assertEquals(WalletApplet.TLV_KEY_TEMPLATE, keyTemplate[0]); - assertEquals(WalletApplet.TLV_PUB_KEY, keyTemplate[2]); - byte[] pubKey = Arrays.copyOfRange(keyTemplate, 4, 4 + keyTemplate[3]); - assertArrayEquals(key.getPubKey(), pubKey); + int pubKeyLen = 0; + + if (!noPubKey) { + assertEquals(WalletApplet.TLV_PUB_KEY, keyTemplate[2]); + byte[] pubKey = Arrays.copyOfRange(keyTemplate, 4, 4 + keyTemplate[3]); + assertArrayEquals(key.getPubKey(), pubKey); + pubKeyLen = 2 + pubKey.length; + } if (publicOnly) { - assertEquals(pubKey.length + 2, keyTemplate[1]); - assertEquals(pubKey.length + 4, keyTemplate.length); + assertEquals(pubKeyLen, keyTemplate[1]); + assertEquals(pubKeyLen + 2, keyTemplate.length); } else { - assertEquals(WalletApplet.TLV_PRIV_KEY, keyTemplate[4 + pubKey.length]); - byte[] privateKey = Arrays.copyOfRange(keyTemplate, 6 + pubKey.length, 6 + pubKey.length + keyTemplate[5 + pubKey.length]); + assertEquals(WalletApplet.TLV_PRIV_KEY, keyTemplate[2 + pubKeyLen]); + byte[] privateKey = Arrays.copyOfRange(keyTemplate, 4 + pubKeyLen, 4 + pubKeyLen + keyTemplate[3 + pubKeyLen]); byte[] tPrivKey = key.getPrivKey().toByteArray();