From 31bf696e7eef03369f40cec47c82cae886db2e00 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen <1883101+SaintPatrck@users.noreply.github.com> Date: Mon, 5 Aug 2024 10:28:37 -0400 Subject: [PATCH] [PM-10373] Fix FIDO 2 credential creation from unprivileged apps (#3658) --- .../manager/Fido2CredentialManagerImpl.kt | 44 +++-- .../manager/Fido2CredentialManagerTest.kt | 164 ++++++++++++------ ...icKeyAuthenticatorAssertionResponseUtil.kt | 34 ++++ 3 files changed, 175 insertions(+), 67 deletions(-) create mode 100644 app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/PublicKeyAuthenticatorAssertionResponseUtil.kt diff --git 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 index 72a715dbe..ac34e9935 100644 --- 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://" diff --git 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 index c9e20abeb..0f5ba04e9 100644 --- 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(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() - 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(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() + + 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(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, ) } diff --git 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 new file mode 100644 index 000000000..8f0cbbcdd --- /dev/null +++ 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), + ), + )