Tighten no overlap check. (#4690)

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

@@ -50,12 +50,21 @@ class AttachmentsClassLoader(attachments: List<Attachment>, 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 {
+        // This function attempts to strike a balance between security and usability when it comes to the no-overlap rule.
-            path.endsWith("/") -> false                     // Directories (packages) can overlap.
+        // TODO - investigate potential exploits.
-            targetPlatformVersion < 4 && ignoreDirectories.any { path.startsWith(it) } -> false    // Ignore jolokia and json-simple for old cordapps.
+        private fun shouldCheckForNoOverlap(path: String, targetPlatformVersion: Int): Boolean {
-            path.endsWith(".class") -> true                 // All class files need to be unique.
+            require(path.toLowerCase() == path)
-            !path.startsWith("meta-inf") -> true            // All files outside of Meta-inf need to be unique.
+            require(!path.contains("\\"))
-            else -> false                                          // This allows overlaps over any non-class files in "Meta-inf".
+
+            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<Attachment>) {

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) }
     }