ExternalVendorCode/corda
1
0
Fork 0
mirror of https://github.com/corda/corda.git synced 2024-12-19 04:57:58 +00:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

CORDA-2540 Adjust no overlap rule to avoid premature optimization (#4693)

Browse Source 
Fix invalid optimization in AttachmentsClassLoader.
This commit is contained in:
Mike Hearn 2019-01-31 17:59:01 +01:00 committed by GitHub
parent 099a747ebf
commit 0632d275ed
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 67 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
core/src/main/kotlin/net/corda/core/serialization/internal/AttachmentsClassLoader.kt
View File

@ -68,15 +68,35 @@ class AttachmentsClassLoader(attachments: List<Attachment>, parent: ClassLoader
}
private fun requireNoDuplicates(attachments: List<Attachment>) {
// 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<String, Attachment>()
for (attachment in attachments) {
attachment.openAsJAR().use { jar ->
@ -91,26 +111,24 @@ class AttachmentsClassLoader(attachments: List<Attachment>, 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" }

15
core/src/test/kotlin/net/corda/core/transactions/AttachmentsClassLoaderTests.kt
View File

@ -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")

12
core/src/test/kotlin/net/corda/core/transactions/TransactionTests.kt
View File

@ -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<CommandWithParties<CommandData>>()
val attachments = emptyList<Attachment>()
val attachments = listOf(object : Attachment {
override fun open(): InputStream = AttachmentsClassLoaderTests::class.java.getResource("isolated-4.0.jar").openStream()
@Suppress("OverridingDeprecatedMember")
override val signers: List<Party> = emptyList()
override val signerKeys: List<PublicKey> = emptyList()
override val size: Int = 1234
override val id: SecureHash = SecureHash.zeroHash
})
val id = SecureHash.randomSHA256()
val timeWindow: TimeWindow? = null
val privacySalt = PrivacySalt()