From 4fb87b6200357fcaef6466695475eccd737ca864 Mon Sep 17 00:00:00 2001 From: michael Date: Wed, 17 Jun 2026 16:10:14 -0700 Subject: [PATCH] refactor(applet): extract StepUpSession from StepUpApplet (M2C.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the four StepUp session-state fields (StepUpSKDevice, StepUpSKReader, stepup_reader_counter, stepup_device_counter) and the IV-stamp + counter-advance math out of StepUpApplet into a dedicated StepUpSession holder. Adds a unit-testable surface for the spec §8.3.1.6/8/9 IV layout and §8.4.3 counter-advance behaviour (4 new tests pinning reader/device IV layout, carry across byte 2, and the 32-bit BE wrap at 0xFFFFFFFF). Behaviour-preserving: full StepUpAppletTest stays green with no test-assertion changes (the test-only getters keep their signatures and just delegate to session.skDevice / session.skReader). StepUpApplet shrinks 444 -> 394 LOC. Full suite: 138 tests, 0 failures, 3 errors (pre-existing GCM trio). --- .../dangerousthings/aliro/StepUpApplet.java | 106 ++++----------- .../dangerousthings/aliro/StepUpSession.java | 123 ++++++++++++++++++ .../aliro/StepUpSessionTest.java | 103 +++++++++++++++ 3 files changed, 254 insertions(+), 78 deletions(-) create mode 100644 applet/src/main/java/com/dangerousthings/aliro/StepUpSession.java create mode 100644 applet/src/test/java/com/dangerousthings/aliro/StepUpSessionTest.java diff --git a/applet/src/main/java/com/dangerousthings/aliro/StepUpApplet.java b/applet/src/main/java/com/dangerousthings/aliro/StepUpApplet.java index 69dc6e7..2bd1793 100644 --- a/applet/src/main/java/com/dangerousthings/aliro/StepUpApplet.java +++ b/applet/src/main/java/com/dangerousthings/aliro/StepUpApplet.java @@ -75,22 +75,13 @@ public class StepUpApplet extends Applet { * ENVELOPE body once the Step-Up AID is the active applet. */ private static final byte INS_ENVELOPE = (byte) 0xC3; - /** Length of each derived Step-Up session key (spec §8.4.3). */ - private static final short STEP_UP_SK_LEN = 32; - /** 12-byte AES-256-GCM IV layout (§8.3.1.8/9): 8B prefix + 4B counter. */ - private static final short GCM_IV_LEN = 12; private static final short GCM_TAG_LEN = 16; - private static final short COUNTER_LEN = 4; - /** {@code StepUpSKDevice} — UD→reader leg of the Step-Up AES-256-GCM - * session, derived from {@code StepUpSK} via HKDF (§8.4.3) when SELECT - * finds an armed {@link SessionContext}. Transient, cleared on deselect. */ - private final byte[] stepUpSKDevice; - - /** {@code StepUpSKReader} — reader→UD leg of the Step-Up GCM session. - * Same derivation context as {@link #stepUpSKDevice}. */ - private final byte[] stepUpSKReader; + /** Holds StepUpSKDevice / StepUpSKReader and the two BE32 message + * counters, plus the IV-stamping math (spec §8.3.1.6/8/9 + §8.4.3). + * All four arrays inside are CLEAR_ON_DESELECT. */ + private final StepUpSession session; /** 32-byte scratch used only during {@link #select()} to stage the * StepUpSK copied out of {@link SessionContext} before HKDF derives the @@ -98,25 +89,8 @@ public class StepUpApplet extends Applet { * outlives the call. */ private final byte[] stepUpSKScratch; - /** Session-bound {@code StepUp_reader_counter} per §8.4.3 + mdoc [6] - * §9.1.1.5: 32-bit big-endian counter starting at {@code 0x00000001} the - * first time the reader→UD direction is used in this Step-Up session, - * incremented after each successful decrypt. CLEAR_ON_DESELECT so each - * Step-Up phase entry starts fresh -- the matching SELECT re-initialises - * the counter alongside the SK derivation. Shared across ENVELOPE and - * EXCHANGE: both commands are reader→device so both consume from the - * same counter. */ - private final byte[] stepUpReaderCounter; - - /** Session-bound {@code StepUp_device_counter} per §8.4.3 + mdoc [6] - * §9.1.1.5: 32-bit big-endian counter for the device→reader direction, - * starting at {@code 0x00000001} on Step-Up session entry, incremented - * after each successful encrypt. In M1 only ENVELOPE returns ciphertext - * (EXCHANGE returns empty), so this advances once per ENVELOPE. */ - private final byte[] stepUpDeviceCounter; - - /** 12-byte scratch for the GCM IV: 8 zero bytes + 4-byte reader counter - * per §8.3.1.8. Rebuilt per EXCHANGE; CLEAR_ON_DESELECT. */ + /** 12-byte scratch for the GCM IV: filled by {@link StepUpSession#readerIv} + * or {@link StepUpSession#deviceIv} before each en/decrypt. CLEAR_ON_DESELECT. */ private final byte[] ivScratch; /** Persistent EXCHANGE plaintext sink for the decrypt-and-discard path. @@ -149,12 +123,9 @@ public class StepUpApplet extends Applet { // EXPEDITED or pulls the field) zeroes the session keys, matching the // "fresh keys per Step-Up phase" invariant we'll need when ENVELOPE / // EXCHANGE handlers run AES-GCM. - stepUpSKDevice = JCSystem.makeTransientByteArray(STEP_UP_SK_LEN, JCSystem.CLEAR_ON_DESELECT); - stepUpSKReader = JCSystem.makeTransientByteArray(STEP_UP_SK_LEN, JCSystem.CLEAR_ON_DESELECT); - stepUpSKScratch = JCSystem.makeTransientByteArray(STEP_UP_SK_LEN, JCSystem.CLEAR_ON_DESELECT); - stepUpReaderCounter = JCSystem.makeTransientByteArray(COUNTER_LEN, JCSystem.CLEAR_ON_DESELECT); - stepUpDeviceCounter = JCSystem.makeTransientByteArray(COUNTER_LEN, JCSystem.CLEAR_ON_DESELECT); - ivScratch = JCSystem.makeTransientByteArray(GCM_IV_LEN, JCSystem.CLEAR_ON_DESELECT); + session = new StepUpSession(); + stepUpSKScratch = JCSystem.makeTransientByteArray(StepUpSession.SK_LEN, JCSystem.CLEAR_ON_DESELECT); + ivScratch = JCSystem.makeTransientByteArray(StepUpSession.IV_LEN, JCSystem.CLEAR_ON_DESELECT); scratchPlaintext = JCSystem.makeTransientByteArray(SCRATCH_PLAINTEXT_LEN, JCSystem.CLEAR_ON_DESELECT); sessionFlags = JCSystem.makeTransientByteArray(FLAGS_LEN, JCSystem.CLEAR_ON_DESELECT); } @@ -175,20 +146,18 @@ public class StepUpApplet extends Applet { SessionContext.copyStepUpSK(stepUpSKScratch, (short) 0); CryptoSingletons.getAliroCrypto().deriveStepUpSessionKeys( stepUpSKScratch, (short) 0, - stepUpSKDevice, (short) 0, - stepUpSKReader, (short) 0); + session.skDevice, (short) 0, + session.skReader, (short) 0); // Wipe the staged StepUpSK — the derived keys are sufficient // from here on and we don't want the IKM lingering in transient. - Util.arrayFillNonAtomic(stepUpSKScratch, (short) 0, STEP_UP_SK_LEN, (byte) 0); + Util.arrayFillNonAtomic(stepUpSKScratch, (short) 0, StepUpSession.SK_LEN, (byte) 0); // Spec §8.4.3 -> mdoc [6] §9.1.1.5: session-bound counters - // initialized to 0x00000001 on session entry, one per direction. + // initialised to 0x00000001 on session entry, one per direction. // CLEAR_ON_DESELECT already zeroes them on each fresh select; - // rewrite explicitly so a Step-Up SELECT mid-session (without a - // deselect in between) also starts both counters at 1. - Util.arrayFillNonAtomic(stepUpReaderCounter, (short) 0, COUNTER_LEN, (byte) 0); - stepUpReaderCounter[3] = (byte) 0x01; - Util.arrayFillNonAtomic(stepUpDeviceCounter, (short) 0, COUNTER_LEN, (byte) 0); - stepUpDeviceCounter[3] = (byte) 0x01; + // session.reset() rewrites explicitly so a Step-Up SELECT + // mid-session (without a deselect in between) also starts both + // counters at 1. + session.reset(); sessionFlags[FLAG_KEYS_READY] = (byte) 1; } else { sessionFlags[FLAG_KEYS_READY] = (byte) 0; @@ -263,16 +232,14 @@ public class StepUpApplet extends Applet { // Build the IV: 8 zero bytes (reader→device prefix per §8.3.1.8) + // stepup_reader_counter, big-endian, in the trailing 4 bytes. - Util.arrayFillNonAtomic(ivScratch, (short) 0, (short) 8, (byte) 0); - Util.arrayCopyNonAtomic(stepUpReaderCounter, (short) 0, - ivScratch, (short) 8, COUNTER_LEN); + session.readerIv(ivScratch, (short) 0); // Decrypt-and-discard. AliroGcm.decrypt throws CryptoException on // tag mismatch; remap to a security SW so an attacker can't tell // tag-mismatch from any other failure mode. try { CryptoSingletons.getAliroGcm().decrypt( - stepUpSKReader, (short) 0, + session.skReader, (short) 0, ivScratch, (short) 0, buf, dataOff, lc, scratchPlaintext, (short) 0); @@ -288,7 +255,7 @@ public class StepUpApplet extends Applet { Util.arrayFillNonAtomic(scratchPlaintext, (short) 0, ptLen, (byte) 0); // Spec §8.3.1.8: reader_counter <- reader_counter + 1 after use. - incrementCounter(stepUpReaderCounter, (short) 0); + session.advanceReaderCounter(); // Ack with SW=9000 and empty payload. If field testing on real // X-CUBE-ALIRO firmware shows the reader rejects an empty payload, @@ -340,13 +307,11 @@ public class StepUpApplet extends Applet { } // Build the reader-side IV: 8 zero bytes (§8.3.1.9) + reader counter. - Util.arrayFillNonAtomic(ivScratch, (short) 0, (short) 8, (byte) 0); - Util.arrayCopyNonAtomic(stepUpReaderCounter, (short) 0, - ivScratch, (short) 8, COUNTER_LEN); + session.readerIv(ivScratch, (short) 0); try { CryptoSingletons.getAliroGcm().decrypt( - stepUpSKReader, (short) 0, + session.skReader, (short) 0, ivScratch, (short) 0, buf, dataOff, lc, scratchPlaintext, (short) 0); @@ -356,7 +321,7 @@ public class StepUpApplet extends Applet { ISOException.throwIt(ISO7816.SW_SECURITY_STATUS_NOT_SATISFIED); } // Spec §8.3.1.9: reader_counter <- reader_counter + 1 after use. - incrementCounter(stepUpReaderCounter, (short) 0); + session.advanceReaderCounter(); // Wipe the decrypted DeviceRequest -- M1 has no use for it. M2 will // replace this with real mdoc parsing and a real DeviceResponse. @@ -368,22 +333,19 @@ public class StepUpApplet extends Applet { // Build the device-side IV: 0x00*7 || 0x01 || device_counter // (§8.3.1.6 -- 8-byte prefix ending in 0x01, then 4B BE counter). - Util.arrayFillNonAtomic(ivScratch, (short) 0, (short) 7, (byte) 0); - ivScratch[7] = (byte) 0x01; - Util.arrayCopyNonAtomic(stepUpDeviceCounter, (short) 0, - ivScratch, (short) 8, COUNTER_LEN); + session.deviceIv(ivScratch, (short) 0); // Encrypt into the APDU buffer at offset 0. Safe to overwrite the // inbound command bytes here because we've finished reading them. // Output length = 1 (ct) + 16 (tag) = 17 bytes; well under the 252-byte // APDU buffer ceiling. short ctLen = CryptoSingletons.getAliroGcm().encrypt( - stepUpSKDevice, (short) 0, + session.skDevice, (short) 0, ivScratch, (short) 0, scratchPlaintext, (short) 0, (short) 1, buf, (short) 0); // Spec §8.3.1.6: device_counter <- device_counter + 1 after use. - incrementCounter(stepUpDeviceCounter, (short) 0); + session.advanceDeviceCounter(); // Wipe the single plaintext byte (CLEAR_ON_DESELECT alone would // leave 0xA0 sitting in transient until reader walks away). @@ -392,18 +354,6 @@ public class StepUpApplet extends Applet { apdu.setOutgoingAndSend((short) 0, ctLen); } - /** 32-bit big-endian counter increment with carry across all 4 bytes. - * Wraps mod 2^32; spec §8.3.3.5.4 says the counter SHALL never reach - * 0xFFFF before increment (note: spec uses 0xFFFF where 0xFFFFFFFF is - * clearly meant -- 4-byte BE counter), so wrap is unreachable in - * practice during normal protocol flow. */ - private static void incrementCounter(byte[] buf, short off) { - for (short i = (short) (off + 3); i >= off; i--) { - buf[i]++; - if (buf[i] != 0) return; - } - } - /** * Minimal FCI for step-up SELECT. The spec (§10.2.1.2) allows the UD to * advertise supported APDU command/response sizes here; those get added @@ -435,10 +385,10 @@ public class StepUpApplet extends Applet { // NOT for use outside the applet's own test module. void copyStepUpSKDeviceForTesting(byte[] out, short outOff) { - Util.arrayCopyNonAtomic(stepUpSKDevice, (short) 0, out, outOff, STEP_UP_SK_LEN); + Util.arrayCopyNonAtomic(session.skDevice, (short) 0, out, outOff, StepUpSession.SK_LEN); } void copyStepUpSKReaderForTesting(byte[] out, short outOff) { - Util.arrayCopyNonAtomic(stepUpSKReader, (short) 0, out, outOff, STEP_UP_SK_LEN); + Util.arrayCopyNonAtomic(session.skReader, (short) 0, out, outOff, StepUpSession.SK_LEN); } } diff --git a/applet/src/main/java/com/dangerousthings/aliro/StepUpSession.java b/applet/src/main/java/com/dangerousthings/aliro/StepUpSession.java new file mode 100644 index 0000000..c807ad6 --- /dev/null +++ b/applet/src/main/java/com/dangerousthings/aliro/StepUpSession.java @@ -0,0 +1,123 @@ +package com.dangerousthings.aliro; + +import javacard.framework.JCSystem; +import javacard.framework.Util; + +/** + * Holder for all Step-Up AES-256-GCM session state (spec §8.4.3 + mdoc [6] + * §9.1.1.5): the two derived session keys ({@code StepUpSKDevice}, + * {@code StepUpSKReader}) and the two big-endian 32-bit message counters + * ({@code stepup_reader_counter}, {@code stepup_device_counter}). + * + *

Extracted out of {@link StepUpApplet} so the IV-construction + + * counter-advance math live in one place and can be unit-tested without + * spinning up the whole applet. {@link StepUpApplet} owns exactly one + * instance; there is no singleton. + * + *

Lifecycle: + *

+ */ +final class StepUpSession { + + /** Length of each derived Step-Up session key (spec §8.4.3). */ + static final short SK_LEN = 32; + + /** 12-byte AES-256-GCM IV total length. */ + static final short IV_LEN = 12; + + /** Trailing 4-byte big-endian counter portion of the IV. */ + static final short COUNTER_LEN = 4; + + /** {@code StepUpSKDevice} — device->reader leg of the GCM session, + * derived from {@code StepUpSK} via HKDF (§8.4.3) by + * {@link StepUpApplet#select()}. Transient, CLEAR_ON_DESELECT. */ + final byte[] skDevice; + + /** {@code StepUpSKReader} — reader->device leg of the GCM session. + * Same derivation context as {@link #skDevice}. */ + final byte[] skReader; + + /** {@code stepup_reader_counter} per §8.4.3 + mdoc [6] §9.1.1.5: + * 32-bit big-endian counter, initialised to {@code 0x00000001} on + * session entry, incremented after each successful reader-side decrypt. + * Shared across ENVELOPE and EXCHANGE (both reader->device). */ + final byte[] readerCounter; + + /** {@code stepup_device_counter} per §8.4.3 + mdoc [6] §9.1.1.5: + * 32-bit big-endian counter for the device->reader direction, init + * to {@code 0x00000001}, incremented after each successful encrypt. */ + final byte[] deviceCounter; + + StepUpSession() { + skDevice = JCSystem.makeTransientByteArray(SK_LEN, JCSystem.CLEAR_ON_DESELECT); + skReader = JCSystem.makeTransientByteArray(SK_LEN, JCSystem.CLEAR_ON_DESELECT); + readerCounter = JCSystem.makeTransientByteArray(COUNTER_LEN, JCSystem.CLEAR_ON_DESELECT); + deviceCounter = JCSystem.makeTransientByteArray(COUNTER_LEN, JCSystem.CLEAR_ON_DESELECT); + } + + /** + * Re-initialise both counters to {@code 0x00000001}. Called from + * {@link StepUpApplet#select()} whenever the SELECT lands armed. + * + *

Keys are NOT zeroed: CLEAR_ON_DESELECT handles that on deselect, + * and the surrounding SELECT immediately re-derives them from a fresh + * StepUpSK anyway. Matches StepUpApplet M1 behaviour. + */ + void reset() { + Util.arrayFillNonAtomic(readerCounter, (short) 0, COUNTER_LEN, (byte) 0); + readerCounter[3] = (byte) 0x01; + Util.arrayFillNonAtomic(deviceCounter, (short) 0, COUNTER_LEN, (byte) 0); + deviceCounter[3] = (byte) 0x01; + } + + /** + * Write the 12-byte reader-side IV at {@code out[outOff..outOff+12]}: + * {@code 0x00 * 8 || stepup_reader_counter (4B BE)} per spec §8.3.1.8/9. + */ + void readerIv(byte[] out, short outOff) { + Util.arrayFillNonAtomic(out, outOff, (short) 8, (byte) 0); + Util.arrayCopyNonAtomic(readerCounter, (short) 0, + out, (short) (outOff + 8), COUNTER_LEN); + } + + /** + * Write the 12-byte device-side IV at {@code out[outOff..outOff+12]}: + * {@code 0x00 * 7 || 0x01 || stepup_device_counter (4B BE)} per spec §8.3.1.6. + */ + void deviceIv(byte[] out, short outOff) { + Util.arrayFillNonAtomic(out, outOff, (short) 7, (byte) 0); + out[(short) (outOff + 7)] = (byte) 0x01; + Util.arrayCopyNonAtomic(deviceCounter, (short) 0, + out, (short) (outOff + 8), COUNTER_LEN); + } + + /** stepup_reader_counter += 1 (32-bit BE, wraps mod 2^32). */ + void advanceReaderCounter() { + incrementCounter(readerCounter, (short) 0); + } + + /** stepup_device_counter += 1 (32-bit BE, wraps mod 2^32). */ + void advanceDeviceCounter() { + incrementCounter(deviceCounter, (short) 0); + } + + /** 32-bit big-endian counter increment with carry across all 4 bytes. + * Wraps mod 2^32; spec §8.3.3.5.4 says the counter SHALL never reach + * the limit, so wrap is unreachable in normal protocol flow. */ + private static void incrementCounter(byte[] buf, short off) { + for (short i = (short) (off + 3); i >= off; i--) { + buf[i]++; + if (buf[i] != 0) return; + } + } +} diff --git a/applet/src/test/java/com/dangerousthings/aliro/StepUpSessionTest.java b/applet/src/test/java/com/dangerousthings/aliro/StepUpSessionTest.java new file mode 100644 index 0000000..b102cae --- /dev/null +++ b/applet/src/test/java/com/dangerousthings/aliro/StepUpSessionTest.java @@ -0,0 +1,103 @@ +package com.dangerousthings.aliro; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; + +/** + * Tests for {@link StepUpSession} — the session-state holder extracted out + * of {@link StepUpApplet} for the Step-Up AES-256-GCM session (spec §8.4.3 + * + §8.3.1.6/8/9). Covers IV layout, counter advance, and 32-bit wrap. + * + *

Note: jcardsim is initialised lazily by {@link StepUpSession}'s + * {@code makeTransientByteArray} calls. The {@link CardSimulator} static + * initialiser does that wiring; instantiating one here is sufficient. + */ +class StepUpSessionTest { + + private static StepUpSession freshSession() { + // CardSimulator's static init installs the jcardsim runtime that + // StepUpSession needs for makeTransientByteArray. Construct one and + // discard — only the static-init side-effect matters. + new com.licel.jcardsim.smartcardio.CardSimulator(); + StepUpSession s = new StepUpSession(); + s.reset(); + return s; + } + + /** + * Spec §8.3.1.8/9 reader-side IV layout: 8-byte zero prefix + + * 4-byte big-endian stepup_reader_counter. After {@link StepUpSession#reset()} + * the counter is {@code 0x00000001} per §8.4.3 + mdoc [6] §9.1.1.5. + */ + @Test + void readerIv_counter1_returnsAllZerosThen0001() { + StepUpSession s = freshSession(); + byte[] iv = new byte[12]; + s.readerIv(iv, (short) 0); + assertArrayEquals( + new byte[] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1 }, + iv, + "fresh-session reader IV = 00*8 || 00 00 00 01"); + } + + /** + * Advance reader counter 255 times from 1 -> 256 (= 0x00000100). Pins + * carry propagation from the low byte into byte 2 of the counter — the + * IV trailing 4 bytes must read {@code 00 00 01 00}. + */ + @Test + void readerIv_counter256_handlesCarryIntoByte2() { + StepUpSession s = freshSession(); + // 1 -> 2 -> ... -> 256: 255 advances. + for (int i = 0; i < 255; i++) { + s.advanceReaderCounter(); + } + byte[] iv = new byte[12]; + s.readerIv(iv, (short) 0); + assertArrayEquals( + new byte[] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0 }, + iv, + "after 255 advances reader IV trails 00 00 01 00 (BE 256)"); + } + + /** + * Spec §8.3.1.6 device-side IV layout: 7-byte zero prefix + 0x01 + + * 4-byte big-endian stepup_device_counter. After reset the device + * counter is {@code 0x00000001} per §8.4.3. + */ + @Test + void deviceIv_counter1_returnsZeros_then01_then0001() { + StepUpSession s = freshSession(); + byte[] iv = new byte[12]; + s.deviceIv(iv, (short) 0); + assertArrayEquals( + new byte[] { 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 1 }, + iv, + "fresh-session device IV = 00*7 || 01 || 00 00 00 01"); + } + + /** + * Spec §8.3.3.5.4 says the counter SHALL never reach the limit, but the + * applet has no explicit overflow guard (YAGNI: the protocol's wire-speed + * ceiling makes this unreachable in practice). Lock the 32-bit BE wrap + * behaviour here so a future "let's add a check" doesn't silently change + * the math layer. + */ + @Test + void incrementCounterWrapsAt0xFFFFFFFF() { + StepUpSession s = freshSession(); + // Force the reader counter to 0xFFFFFFFF, then advance. + s.readerCounter[0] = (byte) 0xFF; + s.readerCounter[1] = (byte) 0xFF; + s.readerCounter[2] = (byte) 0xFF; + s.readerCounter[3] = (byte) 0xFF; + s.advanceReaderCounter(); + byte[] iv = new byte[12]; + s.readerIv(iv, (short) 0); + assertArrayEquals( + new byte[] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, + iv, + "0xFFFFFFFF + 1 wraps to 0x00000000 (mod 2^32)"); + } +}