Skip to content

Commit

Permalink
[bugfixes]
Browse files Browse the repository at this point in the history 
	- 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
  • Loading branch information
@rben-dev @PThierry
rben-dev authored and PThierry committed Sep 5, 2019
1 parent 6248568 commit 3cf8b8a
Show file tree
Hide file tree
Showing 7 changed files with 109 additions and 48 deletions.
17 changes: 13 additions & 4 deletions src/wookey/auth/WooKeyAuth.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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){
Expand Down
14 changes: 12 additions & 2 deletions src/wookey/common/Aes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down
83 changes: 57 additions & 26 deletions src/wookey/common/Hmac.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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;
Expand All @@ -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);
Expand Down
12 changes: 1 addition & 11 deletions src/wookey/common/SecureChannel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
}

Expand Down Expand Up @@ -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;
Expand Down
5 changes: 4 additions & 1 deletion src/wookey/common/WooKey.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
13 changes: 11 additions & 2 deletions src/wookey/dfu/WooKeyDFU.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 */
Expand Down
13 changes: 11 additions & 2 deletions src/wookey/sig/WooKeySIG.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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 */
Expand 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 */
Expand Down

0 comments on commit 3cf8b8a

Please sign in to comment.