From 7881be07edf22dc776349b86407b7234f41c9036 Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Thu, 10 Dec 2015 15:03:34 +0100 Subject: [PATCH] Delete a lot of Kryo/serialisation related boilerplate. This is/was an attempt to be secure against malicious streams, but as Kryo is just a temporary bit of scaffolding and isn't intended to be actually used in any real product, it was just a waste of time and the registration requirement was getting increasingly awkward. --- src/main/kotlin/contracts/CrowdFund.kt | 9 +- .../kotlin/contracts/JavaCommercialPaper.java | 3 +- src/main/kotlin/core/ContractsDSL.kt | 3 +- src/main/kotlin/core/Crypto.kt | 3 +- src/main/kotlin/core/Structures.kt | 13 +- src/main/kotlin/core/Transactions.kt | 9 +- src/main/kotlin/core/Utils.kt | 3 +- src/main/kotlin/core/serialization/Kryo.kt | 243 ++---------------- .../kotlin/core/serialization/KryoTests.kt | 25 +- 9 files changed, 37 insertions(+), 274 deletions(-) diff --git a/src/main/kotlin/contracts/CrowdFund.kt b/src/main/kotlin/contracts/CrowdFund.kt index 5fd068dadc..9c1548bb29 100644 --- a/src/main/kotlin/contracts/CrowdFund.kt +++ b/src/main/kotlin/contracts/CrowdFund.kt @@ -9,7 +9,6 @@ package contracts import core.* -import core.serialization.SerializeableWithKryo import java.security.PublicKey import java.time.Instant import java.util.* @@ -47,7 +46,7 @@ class CrowdFund : Contract { val name: String, val target: Amount, val closingTime: Instant - ) : SerializeableWithKryo { + ) { override fun toString() = "Crowdsourcing($target sought by $owner by $closingTime)" } @@ -62,9 +61,9 @@ class CrowdFund : Contract { } data class Pledge( - val owner: PublicKey, - val amount: Amount - ) : SerializeableWithKryo + val owner: PublicKey, + val amount: Amount + ) interface Commands : Command { diff --git a/src/main/kotlin/contracts/JavaCommercialPaper.java b/src/main/kotlin/contracts/JavaCommercialPaper.java index 0df66d4f63..8a0077ee0e 100644 --- a/src/main/kotlin/contracts/JavaCommercialPaper.java +++ b/src/main/kotlin/contracts/JavaCommercialPaper.java @@ -10,7 +10,6 @@ package contracts; import core.*; import core.TransactionForVerification.*; -import core.serialization.*; import org.jetbrains.annotations.*; import java.security.*; @@ -27,7 +26,7 @@ import static kotlin.CollectionsKt.*; * NOTE: For illustration only. Not unit tested. */ public class JavaCommercialPaper implements Contract { - public static class State implements ContractState, SerializeableWithKryo { + public static class State implements ContractState { private PartyReference issuance; private PublicKey owner; private Amount faceValue; diff --git a/src/main/kotlin/core/ContractsDSL.kt b/src/main/kotlin/core/ContractsDSL.kt index cde752c597..b6753d2a4a 100644 --- a/src/main/kotlin/core/ContractsDSL.kt +++ b/src/main/kotlin/core/ContractsDSL.kt @@ -8,7 +8,6 @@ package core -import core.serialization.SerializeableWithKryo import java.math.BigDecimal import java.security.PublicKey import java.util.* @@ -67,7 +66,7 @@ inline fun requireThat(body: Requirements.() -> Unit) { * TODO: Should amount be abstracted to cover things like quantities of a stock, bond, commercial paper etc? Probably. * TODO: Think about how positive-only vs positive-or-negative amounts can be represented in the type system. */ -data class Amount(val pennies: Long, val currency: Currency) : Comparable, SerializeableWithKryo { +data class Amount(val pennies: Long, val currency: Currency) : Comparable { init { // Negative amounts are of course a vital part of any ledger, but negative values are only valid in certain // contexts: you cannot send a negative amount of cash, but you can (sometimes) have a negative balance. diff --git a/src/main/kotlin/core/Crypto.kt b/src/main/kotlin/core/Crypto.kt index c68059d80e..9d0e151316 100644 --- a/src/main/kotlin/core/Crypto.kt +++ b/src/main/kotlin/core/Crypto.kt @@ -9,7 +9,6 @@ package core import com.google.common.io.BaseEncoding -import core.serialization.SerializeableWithKryo import java.math.BigInteger import java.security.* @@ -71,7 +70,7 @@ object NullPublicKey : PublicKey, Comparable { override fun toString() = "NULL_KEY" } -class DummyPublicKey(val s: String) : PublicKey, Comparable, SerializeableWithKryo { +class DummyPublicKey(val s: String) : PublicKey, Comparable { override fun getAlgorithm() = "DUMMY" override fun getEncoded() = s.toByteArray() override fun getFormat() = "ASN.1" diff --git a/src/main/kotlin/core/Structures.kt b/src/main/kotlin/core/Structures.kt index c429f14305..4115350ee6 100644 --- a/src/main/kotlin/core/Structures.kt +++ b/src/main/kotlin/core/Structures.kt @@ -8,7 +8,6 @@ package core -import core.serialization.SerializeableWithKryo import core.serialization.serialize import java.security.PublicKey @@ -17,7 +16,7 @@ import java.security.PublicKey * file that the program can use to persist data across transactions. States are immutable: once created they are never * updated, instead, any changes must generate a new successor state. */ -interface ContractState : SerializeableWithKryo { +interface ContractState { /** * Refers to a bytecode program that has previously been published to the network. This contract program * will be executed any time this state is used in an input. It must accept in order for the @@ -41,13 +40,13 @@ fun ContractState.hash(): SecureHash = SecureHash.sha256((serialize())) * A stateref is a pointer to a state, this is an equivalent of an "outpoint" in Bitcoin. It records which transaction * defined the state and where in that transaction it was. */ -data class ContractStateRef(val txhash: SecureHash, val index: Int) : SerializeableWithKryo +data class ContractStateRef(val txhash: SecureHash, val index: Int) /** A StateAndRef is simply a (state, ref) pair. For instance, a wallet (which holds available assets) contains these. */ -data class StateAndRef(val state: T, val ref: ContractStateRef) : SerializeableWithKryo +data class StateAndRef(val state: T, val ref: ContractStateRef) /** A [Party] is well known (name, pubkey) pair. In a real system this would probably be an X.509 certificate. */ -data class Party(val name: String, val owningKey: PublicKey) : SerializeableWithKryo { +data class Party(val name: String, val owningKey: PublicKey) { override fun toString() = name fun ref(bytes: OpaqueBytes) = PartyReference(this, bytes) @@ -58,12 +57,12 @@ data class Party(val name: String, val owningKey: PublicKey) : SerializeableWith * Reference to something being stored or issued by a party e.g. in a vault or (more likely) on their normal * ledger. The reference is intended to be encrypted so it's meaningless to anyone other than the party. */ -data class PartyReference(val party: Party, val reference: OpaqueBytes) : SerializeableWithKryo { +data class PartyReference(val party: Party, val reference: OpaqueBytes) { override fun toString() = "${party.name}$reference" } /** Marker interface for classes that represent commands */ -interface Command : SerializeableWithKryo +interface Command /** Commands that inherit from this are intended to have no data items: it's only their presence that matters. */ abstract class TypeOnlyCommand : Command { diff --git a/src/main/kotlin/core/Transactions.kt b/src/main/kotlin/core/Transactions.kt index 021cb39b98..40dd2bdac4 100644 --- a/src/main/kotlin/core/Transactions.kt +++ b/src/main/kotlin/core/Transactions.kt @@ -8,7 +8,6 @@ package core -import core.serialization.SerializeableWithKryo import core.serialization.deserialize import core.serialization.serialize import java.security.KeyPair @@ -49,14 +48,14 @@ import java.util.* */ /** Serialized command plus pubkey pair: the signature is stored at the end of the serialized bytes */ -data class WireCommand(val command: Command, val pubkeys: List) : SerializeableWithKryo { +data class WireCommand(val command: Command, val pubkeys: List) { constructor(command: Command, key: PublicKey) : this(command, listOf(key)) } /** Transaction ready for serialisation, without any signatures attached. */ data class WireTransaction(val inputStates: List, val outputStates: List, - val commands: List) : SerializeableWithKryo { + val commands: List) { fun serializeForSignature(): ByteArray = serialize() fun toLedgerTransaction(timestamp: Instant?, partyKeyMap: Map, originalHash: SecureHash): LedgerTransaction { @@ -146,7 +145,7 @@ interface TimestamperService { fun verifyTimestamp(hash: SecureHash, signedTimestamp: ByteArray): Instant } -data class SignedWireTransaction(val txBits: OpaqueBytes, val sigs: List) : SerializeableWithKryo { +data class SignedWireTransaction(val txBits: OpaqueBytes, val sigs: List) { init { check(sigs.isNotEmpty()) } @@ -201,7 +200,7 @@ data class TimestampedWireTransaction( /** Signature from a timestamping authority. For instance using RFC 3161 */ val timestamp: OpaqueBytes? -) : SerializeableWithKryo { +) { val transactionID: SecureHash = serialize().sha256() fun verifyToLedgerTransaction(timestamper: TimestamperService, partyKeyMap: Map): LedgerTransaction { diff --git a/src/main/kotlin/core/Utils.kt b/src/main/kotlin/core/Utils.kt index 134b38b861..1ce443b211 100644 --- a/src/main/kotlin/core/Utils.kt +++ b/src/main/kotlin/core/Utils.kt @@ -9,12 +9,11 @@ package core import com.google.common.io.BaseEncoding -import core.serialization.SerializeableWithKryo import java.time.Duration import java.util.* /** A simple class that wraps a byte array and makes the equals/hashCode/toString methods work as you actually expect */ -open class OpaqueBytes(val bits: ByteArray) : SerializeableWithKryo { +open class OpaqueBytes(val bits: ByteArray) { init { check(bits.isNotEmpty()) } companion object { diff --git a/src/main/kotlin/core/serialization/Kryo.kt b/src/main/kotlin/core/serialization/Kryo.kt index e726025f6e..2620eadebd 100644 --- a/src/main/kotlin/core/serialization/Kryo.kt +++ b/src/main/kotlin/core/serialization/Kryo.kt @@ -9,26 +9,11 @@ package core.serialization import com.esotericsoftware.kryo.Kryo -import com.esotericsoftware.kryo.KryoException -import com.esotericsoftware.kryo.Serializer import com.esotericsoftware.kryo.io.Input import com.esotericsoftware.kryo.io.Output -import com.esotericsoftware.kryo.serializers.JavaSerializer -import contracts.Cash -import contracts.CommercialPaper -import contracts.CrowdFund -import core.* +import core.OpaqueBytes +import org.objenesis.strategy.StdInstantiatorStrategy import java.io.ByteArrayOutputStream -import java.lang.reflect.InvocationTargetException -import java.security.KeyPairGenerator -import java.security.PublicKey -import java.time.Instant -import java.util.* -import kotlin.reflect.KClass -import kotlin.reflect.KMutableProperty -import kotlin.reflect.jvm.javaType -import kotlin.reflect.memberProperties -import kotlin.reflect.primaryConstructor /** * Serialization utilities, using the Kryo framework with a custom serialiser for immutable data classes and a dead @@ -47,145 +32,23 @@ import kotlin.reflect.primaryConstructor * * But for now we use Kryo to maximise prototyping speed. * - * The goals of this code are twofold: + * Note that this code ignores *ALL* concerns beyond convenience, in particular it ignores: * - * 1) Security - * 2) Convenience - * - * in that order. - * - * SECURITY - * -------- - * - * Even though this is prototype code, we should still respect the Java Secure Coding Guidelines and the advice it - * gives for the use of object graph serialisation: - * - * http://www.oracle.com/technetwork/java/seccodeguide-139067.html - * - * Object graph serialisation is convenient but has a long history of exposing apps to security holes when type system - * invariants are violated by objects being reconstructed unexpectedly, or in illegal states. - * - * Therefore we take the following measures: - * - * - The ImmutableClassSerializer knows how to build deserialised objects using the primary constructor. Any invariants - * enforced by this constructor are therefore enforced (in Kotlin this means logic inside an init{} block, in - * Java it means any code in the only defined constructor). - * - The ICS asserts that Kryo is configured to only deserialise registered classes. Every class that might appear - * in a stream must be specified up front: Kryo will not rummage through the classpath to find any arbitrary - * class the stream happens to mention. This improves both performance and security at a loss of developer - * convenience. - * - * The ICS is intended to be used with classes that meet the following constraints: - * - * - Must be immutable: all properties are final. Note that Kotlin never generates bare public fields, but we should - * add some checks for this being done anyway for cases where a contract is defined using Java. - * - Requires that the data class be marked as intended for serialization using a marker interface. - * - Properties that are not in the constructor are not serialised (but as they are final, they must be either - * initialised to a constant or derived from the constructor arguments unless they are reading external state, - * which is intended to be forbidden). - * - * - * CONVENIENCE - * ----------- - * - * We define a few utilities to make using Kryo convenient. - * - * The createKryo() function returns a pre-configured Kryo class with a very small number of classes registered, that - * are known to be safe. - * - * A serialize() extension function is added to the SerializeableWithKryo marker interface. This is intended for - * serializing immutable data classes (i.e immutable javabeans). A deserialize() method is added to ByteArray to - * get the given class back from the stream. A Kryo.registerImmutableClass<>() function is added to clean up the Java - * syntax a bit: - * - * data class Person(val name: String, val birthdate: Instant?) : SerializeableWithKryo - * - * val kryo = kryo() - * kryo.registerImmutableClass() - * - * val bits: ByteArray = somePerson.serialize(kryo) - * val person2 = bits.deserialize(kryo) + * - Performance + * - Security * + * This code will happily deserialise literally anything, including malicious streams that would reconstruct classes + * in invalid states, thus violating system invariants. It isn't designed to handle malicious streams and therefore, + * isn't usable beyond the prototyping stage. But that's fine: we can revisit serialisation technologies later after + * a formal evaluation process. */ - -/** - * Marker interface for classes to use with [ImmutableClassSerializer]. Note that only constructor defined properties will - * be serialised! - */ -interface SerializeableWithKryo - -class ImmutableClassSerializer(val klass: KClass) : Serializer() { - val props = klass.memberProperties.sortedBy { it.name } - val propsByName = props.toMapBy { it.name } - val constructor = klass.primaryConstructor!! - - init { - // Verify that this class is immutable (all properties are final) - assert(props.none { it is KMutableProperty<*> }) - } - - override fun write(kryo: Kryo, output: Output, obj: T) { - output.writeVarInt(constructor.parameters.size, true) - output.writeInt(constructor.parameters.hashCode()) - for (param in constructor.parameters) { - val kProperty = propsByName[param.name!!]!! - when (param.type.javaType.typeName) { - "int" -> output.writeVarInt(kProperty.get(obj) as Int, true) - "long" -> output.writeVarLong(kProperty.get(obj) as Long, true) - "short" -> output.writeShort(kProperty.get(obj) as Int) - "char" -> output.writeChar(kProperty.get(obj) as Char) - "byte" -> output.writeByte(kProperty.get(obj) as Byte) - "double" -> output.writeDouble(kProperty.get(obj) as Double) - "float" -> output.writeFloat(kProperty.get(obj) as Float) - else -> try { - kryo.writeClassAndObject(output, kProperty.get(obj)) - } catch (e: Exception) { - throw IllegalStateException("Failed to serialize ${param.name} in ${klass.qualifiedName}", e) - } - } - } - } - - override fun read(kryo: Kryo, input: Input, type: Class): T { - assert(type.kotlin == klass) - assert(kryo.isRegistrationRequired) - val numFields = input.readVarInt(true) - val fieldTypeHash = input.readInt() - - // A few quick checks for data evolution. Note that this is not guaranteed to catch every problem! But it's - // good enough for a prototype. - if (numFields != constructor.parameters.size) - throw KryoException("Mismatch between number of constructor parameters and number of serialised fields for ${klass.qualifiedName} ($numFields vs ${constructor.parameters.size})") - if (fieldTypeHash != constructor.parameters.hashCode()) - throw KryoException("Hashcode mismatch for parameter types for ${klass.qualifiedName}: unsupported type evolution has happened.") - - val args = arrayOfNulls(numFields) - var cursor = 0 - for (param in constructor.parameters) { - args[cursor++] = when (param.type.javaType.typeName) { - "int" -> input.readVarInt(true) - "long" -> input.readVarLong(true) - "short" -> input.readShort() - "char" -> input.readChar() - "byte" -> input.readByte() - "double" -> input.readDouble() - "float" -> input.readFloat() - else -> kryo.readClassAndObject(input) - } - } - // If the constructor throws an exception, pass it through instead of wrapping it. - return try { constructor.call(*args) } catch (e: InvocationTargetException) { throw e.cause!! } - } -} - val THREAD_LOCAL_KRYO = ThreadLocal.withInitial { createKryo() } -inline fun Kryo.registerImmutableClass() = register(T::class.java, ImmutableClassSerializer(T::class)) -inline fun ByteArray.deserialize(kryo: Kryo = THREAD_LOCAL_KRYO.get()): T = kryo.readObject(Input(this), T::class.java) -inline fun OpaqueBytes.deserialize(kryo: Kryo = THREAD_LOCAL_KRYO.get()): T = kryo.readObject(Input(this.bits), T::class.java) +inline fun ByteArray.deserialize(kryo: Kryo = THREAD_LOCAL_KRYO.get()): T = kryo.readObject(Input(this), T::class.java) +inline fun OpaqueBytes.deserialize(kryo: Kryo = THREAD_LOCAL_KRYO.get()): T = kryo.readObject(Input(this.bits), T::class.java) -fun SerializeableWithKryo.serialize(kryo: Kryo = THREAD_LOCAL_KRYO.get()): ByteArray { +fun Any.serialize(kryo: Kryo = THREAD_LOCAL_KRYO.get()): ByteArray { val stream = ByteArrayOutputStream() Output(stream).use { kryo.writeObject(it, this) @@ -193,84 +56,12 @@ fun SerializeableWithKryo.serialize(kryo: Kryo = THREAD_LOCAL_KRYO.get()): ByteA return stream.toByteArray() } -private val UNUSED_EC_KEYPAIR = KeyPairGenerator.getInstance("EC").genKeyPair() - fun createKryo(): Kryo { return Kryo().apply { - // Require explicit listing of all types that can be deserialised, to defend against classes that aren't - // designed for serialisation being unexpectedly instantiated. - isRegistrationRequired = true - - // Allow various array and list types. Sometimes when the type is private/internal we have to give an example - // instead and then get the class from that. These have built in Kryo serializers that are safe to use. - register(ByteArray::class.java) - register(Collections.EMPTY_LIST.javaClass) - register(Collections.EMPTY_MAP.javaClass) - register(Collections.singletonList(null).javaClass) - register(Collections.singletonMap(1, 2).javaClass) - register(ArrayList::class.java) - register(emptyList().javaClass) - register(Arrays.asList(1,3).javaClass) - - // These JDK classes use a very minimal custom serialization format and are written to defend against malicious - // streams, so we can just kick it over to java serialization. We get ECPublicKeyImpl/ECPrivteKeyImpl via an - // example: it'd be faster to just import the sun.security.ec package directly, but that wouldn't play nice - // when Java 9 is released, as Project Jigsaw will make internal packages will become unavailable without hacks. - register(Instant::class.java, JavaSerializer()) - register(Currency::class.java, JavaSerializer()) // Only serialises the currency code as a string. - register(UNUSED_EC_KEYPAIR.private.javaClass, JavaSerializer()) - register(UNUSED_EC_KEYPAIR.public.javaClass, JavaSerializer()) - register(PublicKey::class.java, JavaSerializer()) - - // Now register platform types. - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - registerImmutableClass>() - - // Can't use data classes for this in Kotlin 1.0 due to lack of support for inheritance: must write a manual - // serialiser instead :( - register(DigitalSignature.WithKey::class.java, object : Serializer(false, true) { - override fun write(kryo: Kryo, output: Output, sig: DigitalSignature.WithKey) { - output.writeVarInt(sig.bits.size, true) - output.write(sig.bits) - output.writeInt(sig.covering, true) - kryo.writeObject(output, sig.by) - } - - override fun read(kryo: Kryo, input: Input, type: Class): DigitalSignature.WithKey { - val sigLen = input.readVarInt(true) - val sigBits = input.readBytes(sigLen) - val covering = input.readInt(true) - val pubkey = kryo.readObject(input, PublicKey::class.java) - return DigitalSignature.WithKey(pubkey, sigBits, covering) - } - }) - - // TODO: This is obviously a short term hack: there needs to be a way to bundle up and register contracts. - registerImmutableClass() - register(Cash.Commands.Move::class.java) - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - register(CommercialPaper.Commands.Move::class.java) - register(CommercialPaper.Commands.Redeem::class.java) - register(CommercialPaper.Commands.Issue::class.java) - registerImmutableClass() - registerImmutableClass() - registerImmutableClass() - register(CrowdFund.Commands.Register::class.java) - register(CrowdFund.Commands.Pledge::class.java) - register(CrowdFund.Commands.Close::class.java) - - // And for unit testing ... - registerImmutableClass() + // Allow any class to be deserialized (this is insecure but for prototyping we don't care) + isRegistrationRequired = false + // Allow construction of objects using a JVM backdoor that skips invoking the constructors, if there is no + // no-arg constructor available. + instantiatorStrategy = Kryo.DefaultInstantiatorStrategy(StdInstantiatorStrategy()) } } \ No newline at end of file diff --git a/src/test/kotlin/core/serialization/KryoTests.kt b/src/test/kotlin/core/serialization/KryoTests.kt index 749c4cff3b..e7eb7b06f3 100644 --- a/src/test/kotlin/core/serialization/KryoTests.kt +++ b/src/test/kotlin/core/serialization/KryoTests.kt @@ -12,28 +12,18 @@ import com.esotericsoftware.kryo.Kryo import org.junit.Test import java.time.Instant import kotlin.test.assertEquals -import kotlin.test.assertFailsWith import kotlin.test.assertNull class KryoTests { - data class Person(val name: String, val birthday: Instant?) : SerializeableWithKryo - data class MustBeWhizzy(val s: String) : SerializeableWithKryo { - init { - assert(s.startsWith("whiz")) { "must be whizzy" } - } - } + data class Person(val name: String, val birthday: Instant?) - private val kryo: Kryo = createKryo().apply { - registerImmutableClass() - registerImmutableClass() - } + private val kryo: Kryo = createKryo() @Test fun ok() { val april_17th = Instant.parse("1984-04-17T00:30:00.00Z") val mike = Person("mike", april_17th) val bits = mike.serialize(kryo) - assertEquals(64, bits.size) with(bits.deserialize(kryo)) { assertEquals("mike", name) assertEquals(april_17th, birthday) @@ -49,15 +39,4 @@ class KryoTests { assertNull(birthday) } } - - @Test - fun constructorInvariant() { - val pos = MustBeWhizzy("whizzle") - val bits = pos.serialize(kryo) - // Hack the serialized bytes here, like a very naughty hacker might. - bits[10] = 'o'.toByte() - assertFailsWith("must be whizzy") { - bits.deserialize(kryo) - } - } } \ No newline at end of file