From dd7852f2b8438b769117cd78443892ebcd5d1c92 Mon Sep 17 00:00:00 2001 From: Chris Rankin Date: Thu, 19 Mar 2020 11:20:53 +0000 Subject: [PATCH] CORDA-3668: Prevent AttachmentURLConnection from assigning ALL_PERMISSIONS. (#6080) * CORDA-3668: Prevent AttachmentURLConnection from assigning ALL_PERMISSIONS to classes inside an attachment. * Strengthen the comment warning about AttachmentURLConnection.getPermission. --- .../AttachmentsClassLoaderTests.kt | 89 ++++++++++++------- .../internal/AttachmentsClassLoader.kt | 10 +++ 2 files changed, 65 insertions(+), 34 deletions(-) diff --git a/core-tests/src/test/kotlin/net/corda/coretests/transactions/AttachmentsClassLoaderTests.kt b/core-tests/src/test/kotlin/net/corda/coretests/transactions/AttachmentsClassLoaderTests.kt index 29edf8938e..6a98b20172 100644 --- a/core-tests/src/test/kotlin/net/corda/coretests/transactions/AttachmentsClassLoaderTests.kt +++ b/core-tests/src/test/kotlin/net/corda/coretests/transactions/AttachmentsClassLoaderTests.kt @@ -21,14 +21,18 @@ import net.corda.testing.internal.services.InternalMockAttachmentStorage import net.corda.testing.node.internal.FINANCE_CONTRACTS_CORDAPP import net.corda.testing.services.MockAttachmentStorage import org.apache.commons.io.IOUtils +import org.assertj.core.api.Assertions.assertThat import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import java.io.ByteArrayOutputStream import java.io.InputStream import java.net.URL import kotlin.test.assertFailsWith +import kotlin.test.fail class AttachmentsClassLoaderTests { companion object { @@ -84,14 +88,29 @@ class AttachmentsClassLoaderTests { } } + @Test(timeout=300_000) + fun `test contracts have no permissions for protection domain`() { + val isolatedId = importAttachment(ISOLATED_CONTRACTS_JAR_PATH.openStream(), "app", "isolated.jar") + assertNull(System.getSecurityManager()) + + createClassloader(isolatedId).use { classLoader -> + val contractClass = Class.forName(ISOLATED_CONTRACT_CLASS_NAME, true, classLoader) + val protectionDomain = contractClass.protectionDomain ?: fail("Protection Domain missing") + val permissions = protectionDomain.permissions ?: fail("Protection domain has no permissions") + assertThat(permissions.elements().toList()).isEmpty() + assertTrue(permissions.isReadOnly) + } + } + @Test(timeout=300_000) fun `Dynamically load AnotherDummyContract from isolated contracts jar using the AttachmentsClassLoader`() { val isolatedId = importAttachment(ISOLATED_CONTRACTS_JAR_PATH.openStream(), "app", "isolated.jar") - val classloader = createClassloader(isolatedId) - val contractClass = Class.forName(ISOLATED_CONTRACT_CLASS_NAME, true, classloader) - val contract = contractClass.getDeclaredConstructor().newInstance() as Contract - assertEquals("helloworld", contract.declaredField("magicString").value) + createClassloader(isolatedId).use { classloader -> + val contractClass = Class.forName(ISOLATED_CONTRACT_CLASS_NAME, true, classloader) + val contract = contractClass.getDeclaredConstructor().newInstance() as Contract + assertEquals("helloworld", contract.declaredField("magicString").value) + } } @Test(timeout=300_000) @@ -100,7 +119,7 @@ class AttachmentsClassLoaderTests { val att2 = importAttachment(ISOLATED_CONTRACTS_JAR_PATH_V4.openStream(), "app", "isolated-4.0.jar") assertFailsWith(TransactionVerificationException.OverlappingAttachmentsException::class) { - createClassloader(listOf(att1, att2)) + createClassloader(listOf(att1, att2)).use {} } } @@ -111,7 +130,7 @@ class AttachmentsClassLoaderTests { val isolatedSignedId = importAttachment(signedJar.first.toUri().toURL().openStream(), "app", "isolated-signed.jar") // does not throw OverlappingAttachments exception - createClassloader(listOf(isolatedId, isolatedSignedId)) + createClassloader(listOf(isolatedId, isolatedSignedId)).use {} } @Test(timeout=300_000) @@ -120,7 +139,7 @@ class AttachmentsClassLoaderTests { val att2 = importAttachment(FINANCE_CONTRACTS_CORDAPP.jarFile.inputStream(), "app", "finance.jar") // does not throw OverlappingAttachments exception - createClassloader(listOf(att1, att2)) + createClassloader(listOf(att1, att2)).use {} } @Test(timeout=300_000) @@ -128,12 +147,13 @@ class AttachmentsClassLoaderTests { val att1 = importAttachment(fakeAttachment("file1.txt", "some data").inputStream(), "app", "file1.jar") val att2 = importAttachment(fakeAttachment("file2.txt", "some other data").inputStream(), "app", "file2.jar") - val cl = createClassloader(listOf(att1, att2)) - val txt = IOUtils.toString(cl.getResourceAsStream("file1.txt"), Charsets.UTF_8.name()) - assertEquals("some data", txt) + createClassloader(listOf(att1, att2)).use { cl -> + val txt = IOUtils.toString(cl.getResourceAsStream("file1.txt"), Charsets.UTF_8.name()) + assertEquals("some data", txt) - val txt1 = IOUtils.toString(cl.getResourceAsStream("file2.txt"), Charsets.UTF_8.name()) - assertEquals("some other data", txt1) + val txt1 = IOUtils.toString(cl.getResourceAsStream("file2.txt"), Charsets.UTF_8.name()) + assertEquals("some other data", txt1) + } } @Test(timeout=300_000) @@ -141,9 +161,10 @@ class AttachmentsClassLoaderTests { val att1 = importAttachment(fakeAttachment("file1.txt", "same data", "file2.txt", "same other data").inputStream(), "app", "file1.jar") val att2 = importAttachment(fakeAttachment("file1.txt", "same data", "file3.txt", "same totally different").inputStream(), "app", "file2.jar") - val cl = createClassloader(listOf(att1, att2)) - val txt = IOUtils.toString(cl.getResourceAsStream("file1.txt"), Charsets.UTF_8.name()) - assertEquals("same data", txt) + createClassloader(listOf(att1, att2)).use { cl -> + val txt = IOUtils.toString(cl.getResourceAsStream("file1.txt"), Charsets.UTF_8.name()) + assertEquals("same data", txt) + } } @Test(timeout=300_000) @@ -152,7 +173,7 @@ class AttachmentsClassLoaderTests { val att1 = importAttachment(fakeAttachment(path, "some data").inputStream(), "app", "file1.jar") val att2 = importAttachment(fakeAttachment(path, "some other data").inputStream(), "app", "file2.jar") - createClassloader(listOf(att1, att2)) + createClassloader(listOf(att1, att2)).use {} } } @@ -161,7 +182,7 @@ class AttachmentsClassLoaderTests { 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") - createClassloader(listOf(att1, att2)) + createClassloader(listOf(att1, att2)).use {} } @Test(timeout=300_000) @@ -170,7 +191,7 @@ class AttachmentsClassLoaderTests { val att2 = importAttachment(fakeAttachment("meta-inf/services/com.example.something", "some other data").inputStream(), "app", "file2.jar") assertFailsWith(TransactionVerificationException.OverlappingAttachmentsException::class) { - createClassloader(listOf(att1, att2)) + createClassloader(listOf(att1, att2)).use {} } } @@ -180,7 +201,7 @@ class AttachmentsClassLoaderTests { val att2 = storage.importAttachment(fakeAttachment("file1.txt", "some other data").inputStream(), "app", "file2.jar") assertFailsWith(TransactionVerificationException.OverlappingAttachmentsException::class) { - createClassloader(listOf(att1, att2)) + createClassloader(listOf(att1, att2)).use {} } } @@ -191,7 +212,7 @@ class AttachmentsClassLoaderTests { 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) { - createClassloader(listOf(att1, att2)) + createClassloader(listOf(att1, att2)).use {} } } @@ -220,10 +241,10 @@ class AttachmentsClassLoaderTests { val untrustedClassJar = importAttachment(fakeAttachment("/com/example/something/MaliciousClass.class", "some malicious data").inputStream(), "untrusted", "file2.jar") val trustedClassJar = importAttachment(fakeAttachment("/com/example/something/VirtuousClass.class", "some other data").inputStream(), "app", "file3.jar") - createClassloader(listOf(trustedResourceJar, untrustedResourceJar, trustedClassJar)) - - assertFailsWith(TransactionVerificationException.UntrustedAttachmentsException::class) { - createClassloader(listOf(trustedResourceJar, untrustedResourceJar, trustedClassJar, untrustedClassJar)) + createClassloader(listOf(trustedResourceJar, untrustedResourceJar, trustedClassJar)).use { + assertFailsWith(TransactionVerificationException.UntrustedAttachmentsException::class) { + createClassloader(listOf(trustedResourceJar, untrustedResourceJar, trustedClassJar, untrustedClassJar)).use {} + } } } @@ -257,7 +278,7 @@ class AttachmentsClassLoaderTests { signers = listOf(keyPairA.public, keyPairB.public) ) - createClassloader(untrustedAttachment) + createClassloader(untrustedAttachment).use {} } @Test(timeout=300_000) @@ -287,7 +308,7 @@ class AttachmentsClassLoaderTests { signers = listOf(keyPairA.public, keyPairB.public) ) - createClassloader(untrustedAttachment) + createClassloader(untrustedAttachment).use {} } @Test(timeout=300_000) @@ -306,7 +327,7 @@ class AttachmentsClassLoaderTests { ) assertFailsWith(TransactionVerificationException.UntrustedAttachmentsException::class) { - createClassloader(untrustedAttachment) + createClassloader(untrustedAttachment).use {} } } @@ -337,7 +358,7 @@ class AttachmentsClassLoaderTests { ) assertFailsWith(TransactionVerificationException.UntrustedAttachmentsException::class) { - createClassloader(untrustedAttachment) + createClassloader(untrustedAttachment).use {} } } @@ -380,10 +401,10 @@ class AttachmentsClassLoaderTests { ) // pass the inherited trust attachment through the classloader first to ensure it does not affect the next loaded attachment - createClassloader(inheritedTrustAttachment) - - assertFailsWith(TransactionVerificationException.UntrustedAttachmentsException::class) { - createClassloader(untrustedAttachment) + createClassloader(inheritedTrustAttachment).use { + assertFailsWith(TransactionVerificationException.UntrustedAttachmentsException::class) { + createClassloader(untrustedAttachment).use {} + } } } @@ -421,7 +442,7 @@ class AttachmentsClassLoaderTests { ) assertFailsWith(TransactionVerificationException.UntrustedAttachmentsException::class) { - createClassloader(untrustedAttachment) + createClassloader(untrustedAttachment).use {} } } @@ -446,6 +467,6 @@ class AttachmentsClassLoaderTests { signers = listOf(keyPairA.public) ) - createClassloader(trustedAttachment) + createClassloader(trustedAttachment).use {} } } 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 e195a46064..5188ae1eff 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 @@ -18,6 +18,7 @@ import java.io.ByteArrayOutputStream import java.io.IOException import java.io.InputStream import java.net.* +import java.security.Permission import java.util.* /** @@ -378,6 +379,15 @@ object AttachmentURLStreamHandlerFactory : URLStreamHandlerFactory { private class AttachmentURLConnection(url: URL, private val attachment: Attachment) : URLConnection(url) { override fun getContentLengthLong(): Long = attachment.size.toLong() override fun getInputStream(): InputStream = attachment.open() + /** + * Define the permissions that [AttachmentsClassLoader] will need to + * use this [URL]. The attachment is stored in memory, and so we + * don't need any extra permissions here. But if we don't override + * [getPermission] then [AttachmentsClassLoader] will assign the + * default permission of ALL_PERMISSION to these classes' + * [java.security.ProtectionDomain]. This would be a security hole! + */ + override fun getPermission(): Permission? = null override fun connect() { connected = true }