From 0632d275ed3f29ad72f5823051f20316c977cce5 Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Thu, 31 Jan 2019 17:59:01 +0100 Subject: [PATCH] CORDA-2540 Adjust no overlap rule to avoid premature optimization (#4693) Fix invalid optimization in AttachmentsClassLoader. --- .../internal/AttachmentsClassLoader.kt | 68 ++++++++++++------- .../AttachmentsClassLoaderTests.kt | 15 +++- .../core/transactions/TransactionTests.kt | 12 +++- 3 files changed, 67 insertions(+), 28 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 cefe074fc9..8a670ca061 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 @@ -68,15 +68,35 @@ class AttachmentsClassLoader(attachments: List, parent: ClassLoader } private fun requireNoDuplicates(attachments: List) { - // Avoid unnecessary duplicate checking if possible: - // 1. single attachment. - // 2. multiple attachments with non-overlapping contract classes. - if (attachments.size <= 1) return - val overlappingContractClasses = attachments.mapNotNull { it as? ContractAttachment }.flatMap { it.allContracts }.groupingBy { it }.eachCount().filter { it.value > 1 } - if (overlappingContractClasses.isEmpty()) return + require(attachments.isNotEmpty()) { "attachments list is empty" } + if (attachments.size == 1) return + + // Here is where we enforce the no-overlap rule. This rule states that a transaction which has multiple + // attachments defining different files for the same file path is invalid. It's an important part of the + // security model and blocks various sorts of attacks. + // + // Consider the case of a transaction with two attachments, A and B. Attachment B satisfies the constraint + // on the transaction's states, and thus should be bound by the logic imposed by the contract logic in that + // attachment. But if attachment A were to supply a different class file with the same file name, then the + // usual Java classpath semantics would apply and it'd end up being contract A that gets executed, not B. + // This would prevent you from reasoning about the semantics and transitional logic applied to a state; in + // effect the ledger would be open to arbitrary malicious changes. + // + // There are several variants of this attack that mean we must enforce the no-overlap rule on every file. + // For instance the attacking attachment may override an inner class of the contract class, or a dependency. + // + // We hash each file and ignore overlaps where the contents are actually identical. This is to simplify + // migration from hash to signature constraints. In such a migration transaction the same JAR may be + // attached twice, one signed and one unsigned. The signature files are ignored for the purposes of + // overlap checking as they are expected to have similar names and don't affect the semantics of the + // code, and the class files will be identical so that also doesn't affect lookup. Thus both constraints + // can be satisfied with different attachments that are actually behaviourally identical. + // + // It also avoids a problem where the same dependency has been fat-jarred into multiple apps. This can + // happen because we don't have (as of writing, Feb 2019) any infrastructure for tracking or managing + // dependencies between attachments, so, dependent libraries get bundled up together. Detecting duplicates + // avoids accidental triggering of the no-overlap rule in benign circumstances. - // this logic executes only if there are overlapping contract classes - log.debug("Duplicate contract class checking for $overlappingContractClasses") val classLoaderEntries = mutableMapOf() for (attachment in attachments) { attachment.openAsJAR().use { jar -> @@ -91,26 +111,24 @@ class AttachmentsClassLoader(attachments: List, parent: ClassLoader // filesystem tries to be case insensitive. This may break developers who attempt to use ProGuard. // // Also convert to Unix path separators as all resource/class lookups will expect this. - // val path = entry.name.toLowerCase().replace('\\', '/') - // TODO - If 2 entries are identical, it means the same file is present in both attachments, so that should be ok. - if (shouldCheckForNoOverlap(path, targetPlatformVersion)) { - if (path in classLoaderEntries.keys) { - // If 2 entries have the same content hash, it means the same file is present in both attachments, so that is ok. - val contentHash = readAttachment(attachment, path).sha256() - val originalAttachment = classLoaderEntries[path]!! - val originalContentHash = readAttachment(originalAttachment, path).sha256() - if (contentHash == originalContentHash) { - log.debug { "Duplicate entry $path has same content hash $contentHash" } - continue - } else { - log.debug { "Content hash differs for $path" } - throw OverlappingAttachmentsException(path) - } + // Some files don't need overlap checking because they don't affect the way the code runs. + if (!shouldCheckForNoOverlap(path, targetPlatformVersion)) continue + // If 2 entries have the same content hash, it means the same file is present in both attachments, so that is ok. + if (path in classLoaderEntries.keys) { + val contentHash = readAttachment(attachment, path).sha256() + val originalAttachment = classLoaderEntries[path]!! + val originalContentHash = readAttachment(originalAttachment, path).sha256() + if (contentHash == originalContentHash) { + log.debug { "Duplicate entry $path has same content hash $contentHash" } + continue + } else { + log.debug { "Content hash differs for $path" } + throw OverlappingAttachmentsException(path) } - log.debug { "Adding new entry for $path" } - classLoaderEntries[path] = attachment } + log.debug { "Adding new entry for $path" } + classLoaderEntries[path] = attachment } } log.debug { "${classLoaderEntries.size} classloaded entries for $attachment" } 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 f6e2a62fc6..780ed14aad 100644 --- a/core/src/test/kotlin/net/corda/core/transactions/AttachmentsClassLoaderTests.kt +++ b/core/src/test/kotlin/net/corda/core/transactions/AttachmentsClassLoaderTests.kt @@ -17,8 +17,10 @@ import org.junit.Assert.assertEquals import org.junit.Ignore import org.junit.Test import java.io.ByteArrayOutputStream +import java.io.File import java.io.InputStream import java.net.URL +import java.nio.file.Paths import kotlin.test.assertFailsWith class AttachmentsClassLoaderTests { @@ -125,7 +127,6 @@ class AttachmentsClassLoaderTests { 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") @@ -136,7 +137,6 @@ class AttachmentsClassLoaderTests { } } - @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") @@ -147,6 +147,17 @@ class AttachmentsClassLoaderTests { } } + @Test + fun `partial overlaps not possible`() { + // Cover a previous bug whereby overlap checking had been optimized to only check contract classes, which isn't + // a valid optimization as code used by the contract class could then be overlapped. + val att1 = importAttachment(ISOLATED_CONTRACTS_JAR_PATH.openStream(), "app", ISOLATED_CONTRACTS_JAR_PATH.file) + val att2 = importAttachment(fakeAttachment("net/corda/finance/contracts/isolated/AnotherDummyContract\$State.class", "some attackdata").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") diff --git a/core/src/test/kotlin/net/corda/core/transactions/TransactionTests.kt b/core/src/test/kotlin/net/corda/core/transactions/TransactionTests.kt index 591f456737..581bc1ae79 100644 --- a/core/src/test/kotlin/net/corda/core/transactions/TransactionTests.kt +++ b/core/src/test/kotlin/net/corda/core/transactions/TransactionTests.kt @@ -13,10 +13,13 @@ import net.corda.testing.core.* import net.corda.testing.internal.createWireTransaction import net.corda.testing.internal.fakeAttachment import net.corda.testing.internal.rigorousMock +import net.corda.testing.services.MockAttachmentStorage import org.junit.Rule import org.junit.Test +import java.io.InputStream import java.math.BigInteger import java.security.KeyPair +import java.security.PublicKey import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNotEquals @@ -166,7 +169,14 @@ class TransactionTests { val inputs = listOf(StateAndRef(inState, StateRef(SecureHash.randomSHA256(), 0))) val outputs = listOf(outState) val commands = emptyList>() - val attachments = emptyList() + val attachments = listOf(object : Attachment { + override fun open(): InputStream = AttachmentsClassLoaderTests::class.java.getResource("isolated-4.0.jar").openStream() + @Suppress("OverridingDeprecatedMember") + override val signers: List = emptyList() + override val signerKeys: List = emptyList() + override val size: Int = 1234 + override val id: SecureHash = SecureHash.zeroHash + }) val id = SecureHash.randomSHA256() val timeWindow: TimeWindow? = null val privacySalt = PrivacySalt()