From d581b42754ab20b1ea82a066d62d3c4f1c1b70b3 Mon Sep 17 00:00:00 2001 From: Sarkoxed Date: Mon, 19 Oct 2026 15:03:25 +0300 Subject: [PATCH 1/5] Add read_der_length function --- src/noir/lib/utils/src/lib.nr | 27 +++++++++++++++++++++++ src/noir/lib/utils/src/tests.nr | 38 ++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/noir/lib/utils/src/lib.nr b/src/noir/lib/utils/src/lib.nr index 574ee715f..23a745dd6 100644 --- a/src/noir/lib/utils/src/lib.nr +++ b/src/noir/lib/utils/src/lib.nr @@ -422,6 +422,33 @@ pub fn get_document_number_id_card(mrz: MRZData) -> MRZDocumentNumber { get_array_slice::<_, ID_CARD_MRZ_DOCUMENT_NUMBER_LENGTH>(mrz, ID_CARD_MRZ_DOCUMENT_NUMBER_INDEX) } +/// Read a DER length field whose first octet is at `data[pos]`. +/// +/// Returns `(content_length, length_octets_consumed)` where the consumed count covers +/// the length octets only (not the preceding tag). Supports the short form and the 1- +/// and 2-byte long forms, which spans every field encountered in the X.509 certificates +/// and Android attestation structures parsed here (content lengths up to 65535). +pub fn read_der_length(data: [u8; N], pos: u32) -> (u32, u32) { + let first: u8 = data[pos]; + let mut content_len: u32 = 0; + let mut consumed: u32 = 1; + if (first & 0x80) == 0 { + // Short form: the length is the low 7 bits of the single octet. + content_len = (first & 0x7F) as u32; + } else { + // Long form: the low 7 bits give the number of following length octets. + let nlen: u32 = (first & 0x7F) as u32; + assert((nlen == 1) | (nlen == 2), "Unsupported DER length form"); + for i in 0..2 { + if i < nlen { + content_len = content_len * 256 + data[pos + 1 + i] as u32; + } + } + consumed = 1 + nlen; + } + (content_len, consumed) +} + /// Returns total TLV length (tag + length field + content) for any /// ASN.1 element using DER/BER **definite-length** encoding with a /// single-byte tag (tag number field < 31) diff --git a/src/noir/lib/utils/src/tests.nr b/src/noir/lib/utils/src/tests.nr index baf3469fe..1b3976f53 100644 --- a/src/noir/lib/utils/src/tests.nr +++ b/src/noir/lib/utils/src/tests.nr @@ -1,7 +1,7 @@ use crate::{ check_zero_padding, find_subarray_index, find_subarray_index_after_index_strict, find_subarray_index_strict, get_mrz_from_dg1, get_name_from_mrz, pack_be_bytes_into_fields, - pack_be_bytes_into_u128s, split_array, unsafe_get_asn1_element_length, + pack_be_bytes_into_u128s, read_der_length, split_array, unsafe_get_asn1_element_length, }; use super::constants::{ @@ -644,5 +644,41 @@ fn test_get_document_number_id_card() { assert_eq(mrz_document_number, "ZID222222".as_bytes()); } +#[test] +fn test_read_der_length_short_form() { + // Single-octet length < 128 encodes the length directly in its low 7 bits. + let data: [u8; 2] = [0x1E, 0x00]; // length 30 + let (len, consumed) = read_der_length(data, 0); + assert_eq(len, 30); + assert_eq(consumed, 1); +} + +#[test] +fn test_read_der_length_long_form_1_byte() { + // 0x81 => one following length octet; 0x80 = 128. + let data: [u8; 2] = [0x81, 0x80]; + let (len, consumed) = read_der_length(data, 0); + assert_eq(len, 128); + assert_eq(consumed, 2); +} + +#[test] +fn test_read_der_length_long_form_2_bytes() { + // 0x82 => two following length octets; 0x01 0x41 = 321. Placed at a non-zero + // position to mirror real usage (reading the length after a tag octet). + let data: [u8; 5] = [0xAA, 0xBB, 0x82, 0x01, 0x41]; + let (len, consumed) = read_der_length(data, 2); + assert_eq(len, 321); + assert_eq(consumed, 3); +} + +#[test(should_fail_with = "Unsupported DER length form")] +fn test_read_der_length_rejects_unsupported_form() { + // 0x83 => three following length octets, which is beyond what this reader (and the + // certificates/attestations it parses) supports. + let data: [u8; 4] = [0x83, 0x01, 0x00, 0x00]; + let _ = read_der_length(data, 0); +} + // TODO: Add a test for the extended document number // once get_document_number_from_mrz can extract the whole extended document number From a1e03dea45e014ca14aaf45b573c4ec979244608 Mon Sep 17 00:00:00 2001 From: Sarkoxed Date: Mon, 19 Oct 2026 15:21:36 +0300 Subject: [PATCH 2/5] harden the android keystore attestation parser --- src/noir/lib/facematch/src/android/mod.nr | 82 ++++++++++++---- src/noir/lib/facematch/src/android/tests.nr | 101 ++++++++++++++++++++ 2 files changed, 166 insertions(+), 17 deletions(-) create mode 100644 src/noir/lib/facematch/src/android/tests.nr diff --git a/src/noir/lib/facematch/src/android/mod.nr b/src/noir/lib/facematch/src/android/mod.nr index 8e1d4c3e4..cc07f4e0d 100644 --- a/src/noir/lib/facematch/src/android/mod.nr +++ b/src/noir/lib/facematch/src/android/mod.nr @@ -4,41 +4,80 @@ use sha256; use sha512::sha384; use sig_check_ecdsa::{verify_nist_p256, verify_nist_p384}; use utils::{ - check_zero_padding, find_subarray_index, get_array_slice, poseidon2_hash_packed, - unsafe_get_asn1_element_length, + check_zero_padding, find_subarray_index, find_subarray_index_strict, get_array_slice, + poseidon2_hash_packed, read_der_length, unsafe_get_asn1_element_length, }; pub mod constants; pub mod token; use constants::{ANDROID_KEY_ORIGIN_GENERATED, OID_ANDROID_KEY_ATTESTATION}; -//pub mod tests; +pub mod tests; + +/// Advance from a TLV's tag octet (at `pos`) to the start of its content (descend into +/// a constructed element). +fn enter_element(tbs: [u8; N], pos: u32) -> u32 { + let (_, consumed) = read_der_length(tbs, pos + 1); + pos + 1 + consumed +} + +/// Advance from a TLV's tag octet (at `pos`) past the whole element (skip it entirely). +fn skip_element(tbs: [u8; N], pos: u32) -> u32 { + let (len, consumed) = read_der_length(tbs, pos + 1); + pos + 1 + consumed + len +} pub fn get_app_id_from_credential_tbs( tbs: [u8; TBS_MAX_LEN], ) -> ([u8; APP_ID_MAX_LEN], u32) { - // Find Android Key Attestation extension OID in the TBS certificate - let oid_offset = find_subarray_index(OID_ANDROID_KEY_ATTESTATION, tbs); + // Locate the Android Key Attestation extension OID. Strict first-occurrence so the + // anchor cannot be pointed at a planted copy inside attacker-controlled bytes. + let oid_offset = find_subarray_index_strict(OID_ANDROID_KEY_ATTESTATION, tbs); assert( oid_offset < TBS_MAX_LEN, "Android Key Attestation OID not found in credential certificate", ); - // Look for the 6th item in the sequence (tagged with BF 85 45) - // The 6th item contains the app ID as an OCTET STRING - let APP_ID_SEARCH_DISTANCE = 200; // Search within 200 bytes after the OID + // Walk the KeyDescription structurally so the app-id search is confined to the + // CA-attested `softwareEnforced` set. The attacker-chosen `attestationChallenge` + // (KeyDescription field 5) precedes `softwareEnforced` (field 7) in the signed + // bytes, so a flat forward scan from the OID would otherwise pick up a BF 85 45 / + // app-id blob planted inside that challenge (see the facematch app-identity finding). + // + // 04 extension value OCTET STRING + // 30 KeyDescription SEQUENCE + // 02 .. attestationVersion | + // 0A .. attestationSecurityLevel | six fixed fields skipped by length, + // 02 .. keymasterVersion | up to and including the attacker- + // 0A .. keymasterSecurityLevel | controlled attestationChallenge and + // 04 .. attestationChallenge | uniqueId + // 04 .. uniqueId | + // 30 softwareEnforced <- the app id lives in here + let mut p = oid_offset + OID_ANDROID_KEY_ATTESTATION.len(); + assert(tbs[p] == 0x04, "Attestation extension value not found"); + p = enter_element(tbs, p); // into the extension value OCTET STRING + assert(tbs[p] == 0x30, "KeyDescription SEQUENCE not found"); + p = enter_element(tbs, p); // into the KeyDescription SEQUENCE + // Skip the six fixed fields (attestationVersion .. uniqueId); this steps over the + // attacker-controlled attestationChallenge by its declared length. + for _i in 0..6 { + p = skip_element(tbs, p); + } + // Enter softwareEnforced and bound the app-id search to its contents only. + assert(tbs[p] == 0x30, "softwareEnforced SEQUENCE not found"); + let (sw_len, sw_consumed) = read_der_length(tbs, p + 1); + let sw_start = p + 1 + sw_consumed; + let sw_end = sw_start + sw_len; + assert(sw_end <= TBS_MAX_LEN, "softwareEnforced exceeds certificate bounds"); + // Search for the attestationApplicationId [709] tag (BF 85 45) within softwareEnforced. + let APP_ID_SEARCH_DISTANCE = 200; let mut attestation_app_id_sequence_offset = TBS_MAX_LEN; // Default to not found let mut found_attestation_app_id_sequence = false; - // Search for the attestation app id sequence tag pattern (BF 85 45) within the extension - // attestationApplicationId [709] EXPLICIT OCTET STRING for i in 0..APP_ID_SEARCH_DISTANCE { - if (oid_offset + OID_ANDROID_KEY_ATTESTATION.len() + i + 2 < TBS_MAX_LEN) - & !found_attestation_app_id_sequence { - if (tbs[oid_offset + OID_ANDROID_KEY_ATTESTATION.len() + i] == 0xbf) - & (tbs[oid_offset + OID_ANDROID_KEY_ATTESTATION.len() + i + 1] == 0x85) - & (tbs[oid_offset + OID_ANDROID_KEY_ATTESTATION.len() + i + 2] == 0x45) { - attestation_app_id_sequence_offset = - oid_offset + OID_ANDROID_KEY_ATTESTATION.len() + i + 4; // Skip the tag and length + let at = sw_start + i; + if (at + 2 < sw_end) & !found_attestation_app_id_sequence { + if (tbs[at] == 0xbf) & (tbs[at + 1] == 0x85) & (tbs[at + 2] == 0x45) { + attestation_app_id_sequence_offset = at + 4; // Skip the 3-byte tag and length found_attestation_app_id_sequence = true; } } @@ -90,6 +129,15 @@ pub fn get_app_id_from_credential_tbs(tbs: [u8; TBS_MAX_LEN]) -> u8 { // Find Android Key Attestation extension OID in the TBS certificate let oid_offset = find_subarray_index(OID_ANDROID_KEY_ATTESTATION, tbs); diff --git a/src/noir/lib/facematch/src/android/tests.nr b/src/noir/lib/facematch/src/android/tests.nr new file mode 100644 index 000000000..0de3460eb --- /dev/null +++ b/src/noir/lib/facematch/src/android/tests.nr @@ -0,0 +1,101 @@ +// Auto-generated regression tests for the Android FaceMatch app-identity parser. +// The real credential TBS below is captured from the `android_strict_mode_dev` +// facematch fixture; its attestationApplicationId (in softwareEnforced) is +// "app.zkpassport.zkpassport". +use super::get_app_id_from_credential_tbs; + +global REAL_CREDENTIAL_TBS: [u8; 1000] = [ + 48, 130, 2, 94, 160, 3, 2, 1, 2, 2, 1, 1, 48, 10, 6, 8, 42, 134, 72, 206, 61, 4, 3, 2, 48, 63, + 49, 18, 48, 16, 6, 3, 85, 4, 10, 19, 9, 83, 116, 114, 111, 110, 103, 66, 111, 120, 49, 41, 48, + 39, 6, 3, 85, 4, 3, 19, 32, 50, 49, 52, 51, 102, 57, 50, 48, 98, 56, 54, 101, 51, 52, 98, 100, + 100, 54, 53, 99, 99, 54, 97, 101, 100, 101, 50, 48, 55, 101, 48, 102, 48, 30, 23, 13, 55, 48, + 48, 49, 48, 49, 48, 48, 48, 48, 48, 48, 90, 23, 13, 52, 56, 48, 49, 48, 49, 48, 48, 48, 48, 48, + 48, 90, 48, 31, 49, 29, 48, 27, 6, 3, 85, 4, 3, 19, 20, 65, 110, 100, 114, 111, 105, 100, 32, + 75, 101, 121, 115, 116, 111, 114, 101, 32, 75, 101, 121, 48, 89, 48, 19, 6, 7, 42, 134, 72, 206, + 61, 2, 1, 6, 8, 42, 134, 72, 206, 61, 3, 1, 7, 3, 66, 0, 4, 76, 142, 161, 132, 148, 71, 101, + 172, 205, 118, 196, 252, 134, 68, 108, 217, 41, 187, 27, 245, 211, 127, 239, 23, 90, 158, 82, + 131, 76, 230, 52, 132, 218, 17, 82, 102, 252, 97, 145, 180, 238, 160, 223, 173, 161, 228, 153, + 42, 43, 235, 77, 179, 74, 99, 166, 229, 117, 224, 235, 44, 248, 2, 249, 83, 163, 130, 1, 105, + 48, 130, 1, 101, 48, 14, 6, 3, 85, 29, 15, 1, 1, 255, 4, 4, 3, 2, 7, 128, 48, 130, 1, 81, 6, 10, + 43, 6, 1, 4, 1, 214, 121, 2, 1, 17, 4, 130, 1, 65, 48, 130, 1, 61, 2, 2, 1, 44, 10, 1, 2, 2, 2, + 1, 44, 10, 1, 2, 4, 36, 100, 98, 57, 48, 48, 97, 49, 97, 45, 97, 55, 50, 99, 45, 52, 55, 49, 98, + 45, 56, 98, 54, 102, 45, 55, 53, 51, 49, 100, 48, 57, 100, 50, 55, 97, 102, 4, 0, 48, 91, 191, + 133, 61, 8, 2, 6, 1, 153, 227, 140, 96, 168, 191, 133, 69, 75, 4, 73, 48, 71, 49, 33, 48, 31, 4, + 25, 97, 112, 112, 46, 122, 107, 112, 97, 115, 115, 112, 111, 114, 116, 46, 122, 107, 112, 97, + 115, 115, 112, 111, 114, 116, 2, 2, 3, 135, 49, 34, 4, 32, 250, 198, 23, 69, 220, 9, 3, 120, + 111, 185, 237, 230, 42, 150, 43, 57, 159, 115, 72, 240, 187, 111, 137, 155, 131, 50, 102, 117, + 145, 3, 59, 156, 48, 129, 167, 161, 8, 49, 6, 2, 1, 2, 2, 1, 3, 162, 3, 2, 1, 3, 163, 4, 2, 2, + 1, 0, 165, 8, 49, 6, 2, 1, 4, 2, 1, 6, 170, 3, 2, 1, 1, 191, 131, 119, 2, 5, 0, 191, 133, 62, 3, + 2, 1, 0, 191, 133, 64, 76, 48, 74, 4, 32, 0, 63, 26, 222, 157, 71, 110, 97, 43, 0, 242, 152, 62, + 106, 215, 220, 209, 94, 106, 128, 204, 45, 187, 0, 141, 167, 214, 131, 158, 215, 58, 143, 1, 1, + 255, 10, 1, 0, 4, 32, 127, 211, 148, 157, 174, 28, 18, 161, 19, 217, 129, 202, 208, 201, 188, + 37, 85, 14, 231, 194, 41, 247, 184, 251, 176, 151, 50, 164, 224, 109, 74, 2, 191, 133, 65, 5, 2, + 3, 2, 34, 224, 191, 133, 66, 5, 2, 3, 3, 22, 169, 191, 133, 78, 6, 2, 4, 1, 52, 218, 9, 191, + 133, 79, 6, 2, 4, 1, 52, 218, 9, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, +]; + +// Byte offset of the attacker-controllable attestationChallenge *content* within the +// TBS above (KeyDescription field 5, which precedes softwareEnforced). +global CHALLENGE_CONTENT_OFFSET: u32 = 309; + +// A fully-formed attestationApplicationId marker (BF 85 45 ... package "spoofed"), +// 26 bytes, small enough to fit inside the 36-byte attestationChallenge. A pre-fix +// flat forward scan from the extension OID would encounter this before the genuine +// tag in softwareEnforced and return "spoofed"; the structural walk skips the +// challenge entirely and must ignore it. +global PLANTED_SPOOF_MARKER: [u8; 26] = [ + 0xBF, 0x85, 0x45, 0x16, // attestationApplicationId [709], len 22 + 0x04, 0x14, // OCTET STRING, len 20 + 0x30, 0x12, // AttestationApplicationId SEQUENCE, len 18 + 0x31, 0x0E, // packages SET, len 14 + 0x30, 0x0C, // AttestationPackageInfo SEQUENCE, len 12 + 0x04, 0x07, 0x73, 0x70, 0x6F, 0x6F, 0x66, 0x65, 0x64, // packageName OCTET "spoofed" + 0x02, 0x01, 0x01, // version INTEGER 1 + 0x31, 0x00, // signatureDigests SET, empty +]; + +fn expected_app_id() -> ([u8; 32], u32) { + let s = "app.zkpassport.zkpassport".as_bytes(); + let mut e = [0u8; 32]; + for i in 0..s.len() { + e[i] = s[i]; + } + (e, s.len()) +} + +// Happy path: the genuine app id is read from a real attestation certificate. +#[test] +fn test_get_app_id_real_cert() { + let (app_id, len): ([u8; 32], u32) = get_app_id_from_credential_tbs(REAL_CREDENTIAL_TBS); + let (expected, expected_len) = expected_app_id(); + assert(len == expected_len, "app id length mismatch"); + assert(app_id == expected, "app id mismatch"); +} + +// Regression (facematch Android app-identity finding): a spoof marker planted inside +// the attacker-controlled attestationChallenge must NOT be picked up. The structural +// walk reads the app id from softwareEnforced, so the genuine id is returned and the +// planted "spoofed" is ignored. Pre-fix, the flat scan returned "spoofed". +#[test] +fn test_get_app_id_ignores_planted_challenge() { + let mut tbs = REAL_CREDENTIAL_TBS; + for i in 0..PLANTED_SPOOF_MARKER.len() { + tbs[CHALLENGE_CONTENT_OFFSET + i] = PLANTED_SPOOF_MARKER[i]; + } + let (app_id, len): ([u8; 32], u32) = get_app_id_from_credential_tbs(tbs); + let (expected, expected_len) = expected_app_id(); + assert(len == expected_len, "planted challenge changed the app id length"); + assert(app_id == expected, "planted challenge spoofed the app id"); +} From f96f47cf2381cdc1e798e9f15569d5d64a5d1384 Mon Sep 17 00:00:00 2001 From: Sarkoxed Date: Mon, 19 Oct 2026 15:35:55 +0300 Subject: [PATCH 3/5] stop ignoring the test for android --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index f587f853c..e17c9c974 100644 --- a/.gitignore +++ b/.gitignore @@ -20,7 +20,6 @@ src/solidity/snapshots output-fixtures settings.json -src/noir/lib/facematch/src/android/tests.nr src/noir/bin/facematch/android/**/src/tests.nr src/ts/sanctions/input From 17e6fa0e0c6cdc4cf1aaa1fce66b4bf0c6af7758 Mon Sep 17 00:00:00 2001 From: Sarkoxed Date: Tue, 20 Oct 2026 16:53:47 +0300 Subject: [PATCH 4/5] refactor get_app_id_from_credential_tbs --- src/noir/lib/facematch/src/android/mod.nr | 146 +++++++++++----------- 1 file changed, 76 insertions(+), 70 deletions(-) diff --git a/src/noir/lib/facematch/src/android/mod.nr b/src/noir/lib/facematch/src/android/mod.nr index cc07f4e0d..027a6417d 100644 --- a/src/noir/lib/facematch/src/android/mod.nr +++ b/src/noir/lib/facematch/src/android/mod.nr @@ -21,11 +21,39 @@ fn enter_element(tbs: [u8; N], pos: u32) -> u32 { } /// Advance from a TLV's tag octet (at `pos`) past the whole element (skip it entirely). +/// Assumes a single-byte tag. fn skip_element(tbs: [u8; N], pos: u32) -> u32 { let (len, consumed) = read_der_length(tbs, pos + 1); pos + 1 + consumed + len } +/// Number of identifier (tag) octets at `pos`: 1 for the low-tag form, or 1 + the +/// base-128 octets of the high-tag form (low 5 bits == 0x1F) that authorization entries +/// use (e.g. attestationApplicationId [709] = BF 85 45). +fn tag_length(tbs: [u8; N], pos: u32) -> u32 { + let mut len: u32 = 1; + if (tbs[pos] & 0x1F) == 0x1F { + let mut done: bool = false; + for i in 0..4 { + if !done { + len += 1; + if (tbs[pos + 1 + i] & 0x80) == 0 { + done = true; + } + } + } + } + len +} + +/// Advance past a whole TLV whose tag may be multi-byte (a context-specific +/// authorization entry): skip the identifier octets, then the length field and content. +fn skip_tagged_element(tbs: [u8; N], pos: u32) -> u32 { + let tl: u32 = tag_length(tbs, pos); + let (len, consumed) = read_der_length(tbs, pos + tl); + pos + tl + consumed + len +} + pub fn get_app_id_from_credential_tbs( tbs: [u8; TBS_MAX_LEN], ) -> ([u8; APP_ID_MAX_LEN], u32) { @@ -37,28 +65,17 @@ pub fn get_app_id_from_credential_tbs 04 extension value OCTET STRING - // 30 KeyDescription SEQUENCE - // 02 .. attestationVersion | - // 0A .. attestationSecurityLevel | six fixed fields skipped by length, - // 02 .. keymasterVersion | up to and including the attacker- - // 0A .. keymasterSecurityLevel | controlled attestationChallenge and - // 04 .. attestationChallenge | uniqueId - // 04 .. uniqueId | - // 30 softwareEnforced <- the app id lives in here - let mut p = oid_offset + OID_ANDROID_KEY_ATTESTATION.len(); + // Walk the KeyDescription structurally to confine the app-id search to the + // CA-attested `softwareEnforced` set: the attacker-chosen `attestationChallenge` + // (field 5) precedes it, so a flat scan could match a planted BF 85 45 blob there. + // Framing: 04 extension OCTET STRING -> 30 KeyDescription -> 6 fixed fields -> 30 + // softwareEnforced. + let mut p: u32 = oid_offset + OID_ANDROID_KEY_ATTESTATION.len(); assert(tbs[p] == 0x04, "Attestation extension value not found"); p = enter_element(tbs, p); // into the extension value OCTET STRING assert(tbs[p] == 0x30, "KeyDescription SEQUENCE not found"); p = enter_element(tbs, p); // into the KeyDescription SEQUENCE - // Skip the six fixed fields (attestationVersion .. uniqueId); this steps over the - // attacker-controlled attestationChallenge by its declared length. + // Skip the six fixed fields by length, stepping over attestationChallenge. for _i in 0..6 { p = skip_element(tbs, p); } @@ -69,53 +86,45 @@ pub fn get_app_id_from_credential_tbs SEQUENCE -> SET -> + // SEQUENCE -> packageName OCTET STRING (the app id). + let (_, tag_len_bytes) = read_der_length(tbs, tag_offset + 3); + let mut q = tag_offset + 3 + tag_len_bytes; + assert(tbs[q] == 0x04, "app-id OCTET STRING wrapper not found"); + q = enter_element(tbs, q); + assert(tbs[q] == 0x30, "AttestationApplicationId SEQUENCE not found"); + q = enter_element(tbs, q); + assert(tbs[q] == 0x31, "AttestationPackageInfo SET not found"); + q = enter_element(tbs, q); + assert(tbs[q] == 0x30, "AttestationPackageInfo SEQUENCE not found"); + q = enter_element(tbs, q); + assert(tbs[q] == 0x04, "packageName OCTET STRING not found"); + let (app_id_length, app_id_len_bytes) = read_der_length(tbs, q + 1); + let app_id_offset = q + 1 + app_id_len_bytes; + assert(app_id_offset + app_id_length <= TBS_MAX_LEN, "App ID exceeds certificate bounds"); assert(app_id_length <= APP_ID_MAX_LEN, "App ID length exceeds maximum allowed length"); // Extract the app ID with dynamic length, padding with zeros if needed @@ -129,15 +138,12 @@ pub fn get_app_id_from_credential_tbs(tbs: [u8; TBS_MAX_LEN]) -> u8 { // Find Android Key Attestation extension OID in the TBS certificate let oid_offset = find_subarray_index(OID_ANDROID_KEY_ATTESTATION, tbs); From abf868ab0fc67a78852f7ad18f125349410796bd Mon Sep 17 00:00:00 2001 From: Sarkoxed Date: Tue, 20 Oct 2026 17:51:03 +0300 Subject: [PATCH 5/5] refactor get_app_id_from_credential_tbs 2 --- src/noir/lib/facematch/src/android/mod.nr | 26 +++++++++++------------ 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/noir/lib/facematch/src/android/mod.nr b/src/noir/lib/facematch/src/android/mod.nr index 027a6417d..c62a72f7b 100644 --- a/src/noir/lib/facematch/src/android/mod.nr +++ b/src/noir/lib/facematch/src/android/mod.nr @@ -54,6 +54,12 @@ fn skip_tagged_element(tbs: [u8; N], pos: u32) -> u32 { pos + tl + consumed + len } +/// Assert the single-byte-tag TLV at `pos` carries `tag`, then descend into its content. +fn expect_enter(tbs: [u8; N], pos: u32, tag: u8) -> u32 { + assert(tbs[pos] == tag, "unexpected DER tag while walking credential TBS"); + enter_element(tbs, pos) +} + pub fn get_app_id_from_credential_tbs( tbs: [u8; TBS_MAX_LEN], ) -> ([u8; APP_ID_MAX_LEN], u32) { @@ -71,10 +77,8 @@ pub fn get_app_id_from_credential_tbs 30 KeyDescription -> 6 fixed fields -> 30 // softwareEnforced. let mut p: u32 = oid_offset + OID_ANDROID_KEY_ATTESTATION.len(); - assert(tbs[p] == 0x04, "Attestation extension value not found"); - p = enter_element(tbs, p); // into the extension value OCTET STRING - assert(tbs[p] == 0x30, "KeyDescription SEQUENCE not found"); - p = enter_element(tbs, p); // into the KeyDescription SEQUENCE + p = expect_enter(tbs, p, 0x04); // extension value OCTET STRING + p = expect_enter(tbs, p, 0x30); // KeyDescription SEQUENCE // Skip the six fixed fields by length, stepping over attestationChallenge. for _i in 0..6 { p = skip_element(tbs, p); @@ -92,7 +96,7 @@ pub fn get_app_id_from_credential_tbs packageName OCTET STRING (the app id). let (_, tag_len_bytes) = read_der_length(tbs, tag_offset + 3); let mut q = tag_offset + 3 + tag_len_bytes; - assert(tbs[q] == 0x04, "app-id OCTET STRING wrapper not found"); - q = enter_element(tbs, q); - assert(tbs[q] == 0x30, "AttestationApplicationId SEQUENCE not found"); - q = enter_element(tbs, q); - assert(tbs[q] == 0x31, "AttestationPackageInfo SET not found"); - q = enter_element(tbs, q); - assert(tbs[q] == 0x30, "AttestationPackageInfo SEQUENCE not found"); - q = enter_element(tbs, q); + q = expect_enter(tbs, q, 0x04); // OCTET STRING wrapper + q = expect_enter(tbs, q, 0x30); // AttestationApplicationId SEQUENCE + q = expect_enter(tbs, q, 0x31); // SET OF AttestationPackageInfo + q = expect_enter(tbs, q, 0x30); // AttestationPackageInfo SEQUENCE assert(tbs[q] == 0x04, "packageName OCTET STRING not found"); let (app_id_length, app_id_len_bytes) = read_der_length(tbs, q + 1); let app_id_offset = q + 1 + app_id_len_bytes;