diff --git a/changelog.txt b/changelog.txt index ddfbd02e14..da73fdcca5 100644 --- a/changelog.txt +++ b/changelog.txt @@ -1,5 +1,6 @@ vNext ---------- +- [PATCH] Handle app_link Intent redirection by validating broker install links and rejecting unsupported redirect URIs with appropriate error responses (#3102) - [PATCH] Extend filter-then-clone optimization to load() and getIdTokensForAccountRecord() in MsalOAuth2TokenCache: when ENABLE_FILTER_THEN_CLONE_IN_MEMORY_CACHE flight is enabled, skip clone-all preload and call direct flight-gated overloads that clone only matching credentials; add new getCredentialsFilteredBy overload with kid support (#3100) - [PATCH] Move Multiple Listening apps check to the authorization layer (#3070) - [PATCH] Edge TB: Fix lookup mode (#3108) diff --git a/common4j/src/main/com/microsoft/identity/common/java/providers/BrokerInstallLinkValidator.kt b/common4j/src/main/com/microsoft/identity/common/java/providers/BrokerInstallLinkValidator.kt new file mode 100644 index 0000000000..0589c6a945 --- /dev/null +++ b/common4j/src/main/com/microsoft/identity/common/java/providers/BrokerInstallLinkValidator.kt @@ -0,0 +1,140 @@ +// Copyright (c) Microsoft Corporation. +// All rights reserved. +// +// This code is licensed under the MIT License. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files(the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions : +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +package com.microsoft.identity.common.java.providers + +import com.microsoft.identity.common.java.util.CommonURIBuilder +import org.apache.hc.core5.http.NameValuePair +import java.net.URISyntaxException + +/** + * Strict allowlist validator for the `app_link` query-parameter value carried on + * a broker-installation redirect URI (`msauth://...?app_link=`). + * + * The classifier in [RawAuthorizationResult.getResultCodeFromFinalRedirectUri] uses this to + * decide whether to return [RawAuthorizationResult.ResultCode.BROKER_INSTALLATION_TRIGGERED] + * (which downstream Android sinks turn into `startActivity(ACTION_VIEW, app_link)`). + * + * The Microsoft identity service (eSTS) only ever emits one of three concrete + * `app_link` values today: + * 1. `https://play.google.com/store/apps/details?id=com.azure.authenticator` + * 2. `https://play.google.com/store/apps/details?id=com.microsoft.windowsintune.companyportal` + * 3. `https://go.microsoft.com/fwlink/?linkid=2134649` (China Company Portal) + * Each may carry an optional `referrer` parameter set by the server. + */ +object BrokerInstallLinkValidator { + + private const val SCHEME_HTTPS = "https" + private const val HOST_PLAY = "play.google.com" + private const val HOST_FWLINK = "go.microsoft.com" + private const val PATH_PLAY = "/store/apps/details" + private const val PATH_FWLINK = "/fwlink" + private const val PATH_FWLINK_TRAILING = "/fwlink/" + + private const val PARAM_ID = "id" + private const val PARAM_LINKID = "linkid" + private const val PARAM_REFERRER = "referrer" + + private val ALLOWED_PACKAGE_IDS = setOf( + "com.azure.authenticator", + "com.microsoft.windowsintune.companyportal" + ) + + private val ALLOWED_FWLINK_IDS = setOf("2134649") + + /** + * @return `true` iff [url] decodes to one of the allowlisted broker-install + * destinations defined above; `false` for any other input (including null, + * blank, malformed, or attacker-controlled values). + */ + @JvmStatic + fun isSafeBrokerInstallLink(url: String?): Boolean { + if (url.isNullOrBlank()) return false + + val builder: CommonURIBuilder = try { + CommonURIBuilder(url) + } catch (e: URISyntaxException) { + return false + } + + // Scheme must be exactly https (case-insensitive). + if (!SCHEME_HTTPS.equals(builder.scheme, ignoreCase = true)) return false + + // Reject embedded credentials, fragments, and non-default ports. + if (builder.userInfo != null) return false + if (builder.fragment != null) return false + if (builder.port != -1) return false + + val host = builder.host ?: return false + val path = builder.path ?: return false + + // Use getQueryParams() from CommonURIBuilder to parse query parameters. + // toUniqueParamMap returns null if any key appears more than once, defending + // against parameter-smuggling attacks such as ?id=safe&id=evil. + val params = toUniqueParamMap(builder.queryParams) ?: return false + + return when { + HOST_PLAY.equals(host, ignoreCase = true) -> isValidPlayLink(path, params) + HOST_FWLINK.equals(host, ignoreCase = true) -> isValidFwlink(path, params) + else -> false + } + } + + private fun isValidPlayLink(path: String, params: Map): Boolean { + if (path != PATH_PLAY) return false + val id = params[PARAM_ID] ?: return false + if (!ALLOWED_PACKAGE_IDS.any { it.equals(id, ignoreCase = true) }) return false + return hasOnlyAllowedExtras(params, PARAM_ID) + } + + private fun isValidFwlink(path: String, params: Map): Boolean { + if (path != PATH_FWLINK && path != PATH_FWLINK_TRAILING) return false + val linkId = params[PARAM_LINKID] ?: return false + if (linkId !in ALLOWED_FWLINK_IDS) return false + return hasOnlyAllowedExtras(params, PARAM_LINKID) + } + + private fun hasOnlyAllowedExtras(params: Map, requiredKey: String): Boolean { + for (key in params.keys) { + if (key == requiredKey) continue + if (key == PARAM_REFERRER) continue + return false + } + return true + } + + /** + * Converts a [NameValuePair] list (from [CommonURIBuilder.getQueryParams]) into a map. + * + * Returns `null` if any key appears more than once — this defends against + * parameter-smuggling attacks such as `?id=safe&id=evil` where a permissive + * parser might pick the wrong value. + */ + private fun toUniqueParamMap(pairs: List): Map? { + val out = LinkedHashMap() + for (pair in pairs) { + if (out.containsKey(pair.name)) return null + out[pair.name] = pair.value ?: "" + } + return out + } +} diff --git a/common4j/src/main/com/microsoft/identity/common/java/providers/RawAuthorizationResult.java b/common4j/src/main/com/microsoft/identity/common/java/providers/RawAuthorizationResult.java index 47f30c6402..f117bfefd8 100644 --- a/common4j/src/main/com/microsoft/identity/common/java/providers/RawAuthorizationResult.java +++ b/common4j/src/main/com/microsoft/identity/common/java/providers/RawAuthorizationResult.java @@ -193,6 +193,8 @@ public static RawAuthorizationResult fromRedirectUri(@NonNull final String redir .resultCode(getResultCodeFromFinalRedirectUri(uri)) .authorizationFinalUri(uri) .build(); + } catch (final ClientException e) { + return fromException(e); } catch (final URISyntaxException e) { return fromException(new ClientException(ClientException.MALFORMED_URL, "Failed to parse redirect URL", e)); @@ -217,7 +219,7 @@ public static RawAuthorizationResult fromPropertyBag(@NonNull final PropertyBag .build(); } - private static ResultCode getResultCodeFromFinalRedirectUri(@NonNull final URI uri) throws URISyntaxException { + private static ResultCode getResultCodeFromFinalRedirectUri(@NonNull final URI uri) throws URISyntaxException, ClientException { final String methodTag = TAG + "getResultCodeFromFinalRedirectUri"; final Map parameters = UrlUtil.getParameters(uri); @@ -225,6 +227,14 @@ private static ResultCode getResultCodeFromFinalRedirectUri(@NonNull final URI u // i.e. (Browser) msauth://com.msft.identity.client.sample.local/1wIqXSqBj7w%2Bh11ZifsnqwgyKrY%3D?wpj=1&username=idlab1%40msidlab4.onmicrosoft.com&app_link=https%3a%2f%2fplay.google.com%2fstore%2fapps%2fdetails%3fid%3dcom.azure.authenticator // (WebView) msauth://wpj/?username=idlab1%40msidlab4.onmicrosoft.com&app_link=https%3a%2f%2fplay.google.com%2fstore%2fapps%2fdetails%3fid%3dcom.azure.authenticator%26referrer%3dcom.msft.identity.client.sample.local if (parameters.containsKey(APP_LINK_KEY)) { + // Only the eSTS-emitted Play Store and China fwlink targets are accepted; + // anything else is rejected as an unsupported redirect (UNSUPPORTED_URL). + final String appLink = parameters.get(APP_LINK_KEY); + if (!BrokerInstallLinkValidator.isSafeBrokerInstallLink(appLink)) { + Logger.warn(methodTag, "Rejected app_link that is not on the broker-install allowlist."); + throw new ClientException(ClientException.UNSUPPORTED_URL, + "app_link rejected by allowlist"); + } Logger.info(methodTag, "Return to caller with BROWSER_CODE_WAIT_FOR_BROKER_INSTALL, and waiting for result."); return ResultCode.BROKER_INSTALLATION_TRIGGERED; } diff --git a/common4j/src/test/com/microsoft/identity/common/java/providers/BrokerInstallLinkValidatorTest.kt b/common4j/src/test/com/microsoft/identity/common/java/providers/BrokerInstallLinkValidatorTest.kt new file mode 100644 index 0000000000..ecc58b47a7 --- /dev/null +++ b/common4j/src/test/com/microsoft/identity/common/java/providers/BrokerInstallLinkValidatorTest.kt @@ -0,0 +1,311 @@ +// Copyright (c) Microsoft Corporation. +// All rights reserved. +// +// This code is licensed under the MIT License. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files(the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions : +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +package com.microsoft.identity.common.java.providers + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class BrokerInstallLinkValidatorTest { + + // region Positive cases + + @Test + fun acceptsAuthenticatorPlayLink() { + assertTrue( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "https://play.google.com/store/apps/details?id=com.azure.authenticator" + ) + ) + } + + @Test + fun acceptsCompanyPortalPlayLink() { + assertTrue( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "https://play.google.com/store/apps/details?id=com.microsoft.windowsintune.companyportal" + ) + ) + } + + @Test + fun acceptsPlayLinkWithReferrer() { + assertTrue( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "https://play.google.com/store/apps/details?id=com.azure.authenticator&referrer=com.contoso.app" + ) + ) + } + + @Test + fun acceptsChinaFwlinkWithTrailingSlash() { + assertTrue( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "https://go.microsoft.com/fwlink/?linkid=2134649" + ) + ) + } + + @Test + fun acceptsChinaFwlinkWithoutTrailingSlash() { + assertTrue( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "https://go.microsoft.com/fwlink?linkid=2134649" + ) + ) + } + + @Test + fun acceptsHttpsCaseInsensitiveScheme() { + assertTrue( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "HTTPS://play.google.com/store/apps/details?id=com.azure.authenticator" + ) + ) + } + + @Test + fun acceptsMixedCaseHost() { + assertTrue( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "https://Play.Google.Com/store/apps/details?id=com.azure.authenticator" + ) + ) + } + + // endregion + + // region Negative cases - input shape + + @Test + fun rejectsNull() { + assertFalse(BrokerInstallLinkValidator.isSafeBrokerInstallLink(null)) + } + + @Test + fun rejectsEmpty() { + assertFalse(BrokerInstallLinkValidator.isSafeBrokerInstallLink("")) + } + + @Test + fun rejectsWhitespace() { + assertFalse(BrokerInstallLinkValidator.isSafeBrokerInstallLink(" ")) + } + + @Test + fun rejectsMalformedUri() { + assertFalse(BrokerInstallLinkValidator.isSafeBrokerInstallLink(":/")) + } + + // endregion + + // region Negative cases - scheme + + @Test + fun rejectsHttpScheme() { + assertFalse( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "http://play.google.com/store/apps/details?id=com.azure.authenticator" + ) + ) + } + + @Test + fun rejectsJavascriptScheme() { + assertFalse(BrokerInstallLinkValidator.isSafeBrokerInstallLink("javascript:alert(1)")) + } + + @Test + fun rejectsIntentScheme() { + assertFalse( + BrokerInstallLinkValidator.isSafeBrokerInstallLink( + "intent://x#Intent;scheme=https;end" + ) + ) + } + + @Test + fun rejectsFileScheme() { + assertFalse(BrokerInstallLinkValidator.isSafeBrokerInstallLink("file:///etc/passwd")) + } + + @Test + fun rejectsDataScheme() { + // data: with literal '<' is not even a valid URI; should be rejected without throwing. + assertFalse(BrokerInstallLinkValidator.isSafeBrokerInstallLink("data:text/html,