From 17e7cd3abc7cbf6130bd9fe361735c9279d1186a Mon Sep 17 00:00:00 2001 From: Dominic Fox <40790090+r3domfox@users.noreply.github.com> Date: Fri, 1 Mar 2019 14:53:08 +0000 Subject: [PATCH] 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 | 8 -- .../net/corda/core/concurrent/CordaFuture.kt | 2 - .../core/contracts/ContractAttachment.kt | 1 - .../TransactionVerificationException.kt | 1 - .../net/corda/core/crypto/CompositeKey.kt | 1 - .../kotlin/net/corda/core/crypto/NullKeys.kt | 1 - .../core/internal/ResolveTransactionsFlow.kt | 3 +- .../core/serialization/SerializationAPI.kt | 1 + .../core/transactions/SignedTransaction.kt | 1 - .../net/corda/core/utilities/ByteArrays.kt | 2 +- .../persistence/NodeAttachmentService.kt | 1 - .../NetworkRegistrationService.kt | 1 - .../internal/amqp/CustomSerializerRegistry.kt | 70 +++++++++++++--- .../amqp/CustomSerializerRegistryTests.kt | 84 +++++++++++++++++++ 14 files changed, 145 insertions(+), 32 deletions(-) create mode 100644 serialization/src/test/kotlin/net/corda/serialization/internal/amqp/CustomSerializerRegistryTests.kt diff --git a/.ci/api-current.txt b/.ci/api-current.txt index d717b5a464..1f04313ce4 100644 --- a/.ci/api-current.txt +++ b/.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, ? 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 (net.corda.core.contracts.Attachment, String) public (net.corda.core.contracts.Attachment, String, java.util.Set) @@ -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 (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 (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) 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 (java.util.Set, java.util.List, 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 (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 (byte[], int, int) @NotNull diff --git a/core/src/main/kotlin/net/corda/core/concurrent/CordaFuture.kt b/core/src/main/kotlin/net/corda/core/concurrent/CordaFuture.kt index 2f6d95795a..4977f3a34b 100644 --- a/core/src/main/kotlin/net/corda/core/concurrent/CordaFuture.kt +++ b/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 : Future { /** * Run the given callback when this future is done, on the completion thread. diff --git a/core/src/main/kotlin/net/corda/core/contracts/ContractAttachment.kt b/core/src/main/kotlin/net/corda/core/contracts/ContractAttachment.kt index 5fc1ad04de..6ba6b07d1b 100644 --- a/core/src/main/kotlin/net/corda/core/contracts/ContractAttachment.kt +++ b/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, diff --git a/core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt b/core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt index 760bd4b441..ef56349e7b 100644 --- a/core/src/main/kotlin/net/corda/core/contracts/TransactionVerificationException.kt +++ b/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) { diff --git a/core/src/main/kotlin/net/corda/core/crypto/CompositeKey.kt b/core/src/main/kotlin/net/corda/core/crypto/CompositeKey.kt index 64f64c3f75..9dbd400660 100644 --- a/core/src/main/kotlin/net/corda/core/crypto/CompositeKey.kt +++ b/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) : PublicKey { companion object { const val KEY_ALGORITHM = "COMPOSITE" diff --git a/core/src/main/kotlin/net/corda/core/crypto/NullKeys.kt b/core/src/main/kotlin/net/corda/core/crypto/NullKeys.kt index 151f00b950..47b7c823eb 100644 --- a/core/src/main/kotlin/net/corda/core/crypto/NullKeys.kt +++ b/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 { override fun getAlgorithm() = "NULL" override fun getEncoded() = byteArrayOf(0) diff --git a/core/src/main/kotlin/net/corda/core/internal/ResolveTransactionsFlow.kt b/core/src/main/kotlin/net/corda/core/internal/ResolveTransactionsFlow.kt index a0cec55f95..54fdce23eb 100644 --- a/core/src/main/kotlin/net/corda/core/internal/ResolveTransactionsFlow.kt +++ b/core/src/main/kotlin/net/corda/core/internal/ResolveTransactionsFlow.kt @@ -64,8 +64,7 @@ class ResolveTransactionsFlow(txHashesArg: Set, return sort.complete() } } - - @CordaSerializable + class ExcessivelyLargeTransactionGraph : FlowException() // TODO: Figure out a more appropriate DOS limit here, 5000 is simply a very bad guess. diff --git a/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt b/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt index 5ead84aef6..c97a511db2 100644 --- a/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt +++ b/core/src/main/kotlin/net/corda/core/serialization/SerializationAPI.kt @@ -330,6 +330,7 @@ fun T.serialize(serializationFactory: SerializationFactory = Serializa */ @Suppress("unused") @KeepForDJVM +@CordaSerializable class SerializedBytes(bytes: ByteArray) : OpaqueBytes(bytes) { companion object { /** diff --git a/core/src/main/kotlin/net/corda/core/transactions/SignedTransaction.kt b/core/src/main/kotlin/net/corda/core/transactions/SignedTransaction.kt index 6b5be2392a..a7e343161d 100644 --- a/core/src/main/kotlin/net/corda/core/transactions/SignedTransaction.kt +++ b/core/src/main/kotlin/net/corda/core/transactions/SignedTransaction.kt @@ -347,7 +347,6 @@ data class SignedTransaction(val txBits: SerializedBytes, } @KeepForDJVM - @CordaSerializable class SignaturesMissingException(val missing: Set, val descriptions: List, override val id: SecureHash) : NamedByHash, SignatureException(missingSignatureMsg(missing, descriptions, id)), CordaThrowable by CordaException(missingSignatureMsg(missing, descriptions, id)) diff --git a/core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt b/core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt index 296929d29b..a09f7e78b8 100644 --- a/core/src/main/kotlin/net/corda/core/utilities/ByteArrays.kt +++ b/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 { /** @@ -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 { /** diff --git a/node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt b/node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt index 13296ffd99..b316c11685 100644 --- a/node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt +++ b/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, diff --git a/node/src/main/kotlin/net/corda/node/utilities/registration/NetworkRegistrationService.kt b/node/src/main/kotlin/net/corda/node/utilities/registration/NetworkRegistrationService.kt index cafc16dd45..f62f318a0e 100644 --- a/node/src/main/kotlin/net/corda/node/utilities/registration/NetworkRegistrationService.kt +++ b/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?) -@CordaSerializable class CertificateRequestException(message: String) : CordaException(message) diff --git a/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/CustomSerializerRegistry.kt b/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/CustomSerializerRegistry.kt index 433b33b899..d1820a3c83 100644 --- a/serialization/src/main/kotlin/net/corda/serialization/internal/amqp/CustomSerializerRegistry.kt +++ b/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>, 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) 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? } @@ -89,28 +121,42 @@ class CachingCustomSerializerRegistry( } private fun doFindCustomSerializer(clazz: Class<*>, declaredType: Type): AMQPSerializer? { - // 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 - } 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 } } \ No newline at end of file diff --git a/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/CustomSerializerRegistryTests.kt b/serialization/src/test/kotlin/net/corda/serialization/internal/amqp/CustomSerializerRegistryTests.kt new file mode 100644 index 0000000000..dbed515a1a --- /dev/null +++ b/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() { + 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 { + 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 { + unit.find(Cash::class.java) + } + } + + @Test + fun `primitive types cannot have custom serializers`() { + unit.register(TestCustomSerializer("a") { type -> type == Float::class.java }) + + assertFailsWith { + unit.find(Float::class.java) + } + } + + private fun CustomSerializerRegistry.find(clazz: Class<*>): AMQPSerializer = findCustomSerializer(clazz, clazz)!! +} \ No newline at end of file