[PM-10373] Fix FIDO 2 credential creation from unprivileged apps (#3658)

2024-10-31 07:05:35 +03:00 · 2024-08-05 10:28:37 -04:00 · 2024-08-05 10:28:37 -04:00 · 31bf696e7e
commit 31bf696e7e
parent f46d12c7b1
3 changed files with 175 additions and 67 deletions
--- a/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/manager/Fido2CredentialManagerImpl.kt
+++ b/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/manager/Fido2CredentialManagerImpl.kt
@ -55,18 +55,22 @@ class Fido2CredentialManagerImpl(
        selectedCipherView: CipherView,
    ): Fido2RegisterCredentialResult {
        val clientData = if (fido2CredentialRequest.callingAppInfo.isOriginPopulated()) {
-            fido2CredentialRequest.callingAppInfo.getAppSigningSignatureFingerprint()
-                ?.let { ClientData.DefaultWithCustomHash(hash = it) }
-                ?: return Fido2RegisterCredentialResult.Error
-        } else {
-            ClientData.DefaultWithExtraData(
-                androidPackageName = fido2CredentialRequest
+                fido2CredentialRequest
                    .callingAppInfo
-                    .getAppOrigin(),
-            )
-        }
-        val origin = fido2CredentialRequest.origin
-            ?: fido2CredentialRequest.callingAppInfo.getAppOrigin()
+                    .getAppSigningSignatureFingerprint()
+                    ?.let { ClientData.DefaultWithCustomHash(hash = it) }
+                    ?: return Fido2RegisterCredentialResult.Error
+            } else {
+                ClientData.DefaultWithExtraData(
+                    androidPackageName = fido2CredentialRequest
+                        .callingAppInfo
+                        .packageName,
+                )
+            }
+        val origin = fido2CredentialRequest
+            .origin
+            ?: getOriginUrlFromAttestationOptionsOrNull(fido2CredentialRequest.requestJson)
+            ?: return Fido2RegisterCredentialResult.Error

        return vaultSdkSource
            .registerFido2Credential(
@ -132,13 +136,15 @@ class Fido2CredentialManagerImpl(
        val clientData = request.clientDataHash
            ?.let { ClientData.DefaultWithCustomHash(hash = it) }
            ?: ClientData.DefaultWithExtraData(androidPackageName = callingAppInfo.getAppOrigin())
+        val origin = request.origin
+            ?: getOriginUrlFromAssertionOptionsOrNull(request.requestJson)
+            ?: return Fido2CredentialAssertionResult.Error

        return vaultSdkSource
            .authenticateFido2Credential(
                request = AuthenticateFido2CredentialRequest(
                    userId = userId,
-                    origin = callingAppInfo.origin
-                        ?: callingAppInfo.getAppOrigin(),
+                    origin = origin,
                    requestJson = """{"publicKey": ${request.requestJson}}""",
                    clientData = clientData,
                    selectedCipherView = selectedCipherView,
@ -257,6 +263,18 @@ class Fido2CredentialManagerImpl(

    override fun hasAuthenticationAttemptsRemaining(): Boolean =
        authenticationAttempts < MAX_AUTHENTICATION_ATTEMPTS
+
+    private fun getOriginUrlFromAssertionOptionsOrNull(requestJson: String) =
+        getPasskeyAssertionOptionsOrNull(requestJson)
+            ?.relyingPartyId
+            ?.let { "$HTTPS$it" }
+
+    private fun getOriginUrlFromAttestationOptionsOrNull(requestJson: String) =
+        getPasskeyAttestationOptionsOrNull(requestJson)
+            ?.relyingParty
+            ?.id
+            ?.let { "$HTTPS$it" }
 }

 private const val MAX_AUTHENTICATION_ATTEMPTS = 5
+private const val HTTPS = "https://"
--- a/app/src/test/java/com/x8bit/bitwarden/data/autofill/fido2/manager/Fido2CredentialManagerTest.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/data/autofill/fido2/manager/Fido2CredentialManagerTest.kt
@ -27,6 +27,7 @@ import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource
 import com.x8bit.bitwarden.data.vault.datasource.sdk.model.AuthenticateFido2CredentialRequest
 import com.x8bit.bitwarden.data.vault.datasource.sdk.model.RegisterFido2CredentialRequest
 import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockCipherView
+import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockPublicKeyAssertionResponse
 import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockPublicKeyAttestationResponse
 import com.x8bit.bitwarden.data.vault.datasource.sdk.util.toAndroidFido2PublicKeyCredential
 import com.x8bit.bitwarden.ui.vault.feature.addedit.util.createMockPasskeyAssertionOptions
@ -571,6 +572,39 @@ class Fido2CredentialManagerTest {
            )
        }

+    @Test
+    fun `registerFido2Credential should return Error when origin is null`() = runTest {
+        val mockAssertionRequest = createMockFido2CredentialRequest(
+            number = 1,
+            origin = null,
+            signingInfo = mockSigningInfo,
+        )
+        val mockSelectedCipher = createMockCipherView(number = 1)
+
+        every { Base64.encodeToString(any(), any()) } returns DEFAULT_APP_SIGNATURE
+        every {
+            json.decodeFromString<PasskeyAttestationOptions>(any())
+        } throws SerializationException()
+
+        val result = fido2CredentialManager.registerFido2Credential(
+            userId = "activeUserId",
+            fido2CredentialRequest = mockAssertionRequest,
+            selectedCipherView = mockSelectedCipher,
+        )
+
+        coVerify(exactly = 0) {
+            mockVaultSdkSource.registerFido2Credential(
+                request = any(),
+                fido2CredentialStore = any(),
+            )
+        }
+
+        assertEquals(
+            Fido2RegisterCredentialResult.Error,
+            result,
+        )
+    }
+
    @Suppress("MaxLineLength")
    @Test
    fun `hasAuthenticationAttemptsRemaining returns true when authenticationAttempts is less than 5`() {
@ -700,65 +734,87 @@ class Fido2CredentialManagerTest {
            )
        }

+    @Suppress("MaxLineLength")
    @Test
-    fun `authenticateFido2Credential should use apk key hash for privileged apps`() = runTest {
-        every {
-            mockSigningInfo.apkContentsSigners
-        } returns arrayOf(Signature(DEFAULT_APP_SIGNATURE))
-        every {
-            mockSigningInfo.hasMultipleSigners()
-        } returns false
-        val mockCipherView = createMockCipherView(number = 1)
-        val mockRequest = createMockFido2CredentialAssertionRequest(
-            number = 1,
-            clientDataHash = null,
-            signingInfo = mockSigningInfo,
-            origin = null,
-        )
-        val requestCaptureSlot = slot<AuthenticateFido2CredentialRequest>()
-        val mockSdkRequest = createAuthenticateFido2CredentialRequest(
-            number = 1,
-            clientData = ClientData.DefaultWithExtraData(
-                androidPackageName = "android:apk-key-hash:$DEFAULT_APP_SIGNATURE",
-            ),
-            mockRequest = mockRequest,
-            mockCipherView = mockCipherView,
-            origin = "android:apk-key-hash:$DEFAULT_APP_SIGNATURE",
-        )
-        val mockSdkResponse =
-            mockk<PublicKeyCredentialAuthenticatorAssertionResponse>(relaxed = true)
-        every { Base64.encodeToString(any(), any()) } returns DEFAULT_APP_SIGNATURE
-        coEvery {
-            mockVaultSdkSource.authenticateFido2Credential(
-                request = mockSdkRequest,
-                fido2CredentialStore = any(),
+    fun `authenticateFido2Credential should construct correct assertion request when calling app is unprivileged`() =
+        runTest {
+            val mockAssertionRequest = createMockFido2CredentialAssertionRequest(
+                number = 1,
+                origin = null,
+                clientDataHash = null,
+                signingInfo = mockSigningInfo,
            )
-        } returns mockSdkResponse.asSuccess()
-
-        fido2CredentialManager.authenticateFido2Credential(
-            userId = "activeUserId",
-            request = mockRequest,
-            selectedCipherView = createMockCipherView(number = 1),
-        )
-
-        coVerify {
-            mockVaultSdkSource.authenticateFido2Credential(
-                request = capture(requestCaptureSlot),
-                fido2CredentialStore = any(),
-            )
-        }
-        assertEquals(
-            AuthenticateFido2CredentialRequest(
-                userId = "activeUserId",
-                origin = "android:apk-key-hash:$DEFAULT_APP_SIGNATURE",
-                requestJson = """{"publicKey": ${mockRequest.requestJson}}""",
+            val mockAssertionOptions = createMockPasskeyAssertionOptions(number = 1)
+            val mockSelectedCipher = createMockCipherView(number = 1)
+            val mockSdkRequest = createAuthenticateFido2CredentialRequest(
+                number = 1,
+                origin = "https://${mockAssertionOptions.relyingPartyId}",
                clientData = ClientData.DefaultWithExtraData(
                    androidPackageName = "android:apk-key-hash:$DEFAULT_APP_SIGNATURE",
                ),
-                selectedCipherView = mockCipherView,
-                isUserVerificationSupported = true,
-            ),
-            requestCaptureSlot.captured,
+            )
+            val requestCaptureSlot = slot<AuthenticateFido2CredentialRequest>()
+
+            every { Base64.encodeToString(any(), any()) } returns DEFAULT_APP_SIGNATURE
+            coEvery {
+                mockVaultSdkSource.authenticateFido2Credential(
+                    request = mockSdkRequest,
+                    fido2CredentialStore = any(),
+                )
+            } returns createMockPublicKeyAssertionResponse(number = 1).asSuccess()
+
+            fido2CredentialManager.authenticateFido2Credential(
+                userId = "activeUserId",
+                request = mockAssertionRequest,
+                selectedCipherView = mockSelectedCipher,
+            )
+
+            coVerify {
+                mockVaultSdkSource.authenticateFido2Credential(
+                    request = capture(requestCaptureSlot),
+                    fido2CredentialStore = any(),
+                )
+            }
+
+            assertEquals(
+                "https://${mockAssertionOptions.relyingPartyId}",
+                requestCaptureSlot.captured.origin,
+            )
+        }
+
+    @Suppress("MaxLineLength")
+    @Test
+    fun `authenticateFido2Credential should return Error when origin is null`() = runTest {
+        val mockAssertionRequest = createMockFido2CredentialAssertionRequest(
+            number = 1,
+            origin = null,
+            clientDataHash = null,
+            signingInfo = mockSigningInfo,
+        )
+
+        val mockSelectedCipher = createMockCipherView(number = 1)
+
+        every { Base64.encodeToString(any(), any()) } returns DEFAULT_APP_SIGNATURE
+        every {
+            json.decodeFromString<PasskeyAssertionOptions>(any())
+        } throws SerializationException()
+
+        val result = fido2CredentialManager.authenticateFido2Credential(
+            userId = "activeUserId",
+            request = mockAssertionRequest,
+            selectedCipherView = mockSelectedCipher,
+        )
+
+        coVerify(exactly = 0) {
+            mockVaultSdkSource.authenticateFido2Credential(
+                request = any(),
+                fido2CredentialStore = any(),
+            )
+        }
+
+        assertEquals(
+            Fido2CredentialAssertionResult.Error,
+            result,
        )
    }

--- a/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/PublicKeyAuthenticatorAssertionResponseUtil.kt
+++ b/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/PublicKeyAuthenticatorAssertionResponseUtil.kt
@ -0,0 +1,34 @@
+package com.x8bit.bitwarden.data.vault.datasource.sdk.model
+
+import com.bitwarden.fido.AuthenticatorAssertionResponse
+import com.bitwarden.fido.ClientExtensionResults
+import com.bitwarden.fido.CredPropsResult
+import com.bitwarden.fido.PublicKeyCredentialAuthenticatorAssertionResponse
+import com.bitwarden.fido.SelectedCredential
+
+/**
+ * Creates a mock [PublicKeyCredentialAuthenticatorAssertionResponse] for testing.
+ */
+fun createMockPublicKeyAssertionResponse(number: Int) =
+    PublicKeyCredentialAuthenticatorAssertionResponse(
+        id = "mockId-$number",
+        rawId = "mockId-$number".toByteArray(),
+        ty = "mockTy-$number",
+        authenticatorAttachment = "mockAuthenticatorAttachment-$number",
+        clientExtensionResults = ClientExtensionResults(
+            credProps = CredPropsResult(
+                rk = true,
+                authenticatorDisplayName = "mockAuthenticatorDisplayName-$number",
+            ),
+        ),
+        response = AuthenticatorAssertionResponse(
+            clientDataJson = byteArrayOf(0),
+            authenticatorData = byteArrayOf(0),
+            signature = byteArrayOf(0),
+            userHandle = byteArrayOf(0),
+        ),
+        selectedCredential = SelectedCredential(
+            cipher = createMockCipherView(number = number),
+            credential = createMockFido2CredentialView(number = 1),
+        ),
+    )