Merge pull request #4963 from corda/colljos-backport-secfix-serializer

(BACKPORT) ENT-3121 restrict custom serializers

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

@@ -1,11 +1,30 @@
 package net.corda.serialization.internal.amqp
 
+import net.corda.core.CordaThrowable
 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 +33,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 +122,43 @@ 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
+        isSubClassOf(CordaThrowable::class.java) -> false
+        isAnnotationPresent(CordaSerializable::class.java) -> true
+        else -> false
     }
 }

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

@@ -0,0 +1,96 @@
+package net.corda.serialization.internal.amqp
+
+import net.corda.core.CordaException
+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.Test
+import java.lang.reflect.Type
+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 `exception types can have custom serializers`() {
+        @CordaSerializable
+        class MyCustomException : CordaException("Custom exception annotated with @CordaSerializable")
+
+        val customExceptionSerializer = TestCustomSerializer("a") { type -> type == MyCustomException::class.java }
+        unit.register(customExceptionSerializer)
+
+        assertSame(
+                customExceptionSerializer,
+                unit.find(MyCustomException::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)!!
+}