From 13246763486c52d5e01e00ba41895439040dbb48 Mon Sep 17 00:00:00 2001 From: Tudor Malene Date: Thu, 31 Jan 2019 09:47:59 +0000 Subject: [PATCH] CORDA-2536 - Tighten no overlap check. (#4690) (#4692) (cherry picked from commit 37eb93fa76459babce423940ee8e965cf5801633) --- .../internal/AttachmentsClassLoader.kt | 21 ++++++++---- .../AttachmentsClassLoaderTests.kt | 33 ++++++++++++++++++- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt b/core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt index 77e9c27ecc..cefe074fc9 100644 --- a/core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt +++ b/core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt @@ -50,12 +50,21 @@ class AttachmentsClassLoader(attachments: List, parent: ClassLoader private val ignoreDirectories = listOf("org/jolokia/", "org/json/simple/") private val ignorePackages = ignoreDirectories.map { it.replace("/", ".") } - private fun shouldCheckForNoOverlap(path: String, targetPlatformVersion: Int) = when { - path.endsWith("/") -> false // Directories (packages) can overlap. - targetPlatformVersion < 4 && ignoreDirectories.any { path.startsWith(it) } -> false // Ignore jolokia and json-simple for old cordapps. - path.endsWith(".class") -> true // All class files need to be unique. - !path.startsWith("meta-inf") -> true // All files outside of Meta-inf need to be unique. - else -> false // This allows overlaps over any non-class files in "Meta-inf". + // This function attempts to strike a balance between security and usability when it comes to the no-overlap rule. + // TODO - investigate potential exploits. + private fun shouldCheckForNoOverlap(path: String, targetPlatformVersion: Int): Boolean { + require(path.toLowerCase() == path) + require(!path.contains("\\")) + + return when { + path.endsWith("/") -> false // Directories (packages) can overlap. + targetPlatformVersion < 4 && ignoreDirectories.any { path.startsWith(it) } -> false // Ignore jolokia and json-simple for old cordapps. + path.endsWith(".class") -> true // All class files need to be unique. + !path.startsWith("meta-inf") -> true // All files outside of META-INF need to be unique. + (path == "meta-inf/services/net.corda.core.serialization.serializationwhitelist") -> false // Allow overlapping on the SerializationWhitelist. + path.startsWith("meta-inf/services") -> true // Services can't overlap to prevent a malicious party from injecting additional implementations of an interface used by a contract. + else -> false // This allows overlaps over any non-class files in "META-INF" - except 'services'. + } } private fun requireNoDuplicates(attachments: List) { diff --git a/core/src/test/kotlin/net/corda/core/transactions/AttachmentsClassLoaderTests.kt b/core/src/test/kotlin/net/corda/core/transactions/AttachmentsClassLoaderTests.kt index 81d031689e..f6e2a62fc6 100644 --- a/core/src/test/kotlin/net/corda/core/transactions/AttachmentsClassLoaderTests.kt +++ b/core/src/test/kotlin/net/corda/core/transactions/AttachmentsClassLoaderTests.kt @@ -14,6 +14,7 @@ import net.corda.testing.services.MockAttachmentStorage import org.apache.commons.io.IOUtils import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals +import org.junit.Ignore import org.junit.Test import java.io.ByteArrayOutputStream import java.io.InputStream @@ -116,6 +117,36 @@ class AttachmentsClassLoaderTests { } } + @Test + fun `Overlapping rules for META-INF serializationwhitelist files`() { + val att1 = importAttachment(fakeAttachment("meta-inf/services/net.corda.core.serialization.serializationwhitelist", "some data").inputStream(), "app", "file1.jar") + val att2 = importAttachment(fakeAttachment("meta-inf/services/net.corda.core.serialization.serializationwhitelist", "some other data").inputStream(), "app", "file2.jar") + + AttachmentsClassLoader(arrayOf(att1, att2).map { storage.openAttachment(it)!! }) + } + + @Ignore("Enable once `requireNoDuplicates` is fixed. The check is currently skipped due to the incorrect logic.") + @Test + fun `Overlapping rules for META-INF random service files`() { + val att1 = importAttachment(fakeAttachment("meta-inf/services/com.example.something", "some data").inputStream(), "app", "file1.jar") + val att2 = importAttachment(fakeAttachment("meta-inf/services/com.example.something", "some other data").inputStream(), "app", "file2.jar") + + assertFailsWith(TransactionVerificationException.OverlappingAttachmentsException::class) { + AttachmentsClassLoader(arrayOf(att1, att2).map { storage.openAttachment(it)!! }) + } + } + + @Ignore("Added test that was removed when the hash-2-signature constraint was added. Enable once `requireNoDuplicates` is fixed.") + @Test + fun `Test overlapping file exception`() { + val att1 = storage.importAttachment(fakeAttachment("file1.txt", "some data").inputStream(), "app", "file1.jar") + val att2 = storage.importAttachment(fakeAttachment("file1.txt", "some other data").inputStream(), "app", "file2.jar") + + assertFailsWith(TransactionVerificationException.OverlappingAttachmentsException::class) { + AttachmentsClassLoader(arrayOf(att1, att2).map { storage.openAttachment(it)!! }) + } + } + @Test fun `Check platform independent path handling in attachment jars`() { val att1 = importAttachment(fakeAttachment("/folder1/foldera/file1.txt", "some data").inputStream(), "app", "file1.jar") @@ -133,7 +164,7 @@ class AttachmentsClassLoaderTests { val data2b = readAttachment(storage.openAttachment(att2)!!, "/folder1/folderb/file2.txt") assertArrayEquals("some other data".toByteArray(), data2b) } - + private fun importAttachment(jar: InputStream, uploader: String, filename: String?): AttachmentId { return jar.use { storage.importAttachment(jar, uploader, filename) } }