ENT-3121 - restrict custom serializers (#1852)

* Tests for custom registry restrictions

* ENT-3121 restrict custom serialisation

* Remove redundant code

* Only count declared annotations

* Check annotation on superclasses, remove annotation from ByteArray

* Forbid custom serialization of primitive types

* Remove @CordaSerializable from another class that is always handled by custom serialisation

* Add log warnings to aid diagnosis of custom serialization issues

* Remove another annotation

* Remove another annotation

* Remove another annotation

* Remove another annotation

* Fixup api-current

* Fixup api-current

* KDocs on exceptions

.ci/api-current.txt

@@ -88,7 +88,6 @@ public final class net.corda.core.concurrent.ConcurrencyUtils extends java.lang.
   @NotNull
   public static final String shortCircuitedTaskFailedMessage = "Short-circuited task failed:"
 ##
-@CordaSerializable
 public interface net.corda.core.concurrent.CordaFuture extends java.util.concurrent.Future
   public abstract void then(kotlin.jvm.functions.Function1<? super net.corda.core.concurrent.CordaFuture<V>, ? extends W>)
   @NotNull
@@ -571,7 +570,6 @@ public interface net.corda.core.contracts.Contract
   public abstract void verify(net.corda.core.transactions.LedgerTransaction)
 ##
 @DoNotImplement
-@CordaSerializable
 public final class net.corda.core.contracts.ContractAttachment extends java.lang.Object implements net.corda.core.contracts.Attachment
   public <init>(net.corda.core.contracts.Attachment, String)
   public <init>(net.corda.core.contracts.Attachment, String, java.util.Set<String>)
@@ -1013,7 +1011,6 @@ public final class net.corda.core.contracts.TransactionState extends java.lang.O
 ##
 public final class net.corda.core.contracts.TransactionStateKt extends java.lang.Object
 ##
-@CordaSerializable
 public abstract class net.corda.core.contracts.TransactionVerificationException extends net.corda.core.flows.FlowException
   public <init>(net.corda.core.crypto.SecureHash, String, Throwable)
   @NotNull
@@ -1390,7 +1387,6 @@ public class net.corda.core.crypto.Base58 extends java.lang.Object
   public static java.math.BigInteger decodeToBigInteger(String)
   public static String encode(byte[])
 ##
-@CordaSerializable
 public final class net.corda.core.crypto.CompositeKey extends java.lang.Object implements java.security.PublicKey
   public <init>(int, java.util.List, kotlin.jvm.internal.DefaultConstructorMarker)
   public final void checkValidity()
@@ -1746,7 +1742,6 @@ public final class net.corda.core.crypto.NullKeys extends java.lang.Object
   public final net.corda.core.crypto.TransactionSignature getNULL_SIGNATURE()
   public static final net.corda.core.crypto.NullKeys INSTANCE
 ##
-@CordaSerializable
 public static final class net.corda.core.crypto.NullKeys$NullPublicKey extends java.lang.Object implements java.security.PublicKey, java.lang.Comparable
   public int compareTo(java.security.PublicKey)
   @NotNull
@@ -6225,7 +6220,6 @@ public final class net.corda.core.transactions.SignedTransaction extends java.la
   public final net.corda.core.transactions.SignedTransaction withAdditionalSignatures(Iterable<net.corda.core.crypto.TransactionSignature>)
   public static final net.corda.core.transactions.SignedTransaction$Companion Companion
 ##
-@CordaSerializable
 public static final class net.corda.core.transactions.SignedTransaction$SignaturesMissingException extends java.security.SignatureException implements net.corda.core.CordaThrowable, net.corda.core.contracts.NamedByHash
   public <init>(java.util.Set<? extends java.security.PublicKey>, java.util.List<String>, net.corda.core.crypto.SecureHash)
   public void addSuppressed(Throwable[])
@@ -6412,7 +6406,6 @@ public final class net.corda.core.utilities.ByteArrays extends java.lang.Object
   @NotNull
   public static final String toHexString(byte[])
 ##
-@CordaSerializable
 public abstract class net.corda.core.utilities.ByteSequence extends java.lang.Object implements java.lang.Comparable
   public <init>(byte[], int, int, kotlin.jvm.internal.DefaultConstructorMarker)
   public int compareTo(net.corda.core.utilities.ByteSequence)
@@ -6628,7 +6621,6 @@ public static final class net.corda.core.utilities.OpaqueBytes$Companion extends
   @NotNull
   public final net.corda.core.utilities.OpaqueBytes of(byte...)
 ##
-@CordaSerializable
 public final class net.corda.core.utilities.OpaqueBytesSubSequence extends net.corda.core.utilities.ByteSequence
   public <init>(byte[], int, int)
   @NotNull

core/src/main/kotlin/net/corda/core/concurrent/CordaFuture.kt

@@ -1,6 +1,5 @@
 package net.corda.core.concurrent
 
-import net.corda.core.serialization.CordaSerializable
 import java.util.concurrent.CompletableFuture
 import java.util.concurrent.Future
 
@@ -8,7 +7,6 @@ import java.util.concurrent.Future
  * Same as [Future] with additional methods to provide some of the features of [java.util.concurrent.CompletableFuture] while minimising the API surface area.
  * In Kotlin, to avoid compile errors, whenever CordaFuture is used in a parameter or extension method receiver type, its type parameter should be specified with out variance.
  */
-@CordaSerializable
 interface CordaFuture<V> : Future<V> {
     /**
      * Run the given callback when this future is done, on the completion thread.

core/src/main/kotlin/net/corda/core/contracts/ContractAttachment.kt

@@ -14,7 +14,6 @@ import java.security.PublicKey
  * @property additionalContracts Additional contract names contained within the JAR.
  */
 @KeepForDJVM
-@CordaSerializable
 class ContractAttachment private constructor(
         val attachment: Attachment,
         val contract: ContractClassName,

core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt

@@ -46,7 +46,6 @@ class AttachmentResolutionException(val hash: SecureHash) : FlowException("Attac
  * @property txId the Merkle root hash (identifier) of the transaction that failed verification.
  */
 @Suppress("MemberVisibilityCanBePrivate")
-@CordaSerializable
 abstract class TransactionVerificationException(val txId: SecureHash, message: String, cause: Throwable?)
     : FlowException("$message, transaction: $txId", cause) {
 

core/src/main/kotlin/net/corda/core/crypto/CompositeKey.kt

@@ -29,7 +29,6 @@ import java.util.*
  * signatures required) to satisfy the sub-tree rooted at this node.
  */
 @KeepForDJVM
-@CordaSerializable
 class CompositeKey private constructor(val threshold: Int, children: List<NodeAndWeight>) : PublicKey {
     companion object {
         const val KEY_ALGORITHM = "COMPOSITE"

core/src/main/kotlin/net/corda/core/crypto/NullKeys.kt

@@ -7,7 +7,6 @@ import java.security.PublicKey
 
 @KeepForDJVM
 object NullKeys {
-    @CordaSerializable
     object NullPublicKey : PublicKey, Comparable<PublicKey> {
         override fun getAlgorithm() = "NULL"
         override fun getEncoded() = byteArrayOf(0)

core/src/main/kotlin/net/corda/core/internal/ResolveTransactionsFlow.kt

@@ -64,8 +64,7 @@ class ResolveTransactionsFlow(txHashesArg: Set<SecureHash>,
             return sort.complete()
         }
     }
-
-    @CordaSerializable
+    
     class ExcessivelyLargeTransactionGraph : FlowException()
 
     // TODO: Figure out a more appropriate DOS limit here, 5000 is simply a very bad guess.

core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt

@@ -330,6 +330,7 @@ fun <T : Any> T.serialize(serializationFactory: SerializationFactory = Serializa
  */
 @Suppress("unused")
 @KeepForDJVM
+@CordaSerializable
 class SerializedBytes<T : Any>(bytes: ByteArray) : OpaqueBytes(bytes) {
     companion object {
         /**

core/src/main/kotlin/net/corda/core/transactions/SignedTransaction.kt

@@ -347,7 +347,6 @@ data class SignedTransaction(val txBits: SerializedBytes<CoreTransaction>,
     }
 
     @KeepForDJVM
-    @CordaSerializable
     class SignaturesMissingException(val missing: Set<PublicKey>, val descriptions: List<String>, override val id: SecureHash)
         : NamedByHash, SignatureException(missingSignatureMsg(missing, descriptions, id)), CordaThrowable by CordaException(missingSignatureMsg(missing, descriptions, id))
 

core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt

@@ -20,7 +20,6 @@ import javax.xml.bind.DatatypeConverter
  * @property offset The start position of the sequence within the byte array.
  * @property size The number of bytes this sequence represents.
  */
-@CordaSerializable
 @KeepForDJVM
 sealed class ByteSequence(private val _bytes: ByteArray, val offset: Int, val size: Int) : Comparable<ByteSequence> {
     /**
@@ -145,6 +144,7 @@ sealed class ByteSequence(private val _bytes: ByteArray, val offset: Int, val si
  * functionality to Java, but it won't arrive for a few years yet!
  */
 @KeepForDJVM
+@CordaSerializable
 open class OpaqueBytes(bytes: ByteArray) : ByteSequence(bytes, 0, bytes.size) {
     companion object {
         /**

node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt

@@ -175,7 +175,6 @@ class NodeAttachmentService(
      * this will provide an additional safety check against user error.
      */
     @VisibleForTesting
-    @CordaSerializable
     class HashCheckingStream(val expected: SecureHash.SHA256,
                              val expectedSize: Int,
                              input: InputStream,

node/src/main/kotlin/net/corda/node/utilities/registration/NetworkRegistrationService.kt

@@ -17,5 +17,4 @@ interface NetworkRegistrationService {
 
 data class CertificateResponse(val pollInterval: Duration, val certificates: List<X509Certificate>?)
 
-@CordaSerializable
 class CertificateRequestException(message: String) : CordaException(message)

serialization/src/main/kotlin/net/corda/serialization/internal/amqp/CustomSerializerRegistry.kt

@@ -1,11 +1,29 @@
 package net.corda.serialization.internal.amqp
 
 import net.corda.core.internal.uncheckedCast
+import net.corda.core.serialization.CordaSerializable
 import net.corda.core.utilities.contextLogger
 import net.corda.serialization.internal.model.DefaultCacheProvider
 import net.corda.serialization.internal.model.TypeIdentifier
 import java.lang.reflect.Type
 
+/**
+ * Thrown when a [CustomSerializer] offers to serialize a type for which custom serialization is not permitted, because
+ * it should be handled by standard serialisation methods (or not serialised at all) and there is no valid use case for
+ * a custom method.
+ */
+class IllegalCustomSerializerException(customSerializer: AMQPSerializer<*>, clazz: Class<*>) :
+        Exception("Custom serializer ${customSerializer::class.qualifiedName} registered " +
+                "to serialize non-custom-serializable type $clazz")
+
+/**
+ * Thrown when more than one [CustomSerializer] offers to serialize the same type, which may indicate a malicious attempt
+ * to override already-defined behaviour.
+ */
+class DuplicateCustomSerializerException(serializers: List<AMQPSerializer<*>>, clazz: Class<*>) :
+        Exception("Multiple custom serializers " + serializers.map { it::class.qualifiedName } +
+                " registered to serialize type $clazz")
+
 interface CustomSerializerRegistry {
     /**
      * Register a custom serializer for any type that cannot be serialized or deserialized by the default serializer
@@ -14,6 +32,20 @@ interface CustomSerializerRegistry {
     fun register(customSerializer: CustomSerializer<out Any>)
     fun registerExternal(customSerializer: CorDappCustomSerializer)
 
+    /**
+     * Try to find a custom serializer for the actual class, and declared type, of a value.
+     *
+     * @param clazz The actual class to look for a custom serializer for.
+     * @param declaredType The declared type to look for a custom serializer for.
+     * @return The custom serializer handing the class, if found, or `null`.
+     *
+     * @throws IllegalCustomSerializerException If a custom serializer identifies itself as the serializer for
+     * a class annotated with [CordaSerializable], since all such classes should be serializable via standard object
+     * serialization.
+     *
+     * @throws DuplicateCustomSerializerException If more than one custom serializer identifies itself as the serializer
+     * for the given class, as this creates an ambiguous situation.
+     */
     fun findCustomSerializer(clazz: Class<*>, declaredType: Type): AMQPSerializer<Any>?
 }
 
@@ -89,28 +121,42 @@ class CachingCustomSerializerRegistry(
     }
 
     private fun doFindCustomSerializer(clazz: Class<*>, declaredType: Type): AMQPSerializer<Any>? {
-        // e.g. Imagine if we provided a Map serializer this way, then it won't work if the declared type is
-        // AbstractMap, only Map. Otherwise it needs to inject additional schema for a RestrictedType source of the
-        // super type.  Could be done, but do we need it?
-        for (customSerializer in customSerializers) {
-            if (customSerializer.isSerializerFor(clazz)) {
-                val declaredSuperClass = declaredType.asClass().superclass
+        val declaredSuperClass = declaredType.asClass().superclass
 
-                return if (declaredSuperClass == null
+        val declaredSerializers = customSerializers.mapNotNull { customSerializer ->
+            when {
+                !customSerializer.isSerializerFor(clazz) -> null
+                (declaredSuperClass == null
                         || !customSerializer.isSerializerFor(declaredSuperClass)
-                        || !customSerializer.revealSubclassesInSchema
-                ) {
+                        || !customSerializer.revealSubclassesInSchema) -> {
                     logger.debug("action=\"Using custom serializer\", class=${clazz.typeName}, " +
                             "declaredType=${declaredType.typeName}")
 
                     @Suppress("UNCHECKED_CAST")
                     customSerializer as? AMQPSerializer<Any>
-                } else {
+                }
+                else ->
                     // Make a subclass serializer for the subclass and return that...
                     CustomSerializer.SubClass(clazz, uncheckedCast(customSerializer))
-                }
             }
         }
-        return null
+
+        if (declaredSerializers.isEmpty()) return null
+        if (declaredSerializers.size > 1) {
+            logger.warn("Duplicate custom serializers detected for $clazz: ${declaredSerializers.map { it::class.qualifiedName }}")
+            throw DuplicateCustomSerializerException(declaredSerializers, clazz)
+        }
+        if (clazz.isCustomSerializationForbidden) {
+            logger.warn("Illegal custom serializer detected for $clazz: ${declaredSerializers.first()::class.qualifiedName}")
+            throw IllegalCustomSerializerException(declaredSerializers.first(), clazz)
+        }
+
+        return declaredSerializers.first()
+    }
+
+    private val Class<*>.isCustomSerializationForbidden: Boolean get() = when {
+        AMQPTypeIdentifiers.isPrimitive(this) -> true
+        isAnnotationPresent(CordaSerializable::class.java) -> true
+        else -> false
     }
 }

serialization/src/test/kotlin/net/corda/serialization/internal/amqp/CustomSerializerRegistryTests.kt

@@ -0,0 +1,84 @@
+package net.corda.serialization.internal.amqp
+
+import net.corda.core.serialization.CordaSerializable
+import net.corda.core.serialization.SerializationContext
+import net.corda.finance.contracts.asset.Cash
+import org.apache.qpid.proton.amqp.Symbol
+import org.apache.qpid.proton.codec.Data
+import org.junit.Ignore
+import org.junit.Test
+import java.lang.reflect.Type
+import java.math.BigDecimal
+import kotlin.test.assertFailsWith
+import kotlin.test.assertSame
+
+class CustomSerializerRegistryTests {
+
+    private val descriptorBasedRegistry = DefaultDescriptorBasedSerializerRegistry()
+    private val unit = CachingCustomSerializerRegistry(descriptorBasedRegistry)
+
+    class TestCustomSerializer(descriptorString: String, private val serializerFor: (Class<*>) -> Boolean): CustomSerializer<Any>() {
+        override fun isSerializerFor(clazz: Class<*>): Boolean = serializerFor(clazz)
+
+        override val descriptor: Descriptor get() = throw UnsupportedOperationException()
+        override val schemaForDocumentation: Schema get() = throw UnsupportedOperationException()
+
+        override fun writeDescribedObject(obj: Any, data: Data, type: Type, output: SerializationOutput, context: SerializationContext) {
+            throw UnsupportedOperationException()
+        }
+
+        override val type: Type get() = Any::class.java
+        override val typeDescriptor: Symbol = Symbol.valueOf(descriptorString)
+        override fun writeClassInfo(output: SerializationOutput) {
+            throw UnsupportedOperationException()
+        }
+
+        override fun readObject(obj: Any, schemas: SerializationSchemas, input: DeserializationInput, context: SerializationContext): Any {
+            throw UnsupportedOperationException()
+        }
+    }
+
+    @Test
+    fun `a custom serializer cannot register to serialize a type already annotated with CordaSerializable`() {
+        val serializerForEverything = TestCustomSerializer("a") { true }
+        unit.register(serializerForEverything)
+
+        @CordaSerializable
+        class AnnotatedWithCordaSerializable
+        class NotAnnotatedWithCordaSerializable
+
+        assertSame(
+                serializerForEverything,
+                unit.find(NotAnnotatedWithCordaSerializable::class.java))
+
+        assertFailsWith<IllegalCustomSerializerException> {
+            unit.find(AnnotatedWithCordaSerializable::class.java)
+        }
+    }
+
+    @Test
+    fun `two custom serializers cannot register to serialize the same type`() {
+        val weSerializeCash = TestCustomSerializer("a") { type -> type == Cash::class.java }
+        val weMaliciouslySerializeCash = TestCustomSerializer("b") { type -> type == Cash::class.java }
+
+        unit.run {
+            register(weSerializeCash)
+            register(weMaliciouslySerializeCash)
+        }
+
+        assertFailsWith<DuplicateCustomSerializerException> {
+            unit.find(Cash::class.java)
+        }
+    }
+
+    @Test
+    fun `primitive types cannot have custom serializers`() {
+        unit.register(TestCustomSerializer("a") { type -> type == Float::class.java })
+
+        assertFailsWith<IllegalCustomSerializerException> {
+            unit.find(Float::class.java)
+        }
+    }
+
+    private fun CustomSerializerRegistry.find(clazz: Class<*>): AMQPSerializer<Any> = findCustomSerializer(clazz, clazz)!!
+}