Fido: Add full nullability information

And fix the bugs found by that
microg · Sep 15, 2023 · b3032b9 · b3032b9
1 parent ae607f1
commit b3032b9
Show file tree

Hide file tree

Showing 35 changed files with 392 additions and 92 deletions.
diff --git a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/RequestHandling.kt b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/RequestHandling.kt
@@ -149,7 +149,7 @@ private suspend fun isAppIdAllowed(context: Context, appId: String, facetId: Str
 
 suspend fun RequestOptions.checkIsValid(context: Context, facetId: String, packageName: String?) {
     if (type == REGISTER) {
-        if (registerOptions.authenticatorSelection.requireResidentKey == true) {
+        if (registerOptions.authenticatorSelection?.requireResidentKey == true) {
             throw RequestHandlingException(
                 NOT_SUPPORTED_ERR,
                 "Resident credentials or empty 'allowCredentials' lists are not supported  at this time."

diff --git a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/protocol/Cbor.kt b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/protocol/Cbor.kt
@@ -4,13 +4,16 @@
  */
 package org.microg.gms.fido.core.protocol
 
+import android.util.Log
 import com.google.android.gms.fido.common.Transport
 import com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialDescriptor
 import com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialParameters
 import com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialRpEntity
 import com.google.android.gms.fido.fido2.api.common.PublicKeyCredentialUserEntity
 import com.upokecenter.cbor.CBORObject
 
+private const val TAG = "FidoCbor"
+
 fun CBORObject.AsStringSequence(): Iterable<String> = Iterable {
     object : Iterator<String> {
         var index = 0
@@ -35,21 +38,21 @@ fun Boolean.encodeAsCbor() = CBORObject.FromObject(this)
 fun PublicKeyCredentialRpEntity.encodeAsCbor() = CBORObject.NewMap().apply {
     set("id", id.encodeAsCbor())
     if (!name.isNullOrBlank()) set("name", name.encodeAsCbor())
-    if (!icon.isNullOrBlank()) set("icon", icon.encodeAsCbor())
+    if (!icon.isNullOrBlank()) set("icon", icon!!.encodeAsCbor())
 }
 
 fun PublicKeyCredentialUserEntity.encodeAsCbor() = CBORObject.NewMap().apply {
     set("id", id.encodeAsCbor())
     if (!name.isNullOrBlank()) set("name", name.encodeAsCbor())
-    if (!icon.isNullOrBlank()) set("icon", icon.encodeAsCbor())
+    if (!icon.isNullOrBlank()) set("icon", icon!!.encodeAsCbor())
     if (!displayName.isNullOrBlank()) set("displayName", displayName.encodeAsCbor())
 }
 
 fun CBORObject.decodeAsPublicKeyCredentialUserEntity() = PublicKeyCredentialUserEntity(
-    get("id")?.GetByteString(),
-    get("name")?.AsString(),
+    get("id")?.GetByteString() ?: ByteArray(0).also { Log.w(TAG, "id was not present") },
+    get("name")?.AsString() ?: "".also { Log.w(TAG, "name was not present") },
     get("icon")?.AsString(),
-    get("displayName")?.AsString()
+    get("displayName")?.AsString() ?: "".also { Log.w(TAG, "displayName was not present") }
 )
 
 fun PublicKeyCredentialParameters.encodeAsCbor() = CBORObject.NewMap().apply {
@@ -65,12 +68,12 @@ fun CBORObject.decodeAsPublicKeyCredentialParameters() = PublicKeyCredentialPara
 fun PublicKeyCredentialDescriptor.encodeAsCbor() = CBORObject.NewMap().apply {
     set("type", typeAsString.encodeAsCbor())
     set("id", id.encodeAsCbor())
-    set("transports", transports.encodeAsCbor { it.toString().encodeAsCbor() })
+    set("transports", transports.orEmpty().encodeAsCbor { it.toString().encodeAsCbor() })
 }
 
 fun CBORObject.decodeAsPublicKeyCredentialDescriptor() = PublicKeyCredentialDescriptor(
-    get("type")?.AsString(),
-    get("id")?.GetByteString(),
+    get("type")?.AsString() ?: "".also { Log.w(TAG, "type was not present") },
+    get("id")?.GetByteString() ?: ByteArray(0).also { Log.w(TAG, "id was not present") },
     get("transports")?.AsStringSequence()?.map { Transport.fromString(it) }
 )
 

diff --git a/...services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/TransportHandler.kt b/...services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/TransportHandler.kt
@@ -69,11 +69,11 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
         val extensions = mutableMapOf<String, CBORObject>()
         if (options.authenticationExtensions?.fidoAppIdExtension?.appId != null) {
             extensions["appidExclude"] =
-                options.authenticationExtensions.fidoAppIdExtension.appId.encodeAsCbor()
+                options.authenticationExtensions!!.fidoAppIdExtension!!.appId.encodeAsCbor()
         }
         if (options.authenticationExtensions?.userVerificationMethodExtension?.uvm != null) {
             extensions["uvm"] =
-                options.authenticationExtensions.userVerificationMethodExtension.uvm.encodeAsCbor()
+                options.authenticationExtensions!!.userVerificationMethodExtension!!.uvm.encodeAsCbor()
         }
         val request = AuthenticatorMakeCredentialRequest(
             clientDataHash,
@@ -99,7 +99,7 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
             options.authenticationExtensions?.fidoAppIdExtension?.appId?.toByteArray()?.digest("SHA-256")
         if (!options.registerOptions.parameters.isNullOrEmpty() && options.registerOptions.parameters.all { it.algorithmIdAsInteger != -7 })
             throw IllegalArgumentException("Can't use CTAP1 protocol for non ES256 requests")
-        if (options.registerOptions.authenticatorSelection.requireResidentKey == true)
+        if (options.registerOptions.authenticatorSelection?.requireResidentKey == true)
             throw IllegalArgumentException("Can't use CTAP1 protocol when resident key required")
         val hasCredential = options.registerOptions.excludeList.orEmpty().any { cred ->
             ctap1DeviceHasCredential(connection, clientDataHash, rpIdHash, cred) ||
@@ -163,8 +163,8 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
             connection.hasCtap2Support -> {
                 if (connection.hasCtap1Support &&
                     !connection.canMakeCredentialWithoutUserVerification && connection.hasClientPin &&
-                    options.registerOptions.authenticatorSelection.requireUserVerification != REQUIRED &&
-                    options.registerOptions.authenticatorSelection.requireResidentKey != true
+                    options.registerOptions.authenticatorSelection?.requireUserVerification != REQUIRED &&
+                    options.registerOptions.authenticatorSelection?.requireResidentKey != true
                 ) {
                     Log.d(TAG, "Using CTAP1/U2F for PIN-less registration")
                     ctap1register(connection, options, clientDataHash)
@@ -176,7 +176,7 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
             else -> throw IllegalStateException()
         }
         return AuthenticatorAttestationResponse(
-            keyHandle,
+            keyHandle ?: ByteArray(0).also { Log.w(TAG, "keyHandle was null") },
             clientData,
             AnyAttestationObject(response.authData, response.fmt, response.attStmt).encode(),
             connection.transports.toTypedArray()
@@ -194,16 +194,16 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
         )
         val extensions = mutableMapOf<String, CBORObject>()
         if (options.authenticationExtensions?.fidoAppIdExtension?.appId != null) {
-            extensions["appid"] = options.authenticationExtensions.fidoAppIdExtension.appId.encodeAsCbor()
+            extensions["appid"] = options.authenticationExtensions!!.fidoAppIdExtension!!.appId.encodeAsCbor()
         }
         if (options.authenticationExtensions?.userVerificationMethodExtension?.uvm != null) {
             extensions["uvm"] =
-                options.authenticationExtensions.userVerificationMethodExtension.uvm.encodeAsCbor()
+                options.authenticationExtensions!!.userVerificationMethodExtension!!.uvm.encodeAsCbor()
         }
         val request = AuthenticatorGetAssertionRequest(
             options.rpId,
             clientDataHash,
-            options.signOptions.allowList,
+            options.signOptions.allowList.orEmpty(),
             extensions,
             reqOptions
         )
@@ -217,9 +217,9 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
         clientDataHash: ByteArray,
         rpIdHash: ByteArray
     ): Pair<AuthenticatorGetAssertionResponse, ByteArray> {
-        val cred = options.signOptions.allowList.firstOrNull { cred ->
+        val cred = options.signOptions.allowList.orEmpty().firstOrNull { cred ->
             ctap1DeviceHasCredential(connection, clientDataHash, rpIdHash, cred)
-        } ?: options.signOptions.allowList.first()
+        } ?: options.signOptions.allowList!!.first()
 
         while (true) {
             try {
@@ -253,7 +253,7 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
         } catch (e: Exception) {
             try {
                 if (options.authenticationExtensions?.fidoAppIdExtension?.appId != null) {
-                    val appIdHash = options.authenticationExtensions.fidoAppIdExtension.appId.toByteArray()
+                    val appIdHash = options.authenticationExtensions!!.fidoAppIdExtension!!.appId.toByteArray()
                         .digest("SHA-256")
                     return ctap1sign(connection, options, clientDataHash, appIdHash)
                 }
@@ -278,7 +278,7 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
                 } catch (e: Ctap2StatusException) {
                     if (e.status == 0x2e.toByte() &&
                         connection.hasCtap1Support && connection.hasClientPin &&
-                        options.signOptions.allowList.isNotEmpty() &&
+                        options.signOptions.allowList.orEmpty().isNotEmpty() &&
                         options.signOptions.requireUserVerification != REQUIRED
                     ) {
                         Log.d(TAG, "Falling back to CTAP1/U2F")
@@ -297,7 +297,7 @@ abstract class TransportHandler(val transport: Transport, val callback: Transpor
             else -> throw IllegalStateException()
         }
         return AuthenticatorAssertionResponse(
-            credentialId,
+            credentialId ?: ByteArray(0).also { Log.w(TAG, "keyHandle was null") },
             clientData,
             response.authData,
             response.signature,

diff --git a/...c/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockTransportHandler.kt b/...c/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockTransportHandler.kt
@@ -191,7 +191,7 @@ class ScreenLockTransportHandler(private val activity: FragmentActivity, callbac
     ): AuthenticatorAssertionResponse {
         if (options.type != RequestOptionsType.SIGN) throw RequestHandlingException(ErrorCode.INVALID_STATE_ERR)
         val candidates = mutableListOf<CredentialId>()
-        for (descriptor in options.signOptions.allowList) {
+        for (descriptor in options.signOptions.allowList.orEmpty()) {
             try {
                 val (type, data) = CredentialId.decodeTypeAndData(descriptor.id)
                 if (type == 1.toByte() && store.containsKey(options.rpId, data)) {
@@ -238,7 +238,7 @@ class ScreenLockTransportHandler(private val activity: FragmentActivity, callbac
 
     override fun shouldBeUsedInstantly(options: RequestOptions): Boolean {
         if (options.type != RequestOptionsType.SIGN) return false
-        for (descriptor in options.signOptions.allowList) {
+        for (descriptor in options.signOptions.allowList.orEmpty()) {
             try {
                 val (type, data) = CredentialId.decodeTypeAndData(descriptor.id)
                 if (type == 1.toByte() && store.containsKey(options.rpId, data)) {

diff --git a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/ui/AuthenticatorActivity.kt b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/ui/AuthenticatorActivity.kt
@@ -154,14 +154,14 @@ class AuthenticatorActivity : AppCompatActivity(), TransportHandlerCallback {
                 val knownRegistrationTransports = mutableSetOf<Transport>()
                 val allowedTransports = mutableSetOf<Transport>()
                 if (options.type == RequestOptionsType.SIGN) {
-                    for (descriptor in options.signOptions.allowList) {
+                    for (descriptor in options.signOptions.allowList.orEmpty()) {
                         val knownTransport = database.getKnownRegistrationTransport(options.rpId, descriptor.id.toBase64(Base64.URL_SAFE, Base64.NO_WRAP, Base64.NO_PADDING))
                         if (knownTransport != null && knownTransport in IMPLEMENTED_TRANSPORTS)
                             knownRegistrationTransports.add(knownTransport)
                         if (descriptor.transports.isNullOrEmpty()) {
                             allowedTransports.addAll(Transport.values())
                         } else {
-                            for (transport in descriptor.transports) {
+                            for (transport in descriptor.transports.orEmpty()) {
                                 val allowedTransport = when (transport) {
                                     com.google.android.gms.fido.common.Transport.BLUETOOTH_CLASSIC -> BLUETOOTH
                                     com.google.android.gms.fido.common.Transport.BLUETOOTH_LOW_ENERGY -> BLUETOOTH
@@ -228,8 +228,8 @@ class AuthenticatorActivity : AppCompatActivity(), TransportHandlerCallback {
         if (rpId != null && id != null) database.insertKnownRegistration(rpId, id, transport)
         finishWithCredential(PublicKeyCredential.Builder()
             .setResponse(response)
-            .setRawId(rawId)
-            .setId(id)
+            .setRawId(rawId ?: ByteArray(0).also { Log.w(TAG, "rawId was null") })
+            .setId(id ?: "".also { Log.w(TAG, "id was null") })
             .setAuthenticatorAttachment(if (transport == SCREEN_LOCK) "platform" else "cross-platform")
             .build()
         )

diff --git a/...in/java/com/google/android/gms/fido/fido2/api/common/AttestationConveyancePreference.java b/...in/java/com/google/android/gms/fido/fido2/api/common/AttestationConveyancePreference.java
@@ -11,6 +11,7 @@
 import android.os.Parcel;
 import android.os.Parcelable;
 
+import androidx.annotation.NonNull;
 import org.microg.gms.common.PublicApi;
 
 /**
@@ -28,6 +29,7 @@ public enum AttestationConveyancePreference implements Parcelable {
     }
 
     @Override
+    @NonNull
     public String toString() {
         return value;
     }
@@ -43,6 +45,7 @@ public void writeToParcel(Parcel dest, int flags) {
     }
 
     @PublicApi(exclude = true)
+    @NonNull
     public static AttestationConveyancePreference fromString(String attachment) throws UnsupportedAttestationConveyancePreferenceException {
         for (AttestationConveyancePreference value : values()) {
             if (value.value.equals(attachment)) return value;

diff --git a/.../src/main/java/com/google/android/gms/fido/fido2/api/common/AuthenticationExtensions.java b/.../src/main/java/com/google/android/gms/fido/fido2/api/common/AuthenticationExtensions.java
@@ -8,6 +8,8 @@
 
 package com.google.android.gms.fido.fido2.api.common;
 
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import org.microg.gms.common.PublicApi;
 import org.microg.gms.utils.ToStringHelper;
 import org.microg.safeparcel.AutoSafeParcelable;
@@ -24,16 +26,21 @@
 @PublicApi
 public class AuthenticationExtensions extends AutoSafeParcelable {
     @Field(2)
+    @Nullable
     private FidoAppIdExtension fidoAppIdExtension;
     @Field(3)
+    @Nullable
     private CableAuthenticationExtension cableAuthenticationExtension;
     @Field(4)
+    @Nullable
     private UserVerificationMethodExtension userVerificationMethodExtension;
 
+    @Nullable
     public FidoAppIdExtension getFidoAppIdExtension() {
         return fidoAppIdExtension;
     }
 
+    @Nullable
     public UserVerificationMethodExtension getUserVerificationMethodExtension() {
         return userVerificationMethodExtension;
     }
@@ -70,7 +77,9 @@ public String toString() {
      * Builder for {@link AuthenticationExtensions}.
      */
     public static class Builder {
+        @Nullable
         private FidoAppIdExtension fidoAppIdExtension;
+        @Nullable
         private UserVerificationMethodExtension userVerificationMethodExtension;
 
         /**
@@ -83,7 +92,7 @@ public Builder() {
          * Sets the App ID extension, which allows for authentication of U2F authenticators previously registered
          * under the supplied App ID.
          */
-        public Builder setFido2Extension(FidoAppIdExtension appIdExtension) {
+        public Builder setFido2Extension(@Nullable FidoAppIdExtension appIdExtension) {
             this.fidoAppIdExtension = appIdExtension;
             return this;
         }
@@ -92,14 +101,15 @@ public Builder setFido2Extension(FidoAppIdExtension appIdExtension) {
          * Sets the User Verification Method extension, which allows the relying party to ascertain up to three
          * authentication methods that were used.
          */
-        public Builder setUserVerificationMethodExtension(UserVerificationMethodExtension userVerificationMethodExtension) {
+        public Builder setUserVerificationMethodExtension(@Nullable UserVerificationMethodExtension userVerificationMethodExtension) {
             this.userVerificationMethodExtension = userVerificationMethodExtension;
             return this;
         }
 
         /**
          * Builds the {@link AuthenticationExtensions} object.
          */
+        @NonNull
         public AuthenticationExtensions build() {
             AuthenticationExtensions extensions = new AuthenticationExtensions();
             extensions.fidoAppIdExtension = fidoAppIdExtension;

diff --git a/...a/com/google/android/gms/fido/fido2/api/common/AuthenticationExtensionsClientOutputs.java b/...a/com/google/android/gms/fido/fido2/api/common/AuthenticationExtensionsClientOutputs.java
@@ -8,6 +8,8 @@
 
 package com.google.android.gms.fido.fido2.api.common;
 
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import com.google.android.gms.common.internal.safeparcel.SafeParcelableSerializer;
 import org.microg.gms.common.PublicApi;
 import org.microg.safeparcel.AutoSafeParcelable;
@@ -20,8 +22,10 @@
 @PublicApi
 public class AuthenticationExtensionsClientOutputs extends AutoSafeParcelable {
     @Field(1)
+    @Nullable
     private UvmEntries uvmEntries;
 
+    @Nullable
     public UvmEntries getUvmEntries() {
         return uvmEntries;
     }
@@ -30,6 +34,7 @@ public UvmEntries getUvmEntries() {
      * Serializes the {@link AuthenticationExtensionsClientOutputs} to bytes.
      * Use {@link #deserializeFromBytes(byte[])} to deserialize.
      */
+    @NonNull
     public byte[] serializeToBytes() {
         return SafeParcelableSerializer.serializeToBytes(this);
     }
@@ -39,6 +44,7 @@ public byte[] serializeToBytes() {
      *
      * @return The deserialized {@link AuthenticationExtensionsClientOutputs}
      */
+    @NonNull
     public static AuthenticationExtensionsClientOutputs deserializeFromBytes(byte[] serializedBytes) {
         return SafeParcelableSerializer.deserializeFromBytes(serializedBytes, CREATOR);
     }
@@ -62,6 +68,7 @@ public int hashCode() {
      * Builder for {@link AuthenticationExtensionsClientOutputs}.
      */
     public static class Builder {
+        @Nullable
         private UvmEntries uvmEntries;
 
         /**
@@ -74,14 +81,15 @@ public Builder() {
          * Sets the User Verification Method extension, which allows the relying party to ascertain up to three
          * authentication methods that were used.
          */
-        public Builder setUserVerificationMethodEntries(UvmEntries uvmEntries) {
+        public Builder setUserVerificationMethodEntries(@Nullable UvmEntries uvmEntries) {
             this.uvmEntries = uvmEntries;
             return this;
         }
 
         /**
          * Builds the {@link AuthenticationExtensionsClientOutputs} object.
          */
+        @NonNull
         public AuthenticationExtensionsClientOutputs build() {
             AuthenticationExtensionsClientOutputs extensions = new AuthenticationExtensionsClientOutputs();
             extensions.uvmEntries = uvmEntries;