Rewrite the AttachmentsClassLoader to avoid temporary file copies and fix the overlap check.

Throw a specialised exception that lists missing exceptions during deserialisation, so the dependency resolution code can access it (coming up).

core/build.gradle

@@ -28,6 +28,9 @@ dependencies {
     compile "org.jetbrains.kotlin:kotlin-reflect:$kotlin_version"
     compile "org.jetbrains.kotlin:kotlin-test:$kotlin_version"
 
+    // Thread safety annotations
+    compile "com.google.code.findbugs:jsr305:3.0.1"
+
     // SLF4J: Logging framework.
     compile "org.slf4j:slf4j-jdk14:1.7.13"
 

core/src/main/kotlin/core/node/AttachmentsClassLoader.kt

@@ -1,67 +1,106 @@
 package core.node
 
 import core.Attachment
-import java.io.Closeable
+import core.crypto.SecureHash
-import java.io.File
+import java.io.ByteArrayInputStream
-import java.io.FileOutputStream
+import java.io.ByteArrayOutputStream
+import java.io.FileNotFoundException
+import java.io.InputStream
 import java.net.URL
-import java.net.URLClassLoader
+import java.net.URLConnection
+import java.net.URLStreamHandler
+import java.security.CodeSigner
+import java.security.CodeSource
+import java.security.SecureClassLoader
 import java.util.*
-import java.util.jar.JarEntry
-
-class OverlappingAttachments : Exception()
 
 /**
- * A custom ClassLoader for creating contracts distributed as attachments and for contracts to
+ * A custom ClassLoader that knows how to load classes from a set of attachments. The attachments themselves only
- * access attachments.
+ * need to provide JAR streams, and so could be fetched from a database, local disk, etc. Constructing an
+ * AttachmentsClassLoader is somewhat expensive, as every attachment is scanned to ensure that there are no overlapping
+ * file paths.
  */
-class AttachmentsClassLoader private constructor(val tmpFiles: List<File>)
+class AttachmentsClassLoader(attachments: List<Attachment>) : SecureClassLoader() {
-: URLClassLoader(tmpFiles.map { URL("file", "", it.toString()) }.toTypedArray()), Closeable {
+    private val pathsToAttachments = HashMap<String, Attachment>()
+    private val idsToAttachments = HashMap<SecureHash, Attachment>()
 
-    override fun close() {
+    class OverlappingAttachments(val path: String) : Exception() {
-        super.close()
+        override fun toString() = "Multiple attachments define a file at path $path"
+    }
 
-        for (file in tmpFiles) {
+    init {
-            file.delete()
+        for (attachment in attachments) {
+            attachment.openAsJAR().use { jar ->
+                while (true) {
+                    val entry = jar.nextJarEntry ?: break
+
+                    // We already verified that paths are not strange/game playing when we inserted the attachment
+                    // into the storage service. So we don't need to repeat it here.
+                    //
+                    // We forbid files that differ only in case to avoid issues for Windows/Mac developers where the
+                    // filesystem tries to be case insensitive. This may break developers who attempt to use ProGuard.
+                    //
+                    // TODO: Do we need extra overlap checks?
+                    val path = entry.name.toLowerCase()
+                    if (path in pathsToAttachments)
+                        throw OverlappingAttachments(path)
+                    pathsToAttachments[path] = attachment
+                }
+            }
+            idsToAttachments[attachment.id] = attachment
         }
     }
 
-    override fun loadClass(name: String?, resolve: Boolean): Class<*>? {
+    // Example: attachment://0b4fc1327f3bbebf1bfe98330ea402ae035936c3cb6da9bd3e26eeaa9584e74d/some/file.txt
-        return super.loadClass(name, resolve)
+    //
+    // We have to provide a fake stream handler to satisfy the URL class that the scheme is known. But it's not
+    // a real scheme and we don't register it. It's just here to ensure that there aren't codepaths that could
+    // lead to data loading that we don't control right here in this class (URLs can have evil security properties!)
+    private val fakeStreamHandler = object : URLStreamHandler() {
+        override fun openConnection(u: URL?): URLConnection? {
+            throw UnsupportedOperationException()
+        }
     }
 
-    companion object {
+    private fun Attachment.toURL(path: String?) = URL(null, "attachment://$id/" + (path ?: ""), fakeStreamHandler)
-        fun create(streams: List<Attachment>): AttachmentsClassLoader {
 
-            validate(streams)
+    override fun findClass(name: String): Class<*> {
-
+        val path = name.replace('.', '/').toLowerCase() + ".class"
-            var tmpFiles = streams.map {
+        val attachment = pathsToAttachments[path] ?: throw ClassNotFoundException(name)
-                var filename = File.createTempFile("jar", "")
+        val stream = ByteArrayOutputStream()
-                it.open().use {
+        try {
-                    str ->
+            attachment.extractFile(path, stream)
-                    FileOutputStream(filename).use { str.copyTo(it) }
+        } catch(e: FileNotFoundException) {
+            throw ClassNotFoundException(name)
         }
-                filename
+        val bytes = stream.toByteArray()
+        // We don't attempt to propagate signatures from the JAR into the codesource, because our sandbox does not
+        // depend on external policy files to specify what it can do, so the data wouldn't be useful.
+        val codesource = CodeSource(attachment.toURL(null), emptyArray<CodeSigner>())
+        // TODO: Define an empty ProtectionDomain to start enforcing the standard Java sandbox.
+        // The standard Java sandbox is insufficient for our needs and a much more sophisticated sandboxing
+        // ClassLoader will appear here in future, but it can't hurt to use the default one too: defence in depth!
+        return defineClass(name, bytes, 0, bytes.size, codesource)
     }
 
-            return AttachmentsClassLoader(tmpFiles)
+    override fun findResource(name: String): URL? {
+        val attachment = pathsToAttachments[name.toLowerCase()] ?: return null
+        return attachment.toURL(name)
     }
 
-        private fun validate(streams: List<Attachment>) {
+    override fun getResourceAsStream(name: String): InputStream? {
-            val set = HashSet<String>()
+        val url = getResource(name) ?: return null   // May check parent classloaders, for example.
-
+        if (url.protocol != "attachment") return null
-            val jars = streams.map { it.openAsJAR() }
+        val attachment = idsToAttachments[SecureHash.parse(url.host)] ?: return null
-
+        val path = url.path?.substring(1) ?: return null   // Chop off the leading slash.
-            for (jar in jars) {
+        try {
-
+            val stream = ByteArrayOutputStream()
-                var entry: JarEntry = jar.nextJarEntry ?: continue
+            attachment.extractFile(path, stream)
-                if (set.add(entry.name) == false) {
+            return ByteArrayInputStream(stream.toByteArray())
-                    throw OverlappingAttachments()
+        } catch(e: FileNotFoundException) {
+            return null
         }
     }
 }
 
-    }
-}
-
 

core/src/main/kotlin/core/serialization/Kryo.kt

@@ -12,6 +12,7 @@ import core.*
 import core.crypto.SecureHash
 import core.crypto.generateKeyPair
 import core.crypto.sha256
+import core.node.AttachmentsClassLoader
 import core.node.services.AttachmentStorage
 import de.javakaffee.kryoserializers.ArraysAsListSerializer
 import org.objenesis.strategy.StdInstantiatorStrategy
@@ -21,6 +22,7 @@ import java.nio.file.Files
 import java.nio.file.Path
 import java.time.Instant
 import java.util.*
+import javax.annotation.concurrent.ThreadSafe
 import kotlin.reflect.*
 import kotlin.reflect.jvm.javaType
 
@@ -189,6 +191,51 @@ inline fun <T> Kryo.useClassLoader(cl: ClassLoader, body: () -> T) : T {
     }
 }
 
+/** Thrown during deserialisation to indicate that an attachment needed to construct the [WireTransaction] is not found */
+class MissingAttachmentsException(val ids: List<SecureHash>) : Exception()
+
+/** A serialisation engine that knows how to deserialise code inside a sandbox */
+@ThreadSafe
+object WireTransactionSerializer : Serializer<WireTransaction>() {
+    override fun write(kryo: Kryo, output: Output, obj: WireTransaction) {
+        kryo.writeClassAndObject(output, obj.inputs)
+        kryo.writeClassAndObject(output, obj.attachments)
+        kryo.writeClassAndObject(output, obj.outputs)
+        kryo.writeClassAndObject(output, obj.commands)
+    }
+
+    @Suppress("UNCHECKED_CAST")
+    override fun read(kryo: Kryo, input: Input, type: Class<WireTransaction>): WireTransaction {
+        val inputs = kryo.readClassAndObject(input) as List<StateRef>
+        val attachmentHashes = kryo.readClassAndObject(input) as List<SecureHash>
+
+        // If we're deserialising in the sandbox context, we use our special attachments classloader.
+        // Otherwise we just assume the code we need is on the classpath already.
+        val attachmentStorage = kryo.attachmentStorage
+        val classLoader = if (attachmentStorage != null) {
+            val missing = ArrayList<SecureHash>()
+            val attachments = ArrayList<Attachment>()
+            for (id in attachmentHashes) {
+                val attachment = attachmentStorage.openAttachment(id)
+                if (attachment == null)
+                    missing += id
+                else
+                    attachments += attachment
+            }
+            if (missing.isNotEmpty())
+                throw MissingAttachmentsException(missing)
+            AttachmentsClassLoader(attachments)
+        } else javaClass.classLoader
+
+        kryo.useClassLoader(classLoader) {
+            val outputs = kryo.readClassAndObject(input) as List<ContractState>
+            val commands = kryo.readClassAndObject(input) as List<Command>
+
+            return WireTransaction(inputs, attachmentHashes, outputs, commands)
+        }
+    }
+}
+
 fun createKryo(k: Kryo = Kryo()): Kryo {
     return k.apply {
         // Allow any class to be deserialized (this is insecure but for prototyping we don't care)
@@ -211,35 +258,7 @@ fun createKryo(k: Kryo = Kryo()): Kryo {
             }
         })
 
-       register(WireTransaction::class.java, object : Serializer<WireTransaction>() {
+        register(WireTransaction::class.java, WireTransactionSerializer)
-            override fun write(kryo: Kryo, output: Output, obj: WireTransaction) {
-
-                kryo.writeClassAndObject( output, obj.inputs )
-                kryo.writeClassAndObject( output, obj.attachments )
-
-                kryo.writeClassAndObject( output, obj.outputs )
-                kryo.writeClassAndObject( output, obj.commands )
-
-            }
-
-            @Suppress("UNCHECKED_CAST")
-            override fun read(kryo: Kryo, input: Input, type: Class<WireTransaction>): WireTransaction {
-                var inputs = kryo.readClassAndObject( input ) as List<StateRef>
-                var attachments = kryo.readClassAndObject( input ) as List<SecureHash>
-
-                val attachmentStorage = kryo.attachmentStorage
-
-                // .filterNotNull in order for TwoPartyTradeProtocolTests.checkDependenciesOfSaleAssetAreResolved test to run
-                val classLoader = core.node.AttachmentsClassLoader.create( attachments.map { attachmentStorage?.openAttachment(it) }.filterNotNull() )
-
-                kryo.useClassLoader(classLoader) {
-                    var outputs = kryo.readClassAndObject(input) as List<ContractState>
-                    var commands = kryo.readClassAndObject(input) as List<Command>
-
-                    return WireTransaction(inputs, attachments, outputs, commands)
-                }
-            }
-        })
 
         // Some things where the JRE provides an efficient custom serialisation.
         val ser = JavaSerializer()

src/main/kotlin/core/node/services/NodeAttachmentService.kt

@@ -144,7 +144,7 @@ class NodeAttachmentService(val storePath: Path, val metrics: MetricRegistry) :
                 val cursor = stream.nextJarEntry ?: break
                 val entryPath = Paths.get(cursor.name)
                 // Security check to stop zips trying to escape their rightful place.
-                if (entryPath.isAbsolute || entryPath.normalize() != entryPath)
+                if (entryPath.isAbsolute || entryPath.normalize() != entryPath || '\\' in cursor.name)
                     throw IllegalArgumentException("Path is either absolute or non-normalised: $entryPath")
             }
         }

src/test/kotlin/core/node/AttachmentClassLoaderTests.kt

@@ -1,15 +1,11 @@
 package core.node
 
-import com.esotericsoftware.kryo.KryoException
 import contracts.DUMMY_PROGRAM_ID
 import contracts.DummyContract
 import core.*
 import core.crypto.SecureHash
 import core.node.services.AttachmentStorage
-import core.serialization.attachmentStorage
+import core.serialization.*
-import core.serialization.createKryo
-import core.serialization.deserialize
-import core.serialization.serialize
 import core.testutils.MEGA_CORP
 import org.apache.commons.io.IOUtils
 import org.junit.Test
@@ -28,8 +24,8 @@ interface DummyContractBackdoor {
     fun inspectState(state: ContractState): Int
 }
 
-class ClassLoaderTests {
+class AttachmentClassLoaderTests {
-    val ISOLATED_CONTRACTS_JAR_PATH = ClassLoaderTests::class.java.getResource("isolated.jar")
+    val ISOLATED_CONTRACTS_JAR_PATH = AttachmentClassLoaderTests::class.java.getResource("isolated.jar")
 
     fun importJar(storage: AttachmentStorage) = ISOLATED_CONTRACTS_JAR_PATH.openStream().use { storage.importAttachment(it) }
 
@@ -72,8 +68,8 @@ class ClassLoaderTests {
         val att1 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file.txt", "some data")))
         val att2 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file.txt", "some other data")))
 
-        assertFailsWith(OverlappingAttachments::class) {
+        assertFailsWith(AttachmentsClassLoader.OverlappingAttachments::class) {
-            AttachmentsClassLoader.create(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! })
+            AttachmentsClassLoader(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! })
         }
     }
 
@@ -85,11 +81,10 @@ class ClassLoaderTests {
         val att1 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file1.txt", "some data")))
         val att2 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file2.txt", "some other data")))
 
-        AttachmentsClassLoader.create(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! }).use {
+        val cl = AttachmentsClassLoader(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! })
-            val txt = IOUtils.toString(it.getResourceAsStream("file1.txt"))
+        val txt = IOUtils.toString(cl.getResourceAsStream("file1.txt"))
         assertEquals("some data", txt)
     }
-    }
 
     @Test
     fun `loading class AnotherDummyContract`() {
@@ -99,12 +94,11 @@ class ClassLoaderTests {
         val att1 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file1.txt", "some data")))
         val att2 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file2.txt", "some other data")))
 
-        AttachmentsClassLoader.create(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! }).use {
+        val cl = AttachmentsClassLoader(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! })
-            val contractClass = Class.forName("contracts.isolated.AnotherDummyContract", true, it)
+        val contractClass = Class.forName("contracts.isolated.AnotherDummyContract", true, cl)
         val contract = contractClass.newInstance() as Contract
         assertEquals(SecureHash.sha256("https://anotherdummy.org"), contract.legalContractReference)
     }
-    }
 
 
     /**
@@ -146,10 +140,10 @@ class ClassLoaderTests {
         val att1 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file1.txt", "some data")))
         val att2 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file2.txt", "some other data")))
 
-        val clsLoader = AttachmentsClassLoader.create(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! })
+        val cl = AttachmentsClassLoader(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! })
 
         val kryo = createKryo()
-        kryo.classLoader = clsLoader
+        kryo.classLoader = cl
 
         val state2 = bytes.deserialize(kryo, true)
 
@@ -173,10 +167,10 @@ class ClassLoaderTests {
         val att1 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file1.txt", "some data")))
         val att2 = storage.importAttachment(ByteArrayInputStream(fakeAttachment("file2.txt", "some other data")))
 
-        val clsLoader = AttachmentsClassLoader.create(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! })
+        val cl = AttachmentsClassLoader(arrayOf(att0, att1, att2).map { storage.openAttachment(it)!! })
 
         val kryo = createKryo()
-        kryo.classLoader = clsLoader
+        kryo.classLoader = cl
 
         val state2 = bytes.deserialize(kryo)
 
@@ -251,8 +245,9 @@ class ClassLoaderTests {
         // use empty attachmentStorage
         kryo2.attachmentStorage = MockAttachmentStorage()
 
-        assertFailsWith(KryoException::class) {
+        val e = assertFailsWith(MissingAttachmentsException::class) {
             bytes.deserialize(kryo2)
         }
+        assertEquals(attachmentRef, e.ids.single())
     }
 }