From 3236665599673ef51c7e82cf7ae117bba935f264 Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Thu, 18 Mar 2021 20:46:36 +0000 Subject: [PATCH 1/5] Fix the issue in the upgrade key command. Allow upgrade if any of the boot parameters gets changed. Signed-off-by: BKSSM Venkateswarlu --- .../javacard/test/KMFunctionalTest.java | 76 ++++++++++++++---- .../javacard/keymaster/KMKeymasterApplet.java | 80 ++++++++++++------- 2 files changed, 110 insertions(+), 46 deletions(-) diff --git a/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java b/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java index bc599510..7616bc58 100644 --- a/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java +++ b/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java @@ -446,6 +446,10 @@ public class KMFunctionalTest { (byte) 0xe9, (byte) 0x77, (byte) 0x4c, (byte) 0x45, (byte) 0xc3, (byte) 0xa3, (byte) 0xcf, (byte) 0x0d, (byte) 0x16, (byte) 0x10, (byte) 0xe4, (byte) 0x79, (byte) 0x43, (byte) 0x3a, (byte) 0x21, (byte) 0x5a, (byte) 0x30, (byte) 0xcf}; + private static final int OS_VERSION = 1; + private static final int OS_PATCH_LEVEL = 1; + private static final int VENDOR_PATCH_LEVEL = 1; + private static final int BOOT_PATCH_LEVEL = 1; private CardSimulator simulator; private KMEncoder encoder; @@ -657,7 +661,8 @@ private void provisionCmd(CardSimulator simulator) { provisionSharedSecret(simulator); provisionAttestIds(simulator); // set bootup parameters - setBootParams(simulator, (short) 1, (short) 1, (short) 0, (short) 0); + setBootParams(simulator, (short) OS_VERSION, (short) OS_PATCH_LEVEL, + (short) VENDOR_PATCH_LEVEL, (short) BOOT_PATCH_LEVEL); provisionLocked(simulator); } @@ -2699,18 +2704,61 @@ public void testUpgradeKey() { osPatch = KMIntegerTag.cast(osPatch).getValue(); Assert.assertEquals(KMInteger.cast(osVersion).getShort(), 1); Assert.assertEquals(KMInteger.cast(osPatch).getShort(), 1); - setBootParams(simulator, (short) 2, (short) 2, (short) 1, (short) 1); - ret = upgradeKey(KMByteBlob.instance(keyBlob, (short) 0, (short) keyBlob.length), null, null); - keyBlobPtr = KMArray.cast(ret).get((short) 1); - ret = getKeyCharacteristics(keyBlobPtr); - keyCharacteristics = KMArray.cast(ret).get((short) 1); - hwParams = KMKeyCharacteristics.cast(keyCharacteristics).getHardwareEnforced(); - osVersion = KMKeyParameters.findTag(KMType.UINT_TAG, KMType.OS_VERSION, hwParams); - osVersion = KMIntegerTag.cast(osVersion).getValue(); - osPatch = KMKeyParameters.findTag(KMType.UINT_TAG, KMType.OS_PATCH_LEVEL, hwParams); - osPatch = KMIntegerTag.cast(osPatch).getValue(); - Assert.assertEquals(KMInteger.cast(osVersion).getShort(), 2); - Assert.assertEquals(KMInteger.cast(osPatch).getShort(), 2); + short NO_UPGRADE = 0x01; + short UPGRADE = 0x02; + short[][] test_data = { + {OS_VERSION, OS_PATCH_LEVEL, VENDOR_PATCH_LEVEL, BOOT_PATCH_LEVEL, NO_UPGRADE, KMError.OK }, + {OS_VERSION+1, OS_PATCH_LEVEL, VENDOR_PATCH_LEVEL, BOOT_PATCH_LEVEL, UPGRADE, KMError.OK }, + {OS_VERSION, OS_PATCH_LEVEL+1, VENDOR_PATCH_LEVEL, BOOT_PATCH_LEVEL, UPGRADE, KMError.OK }, + {OS_VERSION, OS_PATCH_LEVEL, VENDOR_PATCH_LEVEL+1, BOOT_PATCH_LEVEL, UPGRADE, KMError.OK }, + {OS_VERSION, OS_PATCH_LEVEL, VENDOR_PATCH_LEVEL, BOOT_PATCH_LEVEL+1, UPGRADE, KMError.OK }, + {OS_VERSION+1, OS_PATCH_LEVEL+1, VENDOR_PATCH_LEVEL+1, BOOT_PATCH_LEVEL+1, UPGRADE, KMError.OK }, + {OS_VERSION+1, OS_PATCH_LEVEL, VENDOR_PATCH_LEVEL+1, BOOT_PATCH_LEVEL, UPGRADE, KMError.OK }, + {OS_VERSION+1, OS_PATCH_LEVEL+1, VENDOR_PATCH_LEVEL, BOOT_PATCH_LEVEL, UPGRADE, KMError.OK }, + {OS_VERSION, OS_PATCH_LEVEL, VENDOR_PATCH_LEVEL, BOOT_PATCH_LEVEL-1, NO_UPGRADE, KMError.INVALID_ARGUMENT }, + {OS_VERSION-1/*0*/, OS_PATCH_LEVEL, VENDOR_PATCH_LEVEL, BOOT_PATCH_LEVEL, UPGRADE, KMError.OK }, + {OS_VERSION, OS_PATCH_LEVEL, VENDOR_PATCH_LEVEL-1, BOOT_PATCH_LEVEL, NO_UPGRADE, KMError.INVALID_ARGUMENT }, + {OS_VERSION, OS_PATCH_LEVEL+1, VENDOR_PATCH_LEVEL-1, BOOT_PATCH_LEVEL, NO_UPGRADE, KMError.INVALID_ARGUMENT }, + {0, OS_PATCH_LEVEL+1, VENDOR_PATCH_LEVEL-1, BOOT_PATCH_LEVEL+1, NO_UPGRADE, KMError.INVALID_ARGUMENT }, + }; + for (int i = 0; i < test_data.length; i++) { + setBootParams(simulator, (short) test_data[i][0], (short) test_data[i][1], + (short) test_data[i][2], (short) test_data[i][3]); + ret = upgradeKey( + KMByteBlob.instance(keyBlob, (short) 0, (short) keyBlob.length), + null, null); + Assert.assertEquals(test_data[i][5], + KMInteger.cast(KMArray.cast(ret).get((short) 0)).getShort()); + keyBlobPtr = KMArray.cast(ret).get((short) 1); + if (test_data[i][4] == UPGRADE) Assert.assertNotEquals(KMByteBlob.cast(keyBlobPtr).length(), 0); + else Assert.assertEquals(KMByteBlob.cast(keyBlobPtr).length(), 0); + if (KMByteBlob.cast(keyBlobPtr).length() != 0) { + ret = getKeyCharacteristics(keyBlobPtr); + keyCharacteristics = KMArray.cast(ret).get((short) 1); + hwParams = KMKeyCharacteristics.cast(keyCharacteristics) + .getHardwareEnforced(); + osVersion = KMKeyParameters.findTag(KMType.UINT_TAG, KMType.OS_VERSION, + hwParams); + osVersion = KMIntegerTag.cast(osVersion).getValue(); + osPatch = KMKeyParameters.findTag(KMType.UINT_TAG, + KMType.OS_PATCH_LEVEL, hwParams); + osPatch = KMIntegerTag.cast(osPatch).getValue(); + short ptr = KMKeyParameters.findTag(KMType.UINT_TAG, + KMType.VENDOR_PATCH_LEVEL, hwParams); + short vendorPatchLevel = KMIntegerTag.cast(ptr).getValue(); + ptr = KMKeyParameters.findTag(KMType.UINT_TAG, KMType.BOOT_PATCH_LEVEL, + hwParams); + short bootPatchLevel = KMIntegerTag.cast(ptr).getValue(); + Assert.assertEquals(KMInteger.cast(osVersion).getShort(), + test_data[i][0]); + Assert.assertEquals(KMInteger.cast(osPatch).getShort(), + test_data[i][1]); + Assert.assertEquals(KMInteger.cast(vendorPatchLevel).getShort(), + test_data[i][2]); + Assert.assertEquals(KMInteger.cast(bootPatchLevel).getShort(), + test_data[i][3]); + } + } cleanUp(); } @@ -2759,8 +2807,6 @@ private short upgradeKey(short keyBlobPtr, byte[] clientId, byte[] appData) { byte[] respBuf = response.getBytes(); short len = (short) respBuf.length; ret = decoder.decode(ret, respBuf, (short) 0, len); - short error = KMInteger.cast(KMArray.cast(ret).get((short) 0)).getShort(); - Assert.assertEquals(error, KMError.OK); return ret; } diff --git a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java index b79cabaa..a14bdce5 100644 --- a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java +++ b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java @@ -1123,6 +1123,9 @@ private void processUpgradeKeyCmd(APDU apdu) { } // parse existing key blob parseEncryptedKeyBlob(scratchPad); + // Use tmpVariables[5] to carry the error code and tmpVariables[6] to check if key needs upgrade. + tmpVariables[5] = KMError.OK; + tmpVariables[6] = 0; // validate characteristics to be upgraded. tmpVariables[0] = KMKeyParameters.findTag(KMType.UINT_TAG, KMType.OS_VERSION, data[HW_PARAMETERS]); @@ -1134,50 +1137,65 @@ private void processUpgradeKeyCmd(APDU apdu) { tmpVariables[3] = repository.getOsPatch(); tmpVariables[4] = KMInteger.uint_8((byte) 0); if (tmpVariables[0] != KMType.INVALID_VALUE) { - // os version in key characteristics must be less the os version stored in javacard or the + // OS version in key characteristics must be less the os version stored in javacard or the // stored version must be zero. Then only upgrade is allowed else it is invalid argument. - if (KMInteger.compare(tmpVariables[0], tmpVariables[2]) != -1 - && KMInteger.compare(tmpVariables[2], tmpVariables[4]) != 0) { - // Key Should not be upgraded, but error code should be OK, As per VTS. + if ((KMInteger.compare(tmpVariables[0], tmpVariables[2]) == 1 + && KMInteger.compare(tmpVariables[2], tmpVariables[4]) == 0) || + (KMInteger.compare(tmpVariables[0], tmpVariables[2]) == -1)) { + // Key requires upgrade. + tmpVariables[6] = 1; + } else if (KMInteger.compare(tmpVariables[1], tmpVariables[3]) == 1) { tmpVariables[5] = KMError.INVALID_ARGUMENT; } } - if (tmpVariables[1] != KMType.INVALID_VALUE) { + if (tmpVariables[5] != KMError.INVALID_ARGUMENT && tmpVariables[1] != KMType.INVALID_VALUE) { // The key characteristics should have had os patch level < os patch level stored in javacard // then only upgrade is allowed. - if (KMInteger.compare(tmpVariables[1], tmpVariables[3]) != -1) { - // Key Should not be upgraded, but error code should be OK, As per VTS. + if (KMInteger.compare(tmpVariables[1], tmpVariables[3]) == 1) { tmpVariables[5] = KMError.INVALID_ARGUMENT; + } else if (KMInteger.compare(tmpVariables[1], tmpVariables[3]) == -1) { + // Key requires upgrade. + tmpVariables[6] = 1; } } - //Compare vendor patch levels - tmpVariables[1] = - KMKeyParameters.findTag(KMType.UINT_TAG, KMType.VENDOR_PATCH_LEVEL, data[HW_PARAMETERS]); - tmpVariables[1] = KMIntegerTag.cast(tmpVariables[1]).getValue(); - tmpVariables[2] = repository.getVendorPatchLevel(); - if (tmpVariables[1] != KMType.INVALID_VALUE) { - // The key characteristics should have had vendor patch level < vendor patch level stored in javacard - // then only upgrade is allowed. - if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) != -1) { - // Key Should not be upgraded, but error code should be OK, As per VTS. - tmpVariables[5] = KMError.INVALID_ARGUMENT; + if (tmpVariables[5] != KMError.INVALID_ARGUMENT) { + // Compare vendor patch levels + tmpVariables[1] = KMKeyParameters.findTag(KMType.UINT_TAG, + KMType.VENDOR_PATCH_LEVEL, data[HW_PARAMETERS]); + tmpVariables[1] = KMIntegerTag.cast(tmpVariables[1]).getValue(); + tmpVariables[2] = repository.getVendorPatchLevel(); + if (tmpVariables[1] != KMType.INVALID_VALUE) { + // The key characteristics should have had vendor patch level < vendor + // patch level stored in javacard + // then only upgrade is allowed. + if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) == 1) { + tmpVariables[5] = KMError.INVALID_ARGUMENT; + } else if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) == -1) { + // Key requires upgrade. + tmpVariables[6] = 1; + } } } - //Compare boot patch levels - tmpVariables[1] = - KMKeyParameters.findTag(KMType.UINT_TAG, KMType.BOOT_PATCH_LEVEL, data[HW_PARAMETERS]); - tmpVariables[1] = KMIntegerTag.cast(tmpVariables[1]).getValue(); - tmpVariables[2] = repository.getBootPatchLevel(); - if (tmpVariables[1] != KMType.INVALID_VALUE) { - // The key characteristics should have had boot patch level < boot patch level stored in javacard - // then only upgrade is allowed. - if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) != -1) { - // Key Should not be upgraded, but error code should be OK, As per VTS. - tmpVariables[5] = KMError.INVALID_ARGUMENT; + if (tmpVariables[5] != KMError.INVALID_ARGUMENT) { + // Compare boot patch levels + tmpVariables[1] = KMKeyParameters.findTag(KMType.UINT_TAG, + KMType.BOOT_PATCH_LEVEL, data[HW_PARAMETERS]); + tmpVariables[1] = KMIntegerTag.cast(tmpVariables[1]).getValue(); + tmpVariables[2] = repository.getBootPatchLevel(); + if (tmpVariables[1] != KMType.INVALID_VALUE) { + // The key characteristics should have had boot patch level < boot patch + // level stored in javacard + // then only upgrade is allowed. + if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) == 1) { + tmpVariables[5] = KMError.INVALID_ARGUMENT; + } else if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) == -1) { + // Key requires upgrade. + tmpVariables[6] = 1; + } } } - if (tmpVariables[5] != KMError.INVALID_ARGUMENT) { + if (tmpVariables[5] != KMError.INVALID_ARGUMENT && tmpVariables[6] == 1) { // copy origin data[ORIGIN] = KMEnumTag.getValue(KMType.ORIGIN, data[HW_PARAMETERS]); // create new key blob with current os version etc. @@ -1187,7 +1205,7 @@ private void processUpgradeKeyCmd(APDU apdu) { } // prepare the response tmpVariables[0] = KMArray.instance((short) 2); - KMArray.cast(tmpVariables[0]).add((short) 0, KMInteger.uint_16(KMError.OK)); + KMArray.cast(tmpVariables[0]).add((short) 0, KMInteger.uint_16(tmpVariables[5])); KMArray.cast(tmpVariables[0]).add((short) 1, data[KEY_BLOB]); bufferStartOffset = repository.allocAvailableMemory(); From 066ac9fe8689ff206510b49e3cb456410e641423 Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Fri, 19 Mar 2021 20:35:13 +0000 Subject: [PATCH 2/5] Optimized the upgradeKey command flow --- .../javacard/test/KMFunctionalTest.java | 6 +- .../javacard/keymaster/KMKeymasterApplet.java | 109 ++++++------------ 2 files changed, 41 insertions(+), 74 deletions(-) diff --git a/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java b/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java index 7616bc58..17db4c8e 100644 --- a/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java +++ b/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java @@ -2730,8 +2730,10 @@ public void testUpgradeKey() { Assert.assertEquals(test_data[i][5], KMInteger.cast(KMArray.cast(ret).get((short) 0)).getShort()); keyBlobPtr = KMArray.cast(ret).get((short) 1); - if (test_data[i][4] == UPGRADE) Assert.assertNotEquals(KMByteBlob.cast(keyBlobPtr).length(), 0); - else Assert.assertEquals(KMByteBlob.cast(keyBlobPtr).length(), 0); + if (test_data[i][4] == UPGRADE) + Assert.assertNotEquals(KMByteBlob.cast(keyBlobPtr).length(), 0); + else + Assert.assertEquals(KMByteBlob.cast(keyBlobPtr).length(), 0); if (KMByteBlob.cast(keyBlobPtr).length() != 0) { ret = getKeyCharacteristics(keyBlobPtr); keyCharacteristics = KMArray.cast(ret).get((short) 1); diff --git a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java index a14bdce5..bff2f166 100644 --- a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java +++ b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java @@ -1096,6 +1096,25 @@ private void processComputeSharedHmacCmd(APDU apdu) { sendOutgoing(apdu); } + private byte validateUpgradeKeyForTag(short tag, short systemParam) { + // validate characteristics to be upgraded. + tmpVariables[0] = KMKeyParameters.findTag(KMType.UINT_TAG, tag, data[HW_PARAMETERS]); + tmpVariables[0] = KMIntegerTag.cast(tmpVariables[0]).getValue(); + tmpVariables[1] = KMInteger.uint_8((byte) 0); + if (tmpVariables[0] != KMType.INVALID_VALUE) { + if ((tag == KMType.OS_VERSION + && KMInteger.compare(tmpVariables[0], systemParam) == 1 + && KMInteger.compare(systemParam, tmpVariables[1]) == 0) + || (KMInteger.compare(tmpVariables[0], systemParam) == -1)) { + // Key needs upgrade. + return (byte) 1; + } else if (KMInteger.compare(tmpVariables[0], systemParam) == 1) { + KMException.throwIt(KMError.INVALID_ARGUMENT); + } + } + return (byte) 0; + } + private void processUpgradeKeyCmd(APDU apdu) { // Receive the incoming request fully from the master into buffer. receiveIncoming(apdu); @@ -1123,79 +1142,25 @@ private void processUpgradeKeyCmd(APDU apdu) { } // parse existing key blob parseEncryptedKeyBlob(scratchPad); - // Use tmpVariables[5] to carry the error code and tmpVariables[6] to check if key needs upgrade. - tmpVariables[5] = KMError.OK; - tmpVariables[6] = 0; - // validate characteristics to be upgraded. - tmpVariables[0] = - KMKeyParameters.findTag(KMType.UINT_TAG, KMType.OS_VERSION, data[HW_PARAMETERS]); - tmpVariables[0] = KMIntegerTag.cast(tmpVariables[0]).getValue(); - tmpVariables[1] = - KMKeyParameters.findTag(KMType.UINT_TAG, KMType.OS_PATCH_LEVEL, data[HW_PARAMETERS]); - tmpVariables[1] = KMIntegerTag.cast(tmpVariables[1]).getValue(); - tmpVariables[2] = repository.getOsVersion(); - tmpVariables[3] = repository.getOsPatch(); - tmpVariables[4] = KMInteger.uint_8((byte) 0); - if (tmpVariables[0] != KMType.INVALID_VALUE) { - // OS version in key characteristics must be less the os version stored in javacard or the - // stored version must be zero. Then only upgrade is allowed else it is invalid argument. - if ((KMInteger.compare(tmpVariables[0], tmpVariables[2]) == 1 - && KMInteger.compare(tmpVariables[2], tmpVariables[4]) == 0) || - (KMInteger.compare(tmpVariables[0], tmpVariables[2]) == -1)) { - // Key requires upgrade. - tmpVariables[6] = 1; - } else if (KMInteger.compare(tmpVariables[1], tmpVariables[3]) == 1) { - tmpVariables[5] = KMError.INVALID_ARGUMENT; - } - } - if (tmpVariables[5] != KMError.INVALID_ARGUMENT && tmpVariables[1] != KMType.INVALID_VALUE) { - // The key characteristics should have had os patch level < os patch level stored in javacard - // then only upgrade is allowed. - if (KMInteger.compare(tmpVariables[1], tmpVariables[3]) == 1) { - tmpVariables[5] = KMError.INVALID_ARGUMENT; - } else if (KMInteger.compare(tmpVariables[1], tmpVariables[3]) == -1) { - // Key requires upgrade. - tmpVariables[6] = 1; - } - } - if (tmpVariables[5] != KMError.INVALID_ARGUMENT) { - // Compare vendor patch levels - tmpVariables[1] = KMKeyParameters.findTag(KMType.UINT_TAG, - KMType.VENDOR_PATCH_LEVEL, data[HW_PARAMETERS]); - tmpVariables[1] = KMIntegerTag.cast(tmpVariables[1]).getValue(); - tmpVariables[2] = repository.getVendorPatchLevel(); - if (tmpVariables[1] != KMType.INVALID_VALUE) { - // The key characteristics should have had vendor patch level < vendor - // patch level stored in javacard - // then only upgrade is allowed. - if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) == 1) { - tmpVariables[5] = KMError.INVALID_ARGUMENT; - } else if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) == -1) { - // Key requires upgrade. - tmpVariables[6] = 1; - } - } - } - if (tmpVariables[5] != KMError.INVALID_ARGUMENT) { - // Compare boot patch levels - tmpVariables[1] = KMKeyParameters.findTag(KMType.UINT_TAG, - KMType.BOOT_PATCH_LEVEL, data[HW_PARAMETERS]); - tmpVariables[1] = KMIntegerTag.cast(tmpVariables[1]).getValue(); - tmpVariables[2] = repository.getBootPatchLevel(); - if (tmpVariables[1] != KMType.INVALID_VALUE) { - // The key characteristics should have had boot patch level < boot patch - // level stored in javacard - // then only upgrade is allowed. - if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) == 1) { - tmpVariables[5] = KMError.INVALID_ARGUMENT; - } else if (KMInteger.compare(tmpVariables[1], tmpVariables[2]) == -1) { - // Key requires upgrade. - tmpVariables[6] = 1; - } - } + // Use tmpVariables[4] to store the error code. + tmpVariables[4] = KMError.OK; + // Use tmpVariables[3] to check if key needs upgrade. + tmpVariables[3] = 0; + try { + tmpVariables[3] |= validateUpgradeKeyForTag(KMType.OS_VERSION, repository.getOsVersion()); + tmpVariables[3] |= validateUpgradeKeyForTag(KMType.OS_PATCH_LEVEL, repository.getOsPatch()); + tmpVariables[3] |= validateUpgradeKeyForTag(KMType.VENDOR_PATCH_LEVEL, repository.getVendorPatchLevel()); + tmpVariables[3] |= validateUpgradeKeyForTag(KMType.BOOT_PATCH_LEVEL, repository.getBootPatchLevel()); + } catch (KMException e) { + + if (KMException.reason != KMError.INVALID_ARGUMENT) + KMException.throwIt(KMException.reason); + // Key should not be upgraded and return error. + tmpVariables[3] = 0; + tmpVariables[4] = KMException.reason; } - if (tmpVariables[5] != KMError.INVALID_ARGUMENT && tmpVariables[6] == 1) { + if (tmpVariables[3] == 1) { // copy origin data[ORIGIN] = KMEnumTag.getValue(KMType.ORIGIN, data[HW_PARAMETERS]); // create new key blob with current os version etc. @@ -1205,7 +1170,7 @@ private void processUpgradeKeyCmd(APDU apdu) { } // prepare the response tmpVariables[0] = KMArray.instance((short) 2); - KMArray.cast(tmpVariables[0]).add((short) 0, KMInteger.uint_16(tmpVariables[5])); + KMArray.cast(tmpVariables[0]).add((short) 0, KMInteger.uint_16(tmpVariables[4])); KMArray.cast(tmpVariables[0]).add((short) 1, data[KEY_BLOB]); bufferStartOffset = repository.allocAvailableMemory(); From 24a4375ea2d10efcd6fa69e71d3839c97b823294 Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Sun, 21 Mar 2021 11:16:52 +0000 Subject: [PATCH 3/5] Incorporated review comments --- .../javacard/test/KMFunctionalTest.java | 26 ++++++---- .../javacard/keymaster/KMKeymasterApplet.java | 47 +++++++++---------- 2 files changed, 38 insertions(+), 35 deletions(-) diff --git a/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java b/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java index 17db4c8e..9e9c2c6c 100644 --- a/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java +++ b/Applet/JCardSimProvider/test/com/android/javacard/test/KMFunctionalTest.java @@ -2726,9 +2726,9 @@ public void testUpgradeKey() { (short) test_data[i][2], (short) test_data[i][3]); ret = upgradeKey( KMByteBlob.instance(keyBlob, (short) 0, (short) keyBlob.length), - null, null); - Assert.assertEquals(test_data[i][5], - KMInteger.cast(KMArray.cast(ret).get((short) 0)).getShort()); + null, null, test_data[i][5]); + if (test_data[i][5] != KMError.OK) + continue; keyBlobPtr = KMArray.cast(ret).get((short) 1); if (test_data[i][4] == UPGRADE) Assert.assertNotEquals(KMByteBlob.cast(keyBlobPtr).length(), 0); @@ -2774,7 +2774,7 @@ public void testDestroyAttIds() { cleanUp(); } - private short upgradeKey(short keyBlobPtr, byte[] clientId, byte[] appData) { + private short upgradeKey(short keyBlobPtr, byte[] clientId, byte[] appData, short expectedErr) { short tagCount = 0; short clientIdTag = 0; short appDataTag = 0; @@ -2803,13 +2803,21 @@ private short upgradeKey(short keyBlobPtr, byte[] clientId, byte[] appData) { CommandAPDU apdu = encodeApdu((byte) INS_UPGRADE_KEY_CMD, arr); // print(commandAPDU.getBytes()); ResponseAPDU response = simulator.transmitCommand(apdu); - short ret = KMArray.instance((short) 2); - KMArray.cast(ret).add((short) 0, KMInteger.exp()); - KMArray.cast(ret).add((short) 1, KMByteBlob.exp()); byte[] respBuf = response.getBytes(); short len = (short) respBuf.length; - ret = decoder.decode(ret, respBuf, (short) 0, len); - return ret; + if (KMError.OK == expectedErr) { + short ret = KMArray.instance((short) 2); + KMArray.cast(ret).add((short) 0, KMInteger.exp()); + KMArray.cast(ret).add((short) 1, KMByteBlob.exp()); + ret = decoder.decode(ret, respBuf, (short) 0, len); + Assert.assertEquals(expectedErr, KMInteger.cast(KMArray.cast(ret).get((short) 0)).getShort()); + return ret; + } else { + short ret = KMInteger.exp(); + ret = decoder.decode(ret, respBuf, (short) 0, len); + Assert.assertEquals(expectedErr, KMInteger.cast(ret).getShort()); + return ret; + } } @Test diff --git a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java index bff2f166..3d2a2a9a 100644 --- a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java +++ b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java @@ -1096,23 +1096,29 @@ private void processComputeSharedHmacCmd(APDU apdu) { sendOutgoing(apdu); } - private byte validateUpgradeKeyForTag(short tag, short systemParam) { - // validate characteristics to be upgraded. + private boolean keyRequiresUpgrade(short tag, short systemParam) { + // validate the tag to be upgraded. tmpVariables[0] = KMKeyParameters.findTag(KMType.UINT_TAG, tag, data[HW_PARAMETERS]); tmpVariables[0] = KMIntegerTag.cast(tmpVariables[0]).getValue(); tmpVariables[1] = KMInteger.uint_8((byte) 0); if (tmpVariables[0] != KMType.INVALID_VALUE) { + // OS version in key characteristics must be less the OS version stored in Javacard or the + // stored version must be zero. Then only upgrade is allowed else it is invalid argument. if ((tag == KMType.OS_VERSION && KMInteger.compare(tmpVariables[0], systemParam) == 1 - && KMInteger.compare(systemParam, tmpVariables[1]) == 0) - || (KMInteger.compare(tmpVariables[0], systemParam) == -1)) { + && KMInteger.compare(systemParam, tmpVariables[1]) == 0)) { // Key needs upgrade. - return (byte) 1; + return true; + } else if ((KMInteger.compare(tmpVariables[0], systemParam) == -1)) { + // Each os version or patch level associated with the key must be less than it's + // corresponding value stored in Javacard, then only upgrade is allowed otherwise it + // is invalid argument. + return true; } else if (KMInteger.compare(tmpVariables[0], systemParam) == 1) { KMException.throwIt(KMError.INVALID_ARGUMENT); } } - return (byte) 0; + return false; } private void processUpgradeKeyCmd(APDU apdu) { @@ -1142,25 +1148,14 @@ private void processUpgradeKeyCmd(APDU apdu) { } // parse existing key blob parseEncryptedKeyBlob(scratchPad); - // Use tmpVariables[4] to store the error code. - tmpVariables[4] = KMError.OK; - // Use tmpVariables[3] to check if key needs upgrade. - tmpVariables[3] = 0; - try { - tmpVariables[3] |= validateUpgradeKeyForTag(KMType.OS_VERSION, repository.getOsVersion()); - tmpVariables[3] |= validateUpgradeKeyForTag(KMType.OS_PATCH_LEVEL, repository.getOsPatch()); - tmpVariables[3] |= validateUpgradeKeyForTag(KMType.VENDOR_PATCH_LEVEL, repository.getVendorPatchLevel()); - tmpVariables[3] |= validateUpgradeKeyForTag(KMType.BOOT_PATCH_LEVEL, repository.getBootPatchLevel()); - } catch (KMException e) { - - if (KMException.reason != KMError.INVALID_ARGUMENT) - KMException.throwIt(KMException.reason); - // Key should not be upgraded and return error. - tmpVariables[3] = 0; - tmpVariables[4] = KMException.reason; - } - - if (tmpVariables[3] == 1) { + boolean keyRequiresUpgrade = false; + // Check if key requires upgrade. + keyRequiresUpgrade |= keyRequiresUpgrade(KMType.OS_VERSION, repository.getOsVersion()); + keyRequiresUpgrade |= keyRequiresUpgrade(KMType.OS_PATCH_LEVEL, repository.getOsPatch()); + keyRequiresUpgrade |= keyRequiresUpgrade(KMType.VENDOR_PATCH_LEVEL, repository.getVendorPatchLevel()); + keyRequiresUpgrade |= keyRequiresUpgrade(KMType.BOOT_PATCH_LEVEL, repository.getBootPatchLevel()); + + if (keyRequiresUpgrade) { // copy origin data[ORIGIN] = KMEnumTag.getValue(KMType.ORIGIN, data[HW_PARAMETERS]); // create new key blob with current os version etc. @@ -1170,7 +1165,7 @@ private void processUpgradeKeyCmd(APDU apdu) { } // prepare the response tmpVariables[0] = KMArray.instance((short) 2); - KMArray.cast(tmpVariables[0]).add((short) 0, KMInteger.uint_16(tmpVariables[4])); + KMArray.cast(tmpVariables[0]).add((short) 0, KMInteger.uint_16(KMError.OK)); KMArray.cast(tmpVariables[0]).add((short) 1, data[KEY_BLOB]); bufferStartOffset = repository.allocAvailableMemory(); From 0f01f36e7cbee0a5738340c55261d096a9e13fc0 Mon Sep 17 00:00:00 2001 From: BKSSMVenkateswarlu <40534495+BKSSMVenkateswarlu@users.noreply.github.com> Date: Sun, 21 Mar 2021 13:37:53 +0000 Subject: [PATCH 4/5] Updated the comment --- .../src/com/android/javacard/keymaster/KMKeymasterApplet.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java index 3d2a2a9a..e8c1a8cc 100644 --- a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java +++ b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java @@ -1097,7 +1097,7 @@ private void processComputeSharedHmacCmd(APDU apdu) { } private boolean keyRequiresUpgrade(short tag, short systemParam) { - // validate the tag to be upgraded. + // validate the tag and check if key needs upgrade. tmpVariables[0] = KMKeyParameters.findTag(KMType.UINT_TAG, tag, data[HW_PARAMETERS]); tmpVariables[0] = KMIntegerTag.cast(tmpVariables[0]).getValue(); tmpVariables[1] = KMInteger.uint_8((byte) 0); From d388973428d4b139c14c74fe0e2891f41cb103a0 Mon Sep 17 00:00:00 2001 From: bvenkateswarlu Date: Mon, 22 Mar 2021 21:22:24 +0000 Subject: [PATCH 5/5] Renamed the keyRequiresUpgrade function with a meaningful name --- .../javacard/keymaster/KMKeymasterApplet.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java index e8c1a8cc..8f40f775 100644 --- a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java +++ b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java @@ -1096,7 +1096,7 @@ private void processComputeSharedHmacCmd(APDU apdu) { sendOutgoing(apdu); } - private boolean keyRequiresUpgrade(short tag, short systemParam) { + private boolean isKeyUpgradeRequired(short tag, short systemParam) { // validate the tag and check if key needs upgrade. tmpVariables[0] = KMKeyParameters.findTag(KMType.UINT_TAG, tag, data[HW_PARAMETERS]); tmpVariables[0] = KMIntegerTag.cast(tmpVariables[0]).getValue(); @@ -1148,14 +1148,14 @@ private void processUpgradeKeyCmd(APDU apdu) { } // parse existing key blob parseEncryptedKeyBlob(scratchPad); - boolean keyRequiresUpgrade = false; + boolean isKeyUpgradeRequired = false; // Check if key requires upgrade. - keyRequiresUpgrade |= keyRequiresUpgrade(KMType.OS_VERSION, repository.getOsVersion()); - keyRequiresUpgrade |= keyRequiresUpgrade(KMType.OS_PATCH_LEVEL, repository.getOsPatch()); - keyRequiresUpgrade |= keyRequiresUpgrade(KMType.VENDOR_PATCH_LEVEL, repository.getVendorPatchLevel()); - keyRequiresUpgrade |= keyRequiresUpgrade(KMType.BOOT_PATCH_LEVEL, repository.getBootPatchLevel()); + isKeyUpgradeRequired |= isKeyUpgradeRequired(KMType.OS_VERSION, repository.getOsVersion()); + isKeyUpgradeRequired |= isKeyUpgradeRequired(KMType.OS_PATCH_LEVEL, repository.getOsPatch()); + isKeyUpgradeRequired |= isKeyUpgradeRequired(KMType.VENDOR_PATCH_LEVEL, repository.getVendorPatchLevel()); + isKeyUpgradeRequired |= isKeyUpgradeRequired(KMType.BOOT_PATCH_LEVEL, repository.getBootPatchLevel()); - if (keyRequiresUpgrade) { + if (isKeyUpgradeRequired) { // copy origin data[ORIGIN] = KMEnumTag.getValue(KMType.ORIGIN, data[HW_PARAMETERS]); // create new key blob with current os version etc.