Skip to content

[PM-22479] Checking signingCertificateHistory for a valid asset link certificate #6177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aj-rosado wants to merge 1 commit into
base: main
Choose a base branch
from
+339 −22
Open

[PM-22479] Checking signingCertificateHistory for a valid asset link certificate #6177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 38 additions & 22 deletions app/src/main/kotlin/com/x8bit/bitwarden/data/credentials/manager/OriginManagerImpl.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import com.bitwarden.ui.platform.base.util.prefixHttpsIfNecessary
import com.x8bit.bitwarden.data.credentials.model.ValidateOriginResult
import com.x8bit.bitwarden.data.credentials.repository.PrivilegedAppRepository
import com.x8bit.bitwarden.data.platform.manager.AssetManager
import com.x8bit.bitwarden.data.platform.util.getSignatureFingerprintAsHexString
import com.x8bit.bitwarden.data.platform.util.getAllSignatureFingerprintsAsHexStrings
import com.x8bit.bitwarden.data.platform.util.validatePrivilegedApp
import timber.log.Timber

Expand Down Expand Up @@ -38,27 +38,43 @@ class OriginManagerImpl(
relyingPartyId: String,
callingAppInfo: CallingAppInfo,
): ValidateOriginResult {
return digitalAssetLinkService
.checkDigitalAssetLinksRelations(
sourceWebSite = relyingPartyId.prefixHttpsIfNecessary(),
targetPackageName = callingAppInfo.packageName,
targetCertificateFingerprint = callingAppInfo
.getSignatureFingerprintAsHexString()
.orEmpty(),
relations = listOf(DELEGATE_PERMISSION_HANDLE_ALL_URLS),
)
.fold(
onSuccess = {
if (it.linked) {
ValidateOriginResult.Success(null)
} else {
ValidateOriginResult.Error.PasskeyNotSupportedForApp
}
},
onFailure = {
ValidateOriginResult.Error.AssetLinkNotFound
},
)
val fingerprints = callingAppInfo.getAllSignatureFingerprintsAsHexStrings()

if (fingerprints.isEmpty()) {
return ValidateOriginResult.Error.PasskeyNotSupportedForApp
}

var assetLinkFound = false
Copy link
Contributor

@claude claude bot Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Question about error differentiation logic

Why track assetLinkFound separately?

The assetLinkFound flag differentiates between two scenarios:

  1. Asset link API succeeded but none matched (linked = false) → PasskeyNotSupportedForApp
  2. Asset link API failed (network/server error) for all attempts → AssetLinkNotFound

Question: Is this distinction important for the user experience? Both scenarios result in validation failure.

The logic appears correct, but consider:

  • If the asset link service returns linked = false, it means the .well-known file was found but doesn't contain a matching certificate
  • If the service throws an exception, it means the .well-known file wasn't found or is inaccessible

This distinction helps provide more accurate error messages to the user about why passkey authentication failed.


// Check each fingerprint in the signing certificate history
return fingerprints
Copy link
Contributor

@claude claude bot Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Security consideration: Certificate ordering

Verify certificate history ordering assumptions

The code iterates through signingCertificateHistory using firstNotNullOfOrNull, which returns as soon as a match is found. This assumes the certificate history is ordered in a specific way.

Questions:

  1. What is the ordering of signingInfo.signingCertificateHistory? (oldest-to-newest or newest-to-oldest?)
  2. Should we prioritize checking the current/newest certificate first for performance?

According to Android documentation, signingCertificateHistory contains the signing certificates with the first entry being the current signer. The current implementation will check certificates in order from current to historical, which is correct.

Recommendation: Add a comment clarifying this ordering assumption:

// Check each fingerprint in the signing certificate history
// Note: signingCertificateHistory is ordered from current to historical certificates
return fingerprints

Copy link
Contributor

@SaintPatrck SaintPatrck Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about this is making my Spidey senses tingle. 🤔

I think checking every cert in the chain is going to make us susceptible to APK signing replay attacks.

Since the first certificate is the latest, it may be more secure to only check it and ignore others in the chain. That would also make this solution simpler. We can remove the if (signingInfo.hasMultipleSigners()) return null statement from getAppSigningSignatureFingerprint and be done. Thoughts?

Copy link
Contributor Author

@aj-rosado aj-rosado Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what we are currently doing and some apps do not have the most recent certificate fingerprint on the well-known file, but have an older one, that is the cause of this issue.

getSigningCertificateHistory should only be used if hasMultipleSigners is false. We were already doing this hasMultipleSigners validation.

Checking all the signatures looks like the intended behaviour from android

When determining if a package is signed by a desired certificate, the returned array should be checked to determine if it is one of the entries.

🤔 might be worth to also check getApkContentsSigners when multipleSigners returns true

Copy link
Contributor

@SaintPatrck SaintPatrck Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline. There are concerns about the mismatch between WhatsApp's assetlinks.json and the app signature being detected.

Accepting a valid certificate signature from anywhere within the chain is a potential vulnerability. The suggested solution at this time is to improve our user-facing error message in this scenario, and reject the request. Additionally, we are reaching out to WhatsApp so we can better understand why key rotation was performed and why their assetlinks.json has not been updated to include the latest key signature.

.firstNotNullOfOrNull { fingerprint ->
digitalAssetLinkService
.checkDigitalAssetLinksRelations(
sourceWebSite = relyingPartyId.prefixHttpsIfNecessary(),
targetPackageName = callingAppInfo.packageName,
targetCertificateFingerprint = fingerprint,
relations = listOf(DELEGATE_PERMISSION_HANDLE_ALL_URLS),
)
.fold(
onSuccess = {
assetLinkFound = true
if (it.linked) {
ValidateOriginResult.Success(null)
} else {
null
}
},
onFailure = {
null
},
)
}
?: if (assetLinkFound) {
ValidateOriginResult.Error.PasskeyNotSupportedForApp
} else {
ValidateOriginResult.Error.AssetLinkNotFound
}
}

