ENT-3141 - Improve jar verification. (#4841)

ENT-3141 Address code review comments

core/src/main/kotlin/net/corda/core/internal/JarSignatureCollector.kt

@@ -19,6 +19,11 @@ object JarSignatureCollector {
      */
     private val unsignableEntryName = "META-INF/(?:(?:.*[.](?:SF|DSA|RSA|EC)|SIG-.*)|INDEX\\.LIST)".toRegex()
 
+    /**
+     * @return if the [entry] [JarEntry] can be signed.
+     */
+    fun isNotSignable(entry: JarEntry): Boolean = entry.isDirectory || unsignableEntryName.matches(entry.name)
+
     /**
      * Returns an ordered list of every [PublicKey] which has signed every signable item in the given [JarInputStream].
      *
@@ -57,8 +62,7 @@ object JarSignatureCollector {
     private val JarInputStream.fileSignerSets: List<Pair<String, Set<CodeSigner>>> get() =
             entries.thatAreSignable.shreddedFrom(this).toFileSignerSet().toList()
 
-    private val Sequence<JarEntry>.thatAreSignable: Sequence<JarEntry> get() =
+    private val Sequence<JarEntry>.thatAreSignable: Sequence<JarEntry> get() = filterNot { isNotSignable(it) }
-            filterNot { entry -> entry.isDirectory || unsignableEntryName.matches(entry.name) }
 
     private fun Sequence<JarEntry>.shreddedFrom(jar: JarInputStream): Sequence<JarEntry> = map { entry ->
         val shredder = ByteArray(1024) // can't share or re-use this, as it's used to compute CRCs during shredding

node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt

@@ -41,6 +41,7 @@ import java.nio.file.Paths
 import java.security.PublicKey
 import java.time.Instant
 import java.util.*
+import java.util.jar.JarEntry
 import java.util.jar.JarInputStream
 import javax.annotation.concurrent.ThreadSafe
 import javax.persistence.*
@@ -71,11 +72,24 @@ class NodeAttachmentService(
         // Note that JarInputStream won't throw any kind of error at all if the file stream is in fact not
         // a ZIP! It'll just pretend it's an empty archive, which is kind of stupid but that's how it works.
         // So we have to check to ensure we found at least one item.
-        private fun checkIsAValidJAR(stream: InputStream) {
+        //
+        // For signed Jars add additional checks to close security holes left by the default jarSigner verifier:
+        //  - All entries listed in the Manifest are in the JAR file.
+        //  - No extra files in the JAR that were not listed in the Manifest.
+        // Together with the check that all entries need to be signed by the same signers that is performed when the signers are read,
+        // it should close any possibility of foul play.
+        internal fun checkIsAValidJAR(stream: InputStream) {
             val jar = JarInputStream(stream, true)
             var count = 0
+
+            // Can be null for not-signed JARs.
+            val allManifestEntries = jar.manifest?.entries?.keys?.toMutableList()
+            val extraFilesNotFoundInEntries = mutableListOf<JarEntry>()
+            val manifestHasEntries= allManifestEntries != null && allManifestEntries.isNotEmpty()
+
             while (true) {
                 val cursor = jar.nextJarEntry ?: break
+                if (manifestHasEntries && !allManifestEntries!!.remove(cursor.name)) extraFilesNotFoundInEntries.add(cursor)
                 val entryPath = Paths.get(cursor.name)
                 // Security check to stop zips trying to escape their rightful place.
                 require(!entryPath.isAbsolute) { "Path $entryPath is absolute" }
@@ -83,6 +97,17 @@ class NodeAttachmentService(
                 require(!('\\' in cursor.name || cursor.name == "." || cursor.name == "..")) { "Bad character in $entryPath" }
                 count++
             }
+
+            // Only perform these checks if the JAR was signed.
+            if (manifestHasEntries) {
+                if (allManifestEntries!!.size > 0) {
+                    throw SecurityException("Signed jar has been tampered with. Files ${allManifestEntries} have been removed.")
+                }
+                val extraSignableFiles = extraFilesNotFoundInEntries.filterNot { JarSignatureCollector.isNotSignable(it) }
+                if (extraSignableFiles.size > 0) {
+                    throw SecurityException("Signed jar has been tampered with. Files ${extraSignableFiles} have been added to the JAR.")
+                }
+            }
             require(count > 0) { "Stream is either empty or not a JAR/ZIP" }
         }
     }
@@ -311,7 +336,7 @@ class NodeAttachmentService(
         currentDBSession().find(NodeAttachmentService.DBAttachment::class.java, attachmentId.toString()) != null
     }
 
-    private fun verifyVersionUniquenessForSignedAttachments(contractClassNames: List<ContractClassName>, contractVersion: Int, signers: List<PublicKey>?){
+    private fun verifyVersionUniquenessForSignedAttachments(contractClassNames: List<ContractClassName>, contractVersion: Int, signers: List<PublicKey>?) {
         if (signers != null && signers.isNotEmpty()) {
             contractClassNames.forEach {
                 val existingContractsImplementations = queryAttachments(AttachmentQueryCriteria.AttachmentsQueryCriteria(
@@ -327,9 +352,10 @@ class NodeAttachmentService(
         }
     }
 
-    private fun increaseDefaultVersionIfWhitelistedAttachment(contractClassNames: List<ContractClassName>, contractVersionFromFile: Int, attachmentId : AttachmentId) =
+    private fun increaseDefaultVersionIfWhitelistedAttachment(contractClassNames: List<ContractClassName>, contractVersionFromFile: Int, attachmentId: AttachmentId) =
             if (contractVersionFromFile == DEFAULT_CORDAPP_VERSION) {
-            val versions = contractClassNames.mapNotNull { servicesForResolution.networkParameters.whitelistedContractImplementations[it]?.indexOf(attachmentId) }.filter { it >= 0 }.map { it + 1 } // +1 as versions starts from 1 not 0
+                val versions = contractClassNames.mapNotNull { servicesForResolution.networkParameters.whitelistedContractImplementations[it]?.indexOf(attachmentId) }
+                        .filter { it >= 0 }.map { it + 1 } // +1 as versions starts from 1 not 0
                 val max = versions.max()
                 if (max != null && max > contractVersionFromFile) {
                     val msg = "Updating version of attachment $attachmentId from '$contractVersionFromFile' to '$max'"
@@ -339,8 +365,7 @@ class NodeAttachmentService(
                         log.debug(msg)
                     max
                 } else contractVersionFromFile
-        }
+            } else contractVersionFromFile
-        else contractVersionFromFile
 
     // TODO: PLT-147: The attachment should be randomised to prevent brute force guessing and thus privacy leaks.
     private fun import(jar: InputStream, uploader: String?, filename: String?): AttachmentId {
@@ -495,7 +520,7 @@ class NodeAttachmentService(
     private fun makeAttachmentIds(it: Map.Entry<Int, List<DBAttachment>>, contractClassName: String): Pair<Version, AttachmentIds> {
         val signed = it.value.filter { it.signers?.isNotEmpty() ?: false }.map { AttachmentId.parse(it.attId) }
         if (!devMode)
-            check (signed.size <= 1) //sanity check
+            check(signed.size <= 1) //sanity check
         else
             log.warn("(Dev Mode) Multiple signed attachments ${signed.map { it.toString() }} for contract $contractClassName version '${it.key}'.")
         val unsigned = it.value.filter { it.signers?.isEmpty() ?: true }.map { AttachmentId.parse(it.attId) }

node/src/test/kotlin/net/corda/node/services/persistence/NodeAttachmentServiceTest.kt

@@ -40,11 +40,14 @@ import org.junit.Before
 import org.junit.Ignore
 import org.junit.Test
 import java.io.ByteArrayOutputStream
+import java.io.InputStream
+import java.net.URL
 import java.nio.charset.StandardCharsets
 import java.nio.file.FileAlreadyExistsException
 import java.nio.file.FileSystem
 import java.nio.file.Path
 import java.util.*
+import java.util.jar.JarInputStream
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
 import kotlin.test.assertNotEquals
@@ -722,6 +725,52 @@ class NodeAttachmentServiceTest {
         }
     }
 
+    @Test
+    fun `The strict JAR verification function fails signed JARs with removed or extra files that are valid according to the usual jarsigner`() {
+
+        // Signed jar that has a modified file.
+        val changedFileJAR = this::class.java.getResource("/changed-file-signed-jar.jar")
+
+        // Signed jar with removed files.
+        val removedFilesJAR = this::class.java.getResource("/removed-files-signed-jar.jar")
+
+        // Signed jar with extra files.
+        val extraFilesJAR = this::class.java.getResource("/extra-files-signed-jar.jar")
+
+        // Valid signed jar with all files.
+        val legalJAR = this::class.java.getResource("/legal-signed-jar.jar")
+
+        fun URL.standardVerifyJar() = JarInputStream(this.openStream(), true).use { jar ->
+            while (true) {
+                jar.nextJarEntry ?: break
+            }
+        }
+
+        // A compliant signed JAR will pass both the standard and the improved validation.
+        legalJAR.standardVerifyJar()
+        NodeAttachmentService.checkIsAValidJAR(legalJAR.openStream())
+
+        // Signed JAR with removed files passes the non-strict check but fails the strict check.
+        removedFilesJAR.standardVerifyJar()
+        assertFailsWith(SecurityException::class) {
+            NodeAttachmentService.checkIsAValidJAR(removedFilesJAR.openStream())
+        }
+
+        // Signed JAR with a changed file fails both the usual and the strict test.
+        assertFailsWith(SecurityException::class) {
+            changedFileJAR.standardVerifyJar()
+        }
+        assertFailsWith(SecurityException::class) {
+            NodeAttachmentService.checkIsAValidJAR(changedFileJAR.openStream())
+        }
+
+        // Signed JAR with an extra file passes the usual but fails the strict test.
+        extraFilesJAR.standardVerifyJar()
+        assertFailsWith(SecurityException::class) {
+            NodeAttachmentService.checkIsAValidJAR(extraFilesJAR.openStream())
+        }
+    }
+
     // Not the real FetchAttachmentsFlow!
     private class FetchAttachmentsFlow : FlowLogic<Unit>() {
         @Suspendable