From 3cf8b8aa5b72ed039590727712e2d1ce06ce9f72 Mon Sep 17 00:00:00 2001 From: Ryad Benadjila Date: Wed, 4 Sep 2019 23:01:39 +0200 Subject: [PATCH] [bugfixes] - Bugfix in secure channel APDU encryption (missing offset) - Bugfix in AES CBC mode where doFinal must be calle in place of Update because of potential buffering yielding in bad result + check in AES when inputbuffer == outputffer and overlapping buffers (see Javacard API) - Use non overlapping buffer when encrypting data in get_key [enhancements] - Better handling of native versus non-native HMAC + explicit warnings to the end user - Remove transaction when constructing persistent objects since some Javacards do not support this well - Remove DEBUG mode in the WooKey class since it is not necessary in production --- src/wookey/auth/WooKeyAuth.java | 17 ++++-- src/wookey/common/Aes.java | 14 ++++- src/wookey/common/Hmac.java | 83 +++++++++++++++++++--------- src/wookey/common/SecureChannel.java | 12 +--- src/wookey/common/WooKey.java | 5 +- src/wookey/dfu/WooKeyDFU.java | 13 ++++- src/wookey/sig/WooKeySIG.java | 13 ++++- 7 files changed, 109 insertions(+), 48 deletions(-) diff --git a/src/wookey/auth/WooKeyAuth.java b/src/wookey/auth/WooKeyAuth.java index 997b778..fde7a94 100644 --- a/src/wookey/auth/WooKeyAuth.java +++ b/src/wookey/auth/WooKeyAuth.java @@ -14,6 +14,9 @@ public class WooKeyAuth extends Applet implements ExtendedLength /* Instructions specific to the AUTH applet */ public static final byte TOKEN_INS_GET_KEY = (byte) 0x10; + /* Variable handling initialization */ + private boolean init_done = false; + public static void install(byte[] bArray, short bOffset, byte bLength) { @@ -59,9 +62,9 @@ private void get_key(APDU apdu, byte ins){ /* Also send SHA-256(ESSIV master key) */ md.reset(); md.doFinal(Keys.MasterSecretKey, (short) 0, (short) 32, W.data, (short) 32); - W.schannel.pin_encrypt_sensitive_data(W.data, W.data, (short) 0, (short) 0, (short) 64); + W.schannel.pin_encrypt_sensitive_data(W.data, W.data, (short) 0, (short) 64, (short) 64); /* Now send the encrypted APDU */ - W.schannel.send_encrypted_apdu(apdu, W.data, (short) 0, (short) 64, (byte) 0x90, (byte) 0x00); + W.schannel.send_encrypted_apdu(apdu, W.data, (short) 64, (short) 64, (byte) 0x90, (byte) 0x00); return; } } @@ -81,13 +84,19 @@ public void process(APDU apdu) return; } - if(W == null){ + if((W == null) || (init_done == false)){ + init_done = false; W = new WooKey(Keys.UserPin, Keys.PetPin, Keys.OurPrivKeyBuf, Keys.OurPubKeyBuf, Keys.WooKeyPubKeyBuf, Keys.LibECCparams, Keys.PetName, Keys.PetNameLength, Keys.max_pin_tries, Keys.max_secure_channel_tries); + init_done = true; } - if(buffer[ISO7816.OFFSET_CLA] != (byte)0x00){ + if(init_done == false){ ISOException.throwIt((short) 0x6660); } + + if(buffer[ISO7816.OFFSET_CLA] != (byte)0x00){ + ISOException.throwIt((short) 0x6661); + } /* Begin to handle the common APDUs */ if(W.common_apdu_process(apdu) == true){ diff --git a/src/wookey/common/Aes.java b/src/wookey/common/Aes.java index 6bf0628..5dbddbd 100644 --- a/src/wookey/common/Aes.java +++ b/src/wookey/common/Aes.java @@ -217,6 +217,13 @@ private void increment_iv(){ } public short aes(byte[] input, short inputoffset, short inputlen, byte[] output, short outputoffset){ + /* If input and output are the same and size is block aligned (which is always the case here since we do not handle padding), + * they should not overlap ... See the Javacard API documentation for Cipher.update */ + if(input == output){ + if((inputoffset < outputoffset) && (outputoffset < (short)(inputoffset + inputlen))){ + CryptoException.throwIt(CryptoException.ILLEGAL_USE); + } + } try{ if(cipherAES == null){ CryptoException.throwIt(CryptoException.UNINITIALIZED_KEY); @@ -231,8 +238,11 @@ public short aes(byte[] input, short inputoffset, short inputlen, byte[] output, if(inputlen % AES_BLOCK_SIZE != 0){ CryptoException.throwIt(CryptoException.ILLEGAL_VALUE); } - /* Note: we use the update method for CBC to keep this feature at the lower level */ - return cipherAES.update(input, inputoffset, inputlen, output, outputoffset); + /* !!NOTE: we would want to use the update method for CBC to keep the CBC context at the lower level, + * however the Cipher.update method is buffered and doFinal must be invoked otherwise bad results + * can be still buffered ... (see the Javacard API documentation) + */ + return cipherAES.doFinal(input, inputoffset, inputlen, output, outputoffset); case CTR: if(USE_AES_CTR_ALT_IMPLEMENTATION == false){ /* NOTE: this seems to be sub-optimal way of performing AES-CTR for big data chunks, since diff --git a/src/wookey/common/Hmac.java b/src/wookey/common/Hmac.java index f4233e9..dc98fdd 100644 --- a/src/wookey/common/Hmac.java +++ b/src/wookey/common/Hmac.java @@ -7,8 +7,14 @@ * The software implementation optionally uses masking to try limiting some leaks. */ public class Hmac { + /* !!WARNING: using native HMAC can provoke unexpected errors on + * cards that do not retur an error when getting an instance, but + * raise an exception when using the instance ... Turn to 'false' + * to fall back to software HMAC if this happens. + */ + private static final boolean TRY_USE_NATIVE_HMAC = true; /* If we have native HMAC support, use it */ - private boolean use_native_hmac = true; + private boolean use_native_hmac = false; Signature hmac_instance = null; HMACKey hmac_key = null; /* The message digest instances */ @@ -21,6 +27,11 @@ public class Hmac { private byte[] dgst_i; private byte digest = 0; /* For random masking */ + /* !!WARNING: the ipad/opad masking can be hard on the eeprom initialization, and some + * Javacards won't be compatible with this. Turn to false if you have an error at + * first applet selection when eeprom is initialized by the applet ... + * (masking has been tested to be working on NXP J3D081 smart cards) + */ private static final boolean USE_HMAC_MASKING = true; private RandomData random = null; private byte[] orig_ipad_masks = null; @@ -29,33 +40,53 @@ public class Hmac { private byte[] opad_masks = null; protected Hmac(byte digest_type){ - /* Check native HMAC support on the card - * NOTE: HMAC implementation is unfortunately not present on the tested cards ... - */ - use_native_hmac = true; - try { - switch(digest_type){ - case MessageDigest.ALG_SHA_224: - CryptoException.throwIt(CryptoException.NO_SUCH_ALGORITHM); - break; - case MessageDigest.ALG_SHA_256: - hmac_instance = Signature.getInstance(Signature.ALG_HMAC_SHA_256, false); - hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC, KeyBuilder.LENGTH_HMAC_SHA_256_BLOCK_64, false); - break; - case MessageDigest.ALG_SHA_384: - hmac_instance = Signature.getInstance(Signature.ALG_HMAC_SHA_384, false); - hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC, KeyBuilder.LENGTH_HMAC_SHA_384_BLOCK_128, false); - break; - case MessageDigest.ALG_SHA_512: - hmac_instance = Signature.getInstance(Signature.ALG_HMAC_SHA_512, false); - hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC, KeyBuilder.LENGTH_HMAC_SHA_512_BLOCK_128, false); - break; + if(TRY_USE_NATIVE_HMAC == true){ + /* Check native HMAC support on the card + * NOTE: HMAC implementation is unfortunately not present on the tested cards so far ... + * but we prepare its usage on compatible cards! + */ + use_native_hmac = true; + try { + switch(digest_type){ + case MessageDigest.ALG_SHA_224: + CryptoException.throwIt(CryptoException.NO_SUCH_ALGORITHM); + break; + case MessageDigest.ALG_SHA_256: + try { + hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC_TRANSIENT_DESELECT, KeyBuilder.LENGTH_HMAC_SHA_256_BLOCK_64, true); + } + catch(CryptoException e){ + hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC_TRANSIENT_DESELECT, KeyBuilder.LENGTH_HMAC_SHA_256_BLOCK_64, false); + } + hmac_instance = Signature.getInstance(Signature.ALG_HMAC_SHA_256, false); + break; + case MessageDigest.ALG_SHA_384: + try { + hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC_TRANSIENT_DESELECT, KeyBuilder.LENGTH_HMAC_SHA_384_BLOCK_128, true); + } + catch(CryptoException e){ + hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC_TRANSIENT_DESELECT, KeyBuilder.LENGTH_HMAC_SHA_384_BLOCK_128, false); + } + hmac_instance = Signature.getInstance(Signature.ALG_HMAC_SHA_384, false); + break; + case MessageDigest.ALG_SHA_512: + try { + hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC_TRANSIENT_DESELECT, KeyBuilder.LENGTH_HMAC_SHA_512_BLOCK_128, true); + } + catch(CryptoException e){ + hmac_key = (HMACKey) KeyBuilder.buildKey(KeyBuilder.TYPE_HMAC_TRANSIENT_DESELECT, KeyBuilder.LENGTH_HMAC_SHA_512_BLOCK_128, false); + } + hmac_instance = Signature.getInstance(Signature.ALG_HMAC_SHA_512, false); + break; + } + } + catch(CryptoException exception){ + /* If we got an exception, this means that the card does not have a + * native HMAC support. Fallback to the software one! + */ + use_native_hmac = false; } } - catch(CryptoException exception){ - use_native_hmac = false; - } - if(use_native_hmac == false){ if((ipad != null) || (opad != null) || (md_i != null) || (md_o != null)){ CryptoException.throwIt(CryptoException.INVALID_INIT); diff --git a/src/wookey/common/SecureChannel.java b/src/wookey/common/SecureChannel.java index e2e73f8..a7c57fd 100644 --- a/src/wookey/common/SecureChannel.java +++ b/src/wookey/common/SecureChannel.java @@ -52,8 +52,6 @@ protected SecureChannel(byte[] default_pin, byte[] OurPrivKeyBuf, byte[] OurPubK public void initialize_eeprom(byte[] default_pin, byte[] OurPrivKeyBuf, byte[] OurPubKeyBuf, byte[] WooKeyPubKeyBuf, byte[] LibECCparams){ try { /* Initialize our long term variables */ - /* We do this in a transaction to be safer ... */ - JCSystem.beginTransaction(); /* Initialize all the crypto algorithms contexts */ hmac_ctx = new Hmac(MessageDigest.ALG_SHA_256); @@ -79,15 +77,9 @@ public void initialize_eeprom(byte[] default_pin, byte[] OurPrivKeyBuf, byte[] O ec_context.initialize_EC_key_pair_context(null, false, WooKeyPubKeyBuf, WooKeyKeyPairWrapper); WooKeyKeyPair = WooKeyKeyPairWrapper.kp; WooKeyPubKey = WooKeyKeyPairWrapper.PubKey; - - /* Commit our transaction */ - JCSystem.commitTransaction(); } catch(CryptoException exception) { - /* Abort our transaction */ - JCSystem.abortTransaction(); - switch(exception.getReason()){ case CryptoException.ILLEGAL_USE: ISOException.throwIt((short) 0x6BD0); @@ -111,8 +103,6 @@ public void initialize_eeprom(byte[] default_pin, byte[] OurPrivKeyBuf, byte[] O } catch(Exception e) { - /* Abort our transaction */ - JCSystem.abortTransaction(); ISOException.throwIt((short) 0x6BD6); } @@ -352,7 +342,7 @@ public void send_encrypted_apdu(APDU apdu, byte[] indata, short indataoffset, sh hmac_ctx.hmac_update(tmp, (short) 1, (short) 1); if(indatalen > 0){ aes_ctr_ctx.aes_init(AES_key, IV, Aes.DECRYPT); - aes_ctr_ctx.aes(indata, (short) 0, indatalen, working_buffer, (short) 0); + aes_ctr_ctx.aes(indata, indataoffset, indatalen, working_buffer, (short) 0); /* Increment the IV by as many blocks as necessary */ add_iv((short) (indatalen / Aes.AES_BLOCK_SIZE)); tmp[0] = (byte)indatalen; diff --git a/src/wookey/common/WooKey.java b/src/wookey/common/WooKey.java index a878423..497df52 100644 --- a/src/wookey/common/WooKey.java +++ b/src/wookey/common/WooKey.java @@ -3,7 +3,10 @@ public class WooKey { - private static final boolean DEBUG_MODE = true; + /* NOTE: the debug mode activates echo tests for the secure channel. + * Should be removed in production. + */ + private static final boolean DEBUG_MODE = false; /* The secure channel instance */ public SecureChannel schannel; diff --git a/src/wookey/dfu/WooKeyDFU.java b/src/wookey/dfu/WooKeyDFU.java index f00061f..836d984 100644 --- a/src/wookey/dfu/WooKeyDFU.java +++ b/src/wookey/dfu/WooKeyDFU.java @@ -40,6 +40,8 @@ public class WooKeyDFU extends Applet implements ExtendedLength /* Useful tmp buffer */ private static byte[] tmp = null; + /* Variable handling initialization */ + private boolean init_done = false; public static void install(byte[] bArray, short bOffset, byte bLength) @@ -244,17 +246,24 @@ public void process(APDU apdu) return; } - if(W == null){ + if((W == null) || (init_done == false)){ + init_done = false; W = new WooKey(Keys.UserPin, Keys.PetPin, Keys.OurPrivKeyBuf, Keys.OurPubKeyBuf, Keys.WooKeyPubKeyBuf, Keys.LibECCparams, Keys.PetName, Keys.PetNameLength, Keys.max_pin_tries, Keys.max_secure_channel_tries); /* Get the shared crypto contexts with the secure channel layer. This is done to **save memory**. */ /* HMAC context */ hmac_ctx = W.schannel.hmac_ctx; /* AES context */ aes_cbc_ctx = W.schannel.aes_cbc_ctx; + init_done = true; + } + if(init_done == false){ + ISOException.throwIt((short) 0x6660); + } + if(buffer[ISO7816.OFFSET_CLA] != (byte)0x00){ - ISOException.throwIt((short) 0x6660); + ISOException.throwIt((short) 0x6661); } /* Begin to handle the common APDUs */ diff --git a/src/wookey/sig/WooKeySIG.java b/src/wookey/sig/WooKeySIG.java index c6162db..70779c3 100644 --- a/src/wookey/sig/WooKeySIG.java +++ b/src/wookey/sig/WooKeySIG.java @@ -54,6 +54,9 @@ public class WooKeySIG extends Applet implements ExtendedLength public static final byte TOKEN_INS_VERIFY_FIRMWARE = (byte) 0x33; public static final byte TOKEN_INS_GET_SIG_TYPE = (byte) 0x34; + /* Variable handling initialization */ + private boolean init_done = false; + public static void install(byte[] bArray, short bOffset, byte bLength) { @@ -354,7 +357,8 @@ public void process(APDU apdu) return; } - if(W == null){ + if((W == null) || (init_done == false)){ + init_done = false; /* Instantiate WooKey common class */ W = new WooKey(Keys.UserPin, Keys.PetPin, Keys.OurPrivKeyBuf, Keys.OurPubKeyBuf, Keys.WooKeyPubKeyBuf, Keys.LibECCparams, Keys.PetName, Keys.PetNameLength, Keys.max_pin_tries, Keys.max_secure_channel_tries); /* Import the firmware signature keys once and for all */ @@ -371,10 +375,15 @@ public void process(APDU apdu) hmac_ctx = W.schannel.hmac_ctx; /* AES context */ aes_cbc_ctx = W.schannel.aes_cbc_ctx; + init_done = true; } + if(init_done == false){ + ISOException.throwIt((short) 0x6660); + } + if(buffer[ISO7816.OFFSET_CLA] != (byte)0x00){ - ISOException.throwIt((short) 0x6660); + ISOException.throwIt((short) 0x6661); } /* Begin to handle the common APDUs */