From 347224c90004b89cb2c403cb96610f3567fd6ede Mon Sep 17 00:00:00 2001
From: Mike Hearn <mike@r3cev.com>
Date: Mon, 20 Mar 2017 19:06:07 +0100
Subject: [PATCH] Improve attachment checks to ensure that a stream is actually
 a JAR/ZIP.

---
 .../services/persistence/NodeAttachmentService.kt   | 13 +++++++++++--
 .../node/services/NodeAttachmentStorageTest.kt      | 10 +++++++++-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt b/node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt
index 2b479de736..f7f2b998c1 100644
--- a/node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt
+++ b/node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt
@@ -21,7 +21,9 @@ import net.corda.node.services.persistence.schemas.Models
 import java.io.ByteArrayInputStream
 import java.io.FilterInputStream
 import java.io.InputStream
-import java.nio.file.*
+import java.nio.file.FileAlreadyExistsException
+import java.nio.file.Path
+import java.nio.file.Paths
 import java.util.*
 import java.util.jar.JarInputStream
 import javax.annotation.concurrent.ThreadSafe
@@ -159,14 +161,21 @@ class NodeAttachmentService(override var storePath: Path, dataSourceProperties:
 
     private fun checkIsAValidJAR(stream: InputStream) {
         // Just iterate over the entries with verification enabled: should be good enough to catch mistakes.
+        // 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.
         val jar = JarInputStream(stream)
+        var count = 0
         while (true) {
             val cursor = jar.nextJarEntry ?: break
             val entryPath = Paths.get(cursor.name)
             // Security check to stop zips trying to escape their rightful place.
-            if (entryPath.isAbsolute || entryPath.normalize() != entryPath || '\\' in cursor.name)
+            if (entryPath.isAbsolute || entryPath.normalize() != entryPath || '\\' in cursor.name || cursor.name == "." || cursor.name == "..")
                 throw IllegalArgumentException("Path is either absolute or non-normalised: $entryPath")
+            count++
         }
+        if (count == 0)
+            throw IllegalArgumentException("Stream is either empty or not a JAR/ZIP")
     }
 
     // Implementations for AcceptsFileUpload
diff --git a/node/src/test/kotlin/net/corda/node/services/NodeAttachmentStorageTest.kt b/node/src/test/kotlin/net/corda/node/services/NodeAttachmentStorageTest.kt
index e0f0a355b2..7215c5603d 100644
--- a/node/src/test/kotlin/net/corda/node/services/NodeAttachmentStorageTest.kt
+++ b/node/src/test/kotlin/net/corda/node/services/NodeAttachmentStorageTest.kt
@@ -9,6 +9,7 @@ import net.corda.core.read
 import net.corda.core.readAll
 import net.corda.core.utilities.LogHelper
 import net.corda.core.write
+import net.corda.core.writeLines
 import net.corda.node.services.database.RequeryConfiguration
 import net.corda.node.services.persistence.NodeAttachmentService
 import net.corda.node.services.persistence.schemas.AttachmentEntity
@@ -86,7 +87,6 @@ class NodeAttachmentStorageTest {
 
     @Test
     fun `duplicates not allowed`() {
-
         val testJar = makeTestJar()
         val storage = NodeAttachmentService(fs.getPath("/"), dataSourceProperties, MetricRegistry())
         testJar.read {
@@ -126,6 +126,14 @@ class NodeAttachmentStorageTest {
         }
     }
 
+    @Test(expected = IllegalArgumentException::class)
+    fun `non jar rejected`() {
+        val storage = NodeAttachmentService(fs.getPath("/"), dataSourceProperties, MetricRegistry())
+        val path = fs.getPath("notajar")
+        path.writeLines(listOf("Hey", "there!"))
+        path.read { storage.importAttachment(it) }
+    }
+
     private var counter = 0
     private fun makeTestJar(): Path {
         counter++