private suspend fun validatePrivilegedAppOrigin(
Expand Down
18 changes: 18 additions & 0 deletions app/src/main/kotlin/com/x8bit/bitwarden/data/platform/util/CallingAppInfoExtensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,24 @@ fun CallingAppInfo.getSignatureFingerprintAsHexString(): String? {
}
}

/**
* Returns a list of all signing certificate hashes formatted as hex strings from the signing
* certificate history. This includes the current signing certificate and any previous ones
* (due to key rotation).
*/
@OptIn(ExperimentalStdlibApi::class)
fun CallingAppInfo.getAllSignatureFingerprintsAsHexStrings(): List<String> {
if (signingInfo.hasMultipleSigners()) return emptyList()

val md = MessageDigest.getInstance(SHA_ALGORITHM)
return signingInfo.signingCertificateHistory.map { signature ->
val hash = md.digest(signature.toByteArray())
hash.joinToString(":") { b ->
b.toHexString(HexFormat.UpperCase)
}
}
}
Copy link
Contributor

@claude claude bot Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Code Quality Suggestion

Consider caching MessageDigest instance

Creating a new MessageDigest instance inside the loop means it's recreated for each signature in the history. Since MessageDigest instances are reusable after calling digest(), consider creating it once outside the map function:

@OptIn(ExperimentalStdlibApi::class)
fun CallingAppInfo.getAllSignatureFingerprintsAsHexStrings(): List<String> {
    if (signingInfo.hasMultipleSigners()) return emptyList()

    val md = MessageDigest.getInstance(SHA_ALGORITHM)
    return signingInfo.signingCertificateHistory.map { signature ->
        val hash = md.digest(signature.toByteArray())
        hash.joinToString(":") { b ->
            b.toHexString(HexFormat.UpperCase)
        }
    }
}

This is already implemented correctly - the md instance is created once and reused. This is more efficient than the previous single-signature approach that created the digest inline.


