diff --git a/changelog.txt b/changelog.txt index e31bf518..db9a0ba4 100644 --- a/changelog.txt +++ b/changelog.txt @@ -5,6 +5,7 @@ Version 1.25.1 - Bump Azure Arc HIMDS api-version from 2019-11-01 to 2020-06-01 (#1045) - Extract shared ExtendedCacheKey helper to de-duplicate cache-key plumbing (#1046) - Fix refreshed tokens not staying in their client-claims cache partition (#1039) +- Fix extended cache-key hash collision by using a length-prefix encoding of the sorted components (#1047) Version 1.25.0 ============= diff --git a/msal4j-sdk/src/integrationtest/java/com/microsoft/aad/msal4j/FmiIT.java b/msal4j-sdk/src/integrationtest/java/com/microsoft/aad/msal4j/FmiIT.java index e0f87757..1274ea39 100644 --- a/msal4j-sdk/src/integrationtest/java/com/microsoft/aad/msal4j/FmiIT.java +++ b/msal4j-sdk/src/integrationtest/java/com/microsoft/aad/msal4j/FmiIT.java @@ -93,7 +93,7 @@ void flow1_FmiCredential_FromCert() throws Exception { "Cache key should use 'atext' credential type for FMI tokens, got: " + cacheKey); // Verify hash for "SomeFmiPath/FmiCredentialPath" matches expected value - String expectedHash = "zm2n0E62zwTsnNsozptLsoOoB_C7i-GfpxHYQQINJUw".toLowerCase(); + String expectedHash = "cojvFy5tZae3nJPKVceBguvVx5vvMNJ8hPHQRbOgjOI".toLowerCase(); assertTrue(cacheKey.endsWith(expectedHash), "Cache key should end with the expected fmi_path hash, got: " + cacheKey); } @@ -172,7 +172,7 @@ void flow3_FmiCredential_FromAnotherFmiCredential() throws Exception { String cacheKey = cca.tokenCache.accessTokens.keySet().iterator().next(); assertTrue(cacheKey.contains("-atext-"), "Cache key should use 'atext' credential type"); - String expectedHash = "7CX57Q63os7benQ6ER0sxgJPtNQSv7TGb5zexcidFoI".toLowerCase(); + String expectedHash = "HaI-Va57U1u3bj1ELRa_dz5BpgHfTDMYv5vUyFoPBQo".toLowerCase(); assertTrue(cacheKey.endsWith(expectedHash), "Cache key should end with expected fmi_path hash for 'SomeFmiPath/Path', got: " + cacheKey); } diff --git a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/StringHelper.java b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/StringHelper.java index 2deeb682..5b87a48c 100644 --- a/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/StringHelper.java +++ b/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/StringHelper.java @@ -83,8 +83,20 @@ static boolean isNullOrBlank(final String str) { /** * Computes an extended cache key hash from a sorted map of key-value components. - * Concatenates sorted key+value pairs, SHA-256 hashes, then Base64URL encodes without padding. - * This algorithm is cross-SDK compatible (same output for the same inputs in all MSAL SDKs). + *

+ * Each entry is serialized using a length-prefix (netstring) encoding of the form + * {@code ::}, where the lengths are the number of + * UTF-8 bytes (not UTF-16 code units) of the key/value. Entries are concatenated in the map's + * sorted-key order, then the result is SHA-256 hashed and Base64URL encoded without padding. + *

+ * The length prefixes make the serialization injective, so semantically different component + * sets can never serialize to the same string. A naive delimiter-less concatenation of + * {@code key + value} is ambiguous (for example {@code {fmi_path:"value"}} and + * {@code {fmi_pat:"hvalue"}} both yield {@code "fmi_pathvalue"}), which would collide onto the + * same cache slot and cause redundant token re-fetches. + *

+ * This scheme is byte-identical to the matching fixes in the other MSAL SDKs (Go/.NET/Python/JS), + * so the same inputs produce the same hash bytes across the SDK family. * * @param cacheKeyComponents a sorted map of component names to values * @return Base64URL-encoded SHA-256 hash, or empty string if the map is null/empty @@ -96,8 +108,10 @@ static String computeExtCacheKeyHash(SortedMap cacheKeyComponent StringBuilder sb = new StringBuilder(); for (Map.Entry entry : cacheKeyComponents.entrySet()) { - sb.append(entry.getKey()); - sb.append(entry.getValue()); + String key = entry.getKey(); + String value = entry.getValue(); + sb.append(key.getBytes(StandardCharsets.UTF_8).length).append(':').append(key); + sb.append(value.getBytes(StandardCharsets.UTF_8).length).append(':').append(value); } return createBase64EncodedSha256Hash(sb.toString()); diff --git a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/FmiTest.java b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/FmiTest.java index 6ef00adf..0f3ced6d 100644 --- a/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/FmiTest.java +++ b/msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/FmiTest.java @@ -8,6 +8,9 @@ import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; +import java.util.Set; +import java.util.SortedMap; import java.util.TreeMap; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; @@ -375,7 +378,7 @@ void fmiPath_CacheKeyFormat_MatchesCrossSDKFormat() throws Exception { // This test verifies that the internal cache key produced by Java uses the correct // format: "-{env}-atext-{clientId}-{tenantId}-{scopes}-{hash}" // Using the same fmi_path as other SDKs' integration tests: "SomeFmiPath/FmiCredentialPath" - // Expected hash (case-sensitive): zm2n0E62zwTsnNsozptLsoOoB_C7i-GfpxHYQQINJUw + // Expected hash (case-sensitive): cojvFy5tZae3nJPKVceBguvVx5vvMNJ8hPHQRbOgjOI // The full cache key is lowercased. // Java resolves login.microsoftonline.com → login.windows.net (preferred alias). DefaultHttpClient httpClientMock = mock(DefaultHttpClient.class); @@ -405,7 +408,7 @@ void fmiPath_CacheKeyFormat_MatchesCrossSDKFormat() throws Exception { String cacheKey = cca.tokenCache.accessTokens.keySet().iterator().next(); String expectedKey = "-login.windows.net-atext-3bf56293-fbb5-42bd-a407-248ba7431a8c-10c419d4-4a50-45b2-aa4e-919fb84df24f-openid profile offline_access api://azurefmitokenexchange/.default-" - + "zm2n0E62zwTsnNsozptLsoOoB_C7i-GfpxHYQQINJUw".toLowerCase(); + + "cojvFy5tZae3nJPKVceBguvVx5vvMNJ8hPHQRbOgjOI".toLowerCase(); assertEquals(expectedKey, cacheKey, "Full cache key should match expected format"); } @@ -416,7 +419,7 @@ void fmiPath_HashValueMatchesCrossSDK() { components.put("fmi_path", "SomeFmiPath/FmiCredentialPath"); String hash = StringHelper.computeExtCacheKeyHash(components); - assertEquals("zm2n0E62zwTsnNsozptLsoOoB_C7i-GfpxHYQQINJUw", hash, + assertEquals("cojvFy5tZae3nJPKVceBguvVx5vvMNJ8hPHQRbOgjOI", hash, "Hash for 'SomeFmiPath/FmiCredentialPath' should match expected value"); // Second known value @@ -424,7 +427,7 @@ void fmiPath_HashValueMatchesCrossSDK() { components2.put("fmi_path", "SomeFmiPath/Path"); String hash2 = StringHelper.computeExtCacheKeyHash(components2); - assertEquals("7CX57Q63os7benQ6ER0sxgJPtNQSv7TGb5zexcidFoI", hash2, + assertEquals("HaI-Va57U1u3bj1ELRa_dz5BpgHfTDMYv5vUyFoPBQo", hash2, "Hash for 'SomeFmiPath/Path' should match expected value"); } @@ -574,4 +577,185 @@ void fmiPath_CacheIsolation_DifferentFmiPathsNotShared() throws Exception { "Different fmi_path values should produce separate cache entries"); } + // ======================================================================== + // Extended cache-key hash: collision resistance & cross-SDK consistency + // + // computeExtCacheKeyHash serializes each sorted entry as + // "::" then SHA-256 → Base64URL (no pad). + // The length prefixes make it injective, so distinct component sets never collide. + // ======================================================================== + + // A deliberately adversarial alphabet: the encoding's own delimiter/digit characters, + // escapes, the empty string, and multibyte characters (2-byte 'é', a combining sequence, + // and a 4-byte emoji) that expose UTF-16-vs-UTF-8 length bugs. + private static final String[] ADVERSARIAL_TOKENS = { + "", "0", "1", "9", ":", "|", "\\", "a", "ab", + "\u00e9", // 'é' as a single 2-byte code point (U+00E9) + "e\u0301", // 'e' + combining acute accent (U+0301), 3 bytes, 2 UTF-16 units + "\uD83D\uDE00" // 😀 emoji, 4 bytes, 2 UTF-16 units (surrogate pair) + }; + + private static String stripPadding(String s) { + int end = s.length(); + while (end > 0 && s.charAt(end - 1) == '=') { + end--; + } + return s.substring(0, end); + } + + private static SortedMap map(String... kv) { + TreeMap m = new TreeMap<>(); + for (int i = 0; i < kv.length; i += 2) { + m.put(kv[i], kv[i + 1]); + } + return m; + } + + @Test + void extCacheKeyHash_Injectivity_NoCollisionsOverAdversarialAlphabet() { + // Build a large set of DISTINCT component maps over the adversarial alphabet. Because the + // encoding is injective, every distinct map must produce a distinct hash. Using a Set of + // TreeMaps de-duplicates inputs by value (AbstractMap.equals), so any shortfall between the + // number of distinct inputs and the number of distinct hashes is a genuine collision. + Set> inputs = new HashSet<>(); + + // Single-entry maps: every (key, value) pair. + for (String k : ADVERSARIAL_TOKENS) { + for (String v : ADVERSARIAL_TOKENS) { + inputs.add(map(k, v)); + } + } + + // Two-entry maps: distinct keys with a couple of value assignments, including values that + // themselves look like length prefixes ("1:x") to stress boundary ambiguity. + String[] values = {"", "x", "1:x", ":", "9"}; + for (int i = 0; i < ADVERSARIAL_TOKENS.length; i++) { + for (int j = i + 1; j < ADVERSARIAL_TOKENS.length; j++) { + String k1 = ADVERSARIAL_TOKENS[i]; + String k2 = ADVERSARIAL_TOKENS[j]; + if (k1.equals(k2)) { + continue; + } + for (String v : values) { + inputs.add(map(k1, v, k2, "z")); + inputs.add(map(k1, "z", k2, v)); + } + } + } + + Set hashes = new HashSet<>(); + for (SortedMap input : inputs) { + hashes.add(StringHelper.computeExtCacheKeyHash(input)); + } + + assertEquals(inputs.size(), hashes.size(), + "Every distinct component map must hash to a distinct value (no collisions)"); + } + + @Test + void extCacheKeyHash_KeyValueBoundaryAmbiguity_ProducesDistinctHashes() { + // The classic delimiter-less bug: {fmi_path:"value"} and {fmi_pat:"hvalue"} both used to + // serialize to "fmi_pathvalue". Length prefixes disambiguate them. + assertNotEquals( + StringHelper.computeExtCacheKeyHash(map("fmi_path", "value")), + StringHelper.computeExtCacheKeyHash(map("fmi_pat", "hvalue")), + "Key/value boundary-ambiguous inputs must not collide"); + + // Multi-entry boundary ambiguity: {a:"b", cd:"e"} vs {ab:"c", d:"e"}. + assertNotEquals( + StringHelper.computeExtCacheKeyHash(map("a", "b", "cd", "e")), + StringHelper.computeExtCacheKeyHash(map("ab", "c", "d", "e")), + "Multi-entry boundary-ambiguous inputs must not collide"); + } + + @Test + void extCacheKeyHash_InputOrderIndependent() { + // Same components inserted in different orders must yield the same hash (the map is sorted). + TreeMap forward = new TreeMap<>(); + forward.put("a", "1"); + forward.put("b", "2"); + forward.put("fmi_path", "p"); + + TreeMap reverse = new TreeMap<>(); + reverse.put("fmi_path", "p"); + reverse.put("b", "2"); + reverse.put("a", "1"); + + assertEquals( + StringHelper.computeExtCacheKeyHash(forward), + StringHelper.computeExtCacheKeyHash(reverse), + "Hash must depend only on the sorted components, not insertion order"); + } + + @Test + void extCacheKeyHash_UsesUtf8ByteLength_NotStringLength() { + // 'é' (U+00E9) is 1 UTF-16 unit but 2 UTF-8 bytes; 'e' + combining accent (U+0301) is + // 2 UTF-16 units and 3 UTF-8 bytes. If the encoding used String.length() (UTF-16 units) + // instead of UTF-8 byte length, these could alias. They must not collide, and neither may + // collide with plain ASCII "e". + String precomposed = StringHelper.computeExtCacheKeyHash(map("\u00e9", "\u00e9")); + String decomposed = StringHelper.computeExtCacheKeyHash(map("e\u0301", "e\u0301")); + String ascii = StringHelper.computeExtCacheKeyHash(map("e", "e")); + + assertNotEquals(precomposed, decomposed, + "Precomposed 'é' and 'e' + combining accent must hash differently (UTF-8 byte length)"); + assertNotEquals(precomposed, ascii); + assertNotEquals(decomposed, ascii); + + // A 4-byte emoji vs its concatenation with an extra char must also stay distinct. + assertNotEquals( + StringHelper.computeExtCacheKeyHash(map("k", "\uD83D\uDE00")), + StringHelper.computeExtCacheKeyHash(map("k", "\uD83D\uDE00x"))); + } + + @Test + void extCacheKeyHash_EmptyAndSingleEntryEdges() { + // Null and empty map short-circuit to "". + assertEquals("", StringHelper.computeExtCacheKeyHash(null)); + assertEquals("", StringHelper.computeExtCacheKeyHash(new TreeMap<>())); + + // Single entry and empty-value entries are all distinct and non-empty. + String single = StringHelper.computeExtCacheKeyHash(map("fmi_path", "p")); + String emptyValue = StringHelper.computeExtCacheKeyHash(map("fmi_path", "")); + String emptyKey = StringHelper.computeExtCacheKeyHash(map("", "p")); + + assertNotEquals("", single); + assertNotEquals("", emptyValue); + assertNotEquals("", emptyKey); + assertNotEquals(single, emptyValue); + assertNotEquals(single, emptyKey); + assertNotEquals(emptyValue, emptyKey); + } + + @Test + void extCacheKeyHash_IsBase64UrlWithoutPadding() { + String hash = StringHelper.computeExtCacheKeyHash(map("fmi_path", "agent-app-id")); + assertFalse(hash.contains("="), "Hash must be Base64URL without padding"); + assertFalse(hash.contains("+"), "Hash must use the URL-safe alphabet (no '+')"); + assertFalse(hash.contains("/"), "Hash must use the URL-safe alphabet (no '/')"); + } + + @Test + void extCacheKeyHash_GoldenVectors_MatchCrossSdk() { + // Byte-identical across MSAL SDKs (Go/.NET/Python/JS share the length-prefix fix). Reference + // vectors are lowercased Base64URL-no-pad; this method preserves the encoder's mixed case, + // so compare case-insensitively (padding already absent, but strip defensively). + assertGoldenVector(map("fmi_path", "agent-app-id"), + "a0ry_zl4gccsdp7gnw927x8s0mrmnodv6tyilt0u07m"); + assertGoldenVector(map("a", "b", "cd", "e"), + "cybgactkrvlzlen1aiwzwl3ay5krkyixommrobc-ri4"); + assertGoldenVector(map("fmi_path", "value"), + "n_lucewkadzv_nybtg-2wtorgf2nrns6ihlfa7vbuzg"); + assertGoldenVector(map("fmi_pat", "hvalue"), + "tjtm16m-suk2_bkniblr25lyuki40qyceco7knuyu0k"); + assertGoldenVector(map("\u00e9", "\u00e9"), + "xskzaoz4ibr3mznftyxctvg1ptuh-0fuzpty7ndbfls"); + } + + private static void assertGoldenVector(SortedMap components, String expectedLower) { + String actual = stripPadding(StringHelper.computeExtCacheKeyHash(components)); + assertTrue(expectedLower.equalsIgnoreCase(actual), + "Cross-SDK golden vector mismatch: expected " + expectedLower + " but got " + actual); + } + }