/**
* Returns true if this [CallingAppInfo] is present in the privileged app [allowList]. Otherwise,
* returns false.
Expand Down
240 changes: 240 additions & 0 deletions app/src/test/kotlin/com/x8bit/bitwarden/data/credentials/manager/OriginManagerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class OriginManagerTest {
every { getOrigin(any()) } returns null
every { signingInfo } returns mockk {
every { apkContentsSigners } returns arrayOf(Signature(DEFAULT_APP_SIGNATURE))
every { signingCertificateHistory } returns arrayOf(Signature(DEFAULT_APP_SIGNATURE))
every { hasMultipleSigners() } returns false
}
}
Expand Down Expand Up @@ -242,11 +243,248 @@ class OriginManagerTest {
),
)
}

@Suppress("MaxLineLength")
@Test
fun `validateOrigin should check all certificate fingerprints from signing history`() =
runTest {
val customMessageDigest = mockk<MessageDigest> {
every { digest(DEFAULT_APP_SIGNATURE.toByteArray()) } returns DEFAULT_APP_SIGNATURE.toByteArray()
every { digest(SECOND_APP_SIGNATURE.toByteArray()) } returns SECOND_APP_SIGNATURE.toByteArray()
}
every { MessageDigest.getInstance(any()) } returns customMessageDigest

val mockAppInfoWithHistory = mockk<CallingAppInfo> {
every { isOriginPopulated() } returns false
every { packageName } returns DEFAULT_PACKAGE_NAME
every { signingInfo } returns mockk {
every { hasMultipleSigners() } returns false
every { signingCertificateHistory } returns arrayOf(
mockk { every { toByteArray() } returns DEFAULT_APP_SIGNATURE.toByteArray() },
mockk { every { toByteArray() } returns SECOND_APP_SIGNATURE.toByteArray() },
)
}
}

// First call with old signature fails
coEvery {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
} returns DEFAULT_ASSET_LINKS_CHECK_RESPONSE.copy(linked = false).asSuccess()

// Second call with new signature succeeds
coEvery {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = SECOND_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
} returns DEFAULT_ASSET_LINKS_CHECK_RESPONSE.asSuccess()

val result = originManager.validateOrigin(
relyingPartyId = DEFAULT_RELYING_PARTY_ID,
callingAppInfo = mockAppInfoWithHistory,
)

// Verify both fingerprints were checked
coVerify(exactly = 1) {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
}
coVerify(exactly = 1) {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = SECOND_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
}

assertEquals(
ValidateOriginResult.Success(null),
result,
)
}
Copy link
Contributor

@claude claude bot Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Excellent test coverage

The test suite comprehensively covers the new functionality:

  • ✅ Multiple certificates in history with fallback validation (line 249)
  • ✅ Early return on first match (line 320)
  • ✅ All certificates fail validation (line 381)
  • ✅ Multiple signers edge case (line 451)

This demonstrates thorough thinking about edge cases and validation scenarios.


@Suppress("MaxLineLength")
@Test
fun `validateOrigin should return Success on first matching fingerprint from signing history`() =
runTest {
val customMessageDigest = mockk<MessageDigest> {
every { digest(DEFAULT_APP_SIGNATURE.toByteArray()) } returns DEFAULT_APP_SIGNATURE.toByteArray()
every { digest(SECOND_APP_SIGNATURE.toByteArray()) } returns SECOND_APP_SIGNATURE.toByteArray()
}
every { MessageDigest.getInstance(any()) } returns customMessageDigest

val mockAppInfoWithHistory = mockk<CallingAppInfo> {
every { isOriginPopulated() } returns false
every { packageName } returns DEFAULT_PACKAGE_NAME
every { signingInfo } returns mockk {
every { hasMultipleSigners() } returns false
every { signingCertificateHistory } returns arrayOf(
mockk { every { toByteArray() } returns DEFAULT_APP_SIGNATURE.toByteArray() },
mockk { every { toByteArray() } returns SECOND_APP_SIGNATURE.toByteArray() },
)
}
}

// First call with old signature succeeds
coEvery {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
} returns DEFAULT_ASSET_LINKS_CHECK_RESPONSE.asSuccess()

val result = originManager.validateOrigin(
relyingPartyId = DEFAULT_RELYING_PARTY_ID,
callingAppInfo = mockAppInfoWithHistory,
)

// Verify only the first fingerprint was checked (early return on success)
coVerify(exactly = 1) {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
}
coVerify(exactly = 0) {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = SECOND_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
}

assertEquals(
ValidateOriginResult.Success(null),
result,
)
}

@Suppress("MaxLineLength")
@Test
fun `validateOrigin should return AssetLinkNotFound when no fingerprints match from signing history`() =
runTest {
val customMessageDigest = mockk<MessageDigest> {
every { digest(DEFAULT_APP_SIGNATURE.toByteArray()) } returns DEFAULT_APP_SIGNATURE.toByteArray()
every { digest(SECOND_APP_SIGNATURE.toByteArray()) } returns SECOND_APP_SIGNATURE.toByteArray()
}
every { MessageDigest.getInstance(any()) } returns customMessageDigest

val mockAppInfoWithHistory = mockk<CallingAppInfo> {
every { isOriginPopulated() } returns false
every { packageName } returns DEFAULT_PACKAGE_NAME
every { signingInfo } returns mockk {
every { hasMultipleSigners() } returns false
every { signingCertificateHistory } returns arrayOf(
mockk { every { toByteArray() } returns DEFAULT_APP_SIGNATURE.toByteArray() },
mockk { every { toByteArray() } returns SECOND_APP_SIGNATURE.toByteArray() },
)
}
}

// Both calls fail
coEvery {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
} returns RuntimeException().asFailure()

coEvery {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = SECOND_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
} returns RuntimeException().asFailure()

val result = originManager.validateOrigin(
relyingPartyId = DEFAULT_RELYING_PARTY_ID,
callingAppInfo = mockAppInfoWithHistory,
)

// Verify both fingerprints were checked
coVerify(exactly = 1) {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = DEFAULT_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
}
coVerify(exactly = 1) {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
sourceWebSite = HTTPS_DEFAULT_RELYING_PARTY_ID,
targetPackageName = DEFAULT_PACKAGE_NAME,
targetCertificateFingerprint = SECOND_CERT_FINGERPRINT,
relations = DELEGATE_PERMISSION_RELATIONS,
)
}

assertEquals(
ValidateOriginResult.Error.AssetLinkNotFound,
result,
)
}

@Suppress("MaxLineLength")
@Test
fun `validateOrigin should return PasskeyNotSupportedForApp when app has multiple signers`() =
runTest {
val mockAppInfoWithMultipleSigners = mockk<CallingAppInfo> {
every { isOriginPopulated() } returns false
every { packageName } returns DEFAULT_PACKAGE_NAME
every { signingInfo } returns mockk {
every { hasMultipleSigners() } returns true
}
}

val result = originManager.validateOrigin(
relyingPartyId = DEFAULT_RELYING_PARTY_ID,
callingAppInfo = mockAppInfoWithMultipleSigners,
)

// Verify no asset link service calls were made
coVerify(exactly = 0) {
mockDigitalAssetLinkService.checkDigitalAssetLinksRelations(
any(),
any(),
any(),
any(),
)
}

assertEquals(
ValidateOriginResult.Error.PasskeyNotSupportedForApp,
result,
)
}
}

private const val DEFAULT_PACKAGE_NAME = "com.x8bit.bitwarden"
private const val DEFAULT_APP_SIGNATURE = "0987654321ABCDEF"
private const val SECOND_APP_SIGNATURE = "FEDCBA9876543210"
private const val DEFAULT_CERT_FINGERPRINT = "30:39:38:37:36:35:34:33:32:31:41:42:43:44:45:46"
private const val SECOND_CERT_FINGERPRINT = "46:45:44:43:42:41:39:38:37:36:35:34:33:32:31:30"
private const val DEFAULT_ORIGIN = "bitwarden.com"
private const val DEFAULT_RELYING_PARTY_ID = "www.bitwarden.com"
private const val GOOGLE_ALLOW_LIST_FILENAME = "fido2_privileged_google.json"
Expand Down Expand Up @@ -295,6 +533,8 @@ private const val FAIL_ALLOW_LIST = """
]
}
"""
private const val HTTPS_DEFAULT_RELYING_PARTY_ID = "https://$DEFAULT_RELYING_PARTY_ID"
private val DELEGATE_PERMISSION_RELATIONS = listOf("delegate_permission/common.handle_all_urls")
private val DEFAULT_ASSET_LINKS_CHECK_RESPONSE =
DigitalAssetLinkCheckResponseJson(
linked = true,
Expand Down
Loading
Loading