From aef8189a1f30efe717c3cf2cfdbf3a662ca6e024 Mon Sep 17 00:00:00 2001 From: Michael Pollmeier Date: Tue, 8 Jul 2025 14:35:59 +0200 Subject: [PATCH 1/3] Deserialization: verify manifest size re https://github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh n.b. it is not our goal to have a "safe" file format, and we need to document that properly. But adding some more straightforward checks doesn't harm. --- .../flatgraph/storage/Deserialization.scala | 15 ++++-- .../scala/flatgraph/SerializationTests.scala | 46 ++++++++++++++++++- 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/core/src/main/scala/flatgraph/storage/Deserialization.scala b/core/src/main/scala/flatgraph/storage/Deserialization.scala index bd999892..8137c45f 100644 --- a/core/src/main/scala/flatgraph/storage/Deserialization.scala +++ b/core/src/main/scala/flatgraph/storage/Deserialization.scala @@ -173,8 +173,9 @@ object Deserialization { } def readManifest(channel: FileChannel): ujson.Value = { - if (channel.size() < HeaderSize) - throw new DeserializationException(s"corrupt file, expected at least $HeaderSize bytes, but only found ${channel.size()}") + val fileSize = channel.size() + if (fileSize < HeaderSize) + throw new DeserializationException(s"corrupt file: expected at least $HeaderSize bytes, but only found ${channel.size()}") val header = ByteBuffer.allocate(HeaderSize).order(ByteOrder.LITTLE_ENDIAN) var readBytes = 0 @@ -185,21 +186,25 @@ object Deserialization { val headerBytes = new Array[Byte](Keys.Header.length) header.get(headerBytes) - if (!Arrays.equals(headerBytes, Keys.Header)) + if (!Arrays.equals(headerBytes, Keys.Header)) { throw new DeserializationException( s"expected header '$MagicBytesString' (`${Keys.Header.mkString("")}`), but found '${headerBytes.mkString("")}'" ) + } val manifestOffset = header.getLong() val manifestSize = channel.size() - manifestOffset - val manifestBytes = ByteBuffer.allocate(manifestSize.toInt) + if (manifestSize > fileSize) { + throw new DeserializationException(s"corrupt file: manifest size ($manifestSize) cannot be larger than the file's size ($fileSize)") + } + + val manifestBytes = ByteBuffer.allocate(manifestSize.toInt) readBytes = 0 while (readBytes < manifestSize) { readBytes += channel.read(manifestBytes, readBytes + manifestOffset) } manifestBytes.flip() ujson.read(manifestBytes) - } private def readPool(manifest: GraphItem, fileChannel: FileChannel, zstdCtx: ZstdWrapper.ZstdCtx): Array[String] = { diff --git a/core/src/test/scala/flatgraph/SerializationTests.scala b/core/src/test/scala/flatgraph/SerializationTests.scala index 03932946..bdf61720 100644 --- a/core/src/test/scala/flatgraph/SerializationTests.scala +++ b/core/src/test/scala/flatgraph/SerializationTests.scala @@ -1,11 +1,16 @@ package flatgraph import flatgraph.misc.DebugDump.debugDump +import flatgraph.storage.Deserialization.DeserializationException import flatgraph.storage.{Deserialization, Serialization} import org.scalatest.matchers.should.Matchers import org.scalatest.wordspec.AnyWordSpec -import java.nio.file.Files +import java.nio.file.{Files, Path} +import java.io.RandomAccessFile +import java.nio.ByteBuffer +import java.nio.ByteOrder +import scala.util.Using class SerializationTests extends AnyWordSpec with Matchers { @@ -40,4 +45,43 @@ class SerializationTests extends AnyWordSpec with Matchers { originalDump shouldBe newDump } + /* show that we're no longer vulnerable to the denial of service issue filed here: + * https://github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh + */ + "is no longer vulnerable to manifest size attack" in { + val schema = TestSchema.make(1, 0) + val graph = Graph(schema) + val diff = DiffGraphBuilder(schema).addNode(new GenericDNode(0)) + DiffGraphApplier.applyDiff(graph, diff) + + val storagePath = Files.createTempFile(s"flatgraph-${getClass.getSimpleName}", "fg") + Serialization.writeGraph(graph, storagePath) + patchFile(storagePath) + + // when the vulnerability was reported, the following line raised a: + // `java.lang.OutOfMemoryError: Requested array size exceeds VM limit` + intercept[DeserializationException] { + Deserialization.readGraph(storagePath, Option(graph.schema)) + }.getMessage should include("corrupt file: manifest size") + } + + /** manipulate file as detailed in https: //github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh */ + private def patchFile(path: Path): Unit = { + Using.resource(new RandomAccessFile(path.toFile, "rw")) { file => + // Seek to end and get file size + file.seek(file.length()) + val fileSize = file.getFilePointer + + // Calculate malicious offset + val maliciousOffset = fileSize - 2147483647L + + // Seek to position 8 and write the offset as little-endian long + file.seek(8) + val buffer = ByteBuffer.allocate(8) + buffer.order(ByteOrder.LITTLE_ENDIAN) + buffer.putLong(maliciousOffset) + file.write(buffer.array()) + } + } + } From cf6f7b205338b63afc9258c879aa2ca699403326 Mon Sep 17 00:00:00 2001 From: Michael Pollmeier Date: Fri, 11 Jul 2025 16:41:42 +0200 Subject: [PATCH 2/3] clarify comment --- core/src/test/scala/flatgraph/SerializationTests.scala | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/test/scala/flatgraph/SerializationTests.scala b/core/src/test/scala/flatgraph/SerializationTests.scala index bdf61720..af02d3c7 100644 --- a/core/src/test/scala/flatgraph/SerializationTests.scala +++ b/core/src/test/scala/flatgraph/SerializationTests.scala @@ -45,8 +45,10 @@ class SerializationTests extends AnyWordSpec with Matchers { originalDump shouldBe newDump } - /* show that we're no longer vulnerable to the denial of service issue filed here: - * https://github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh + /* Show that we're no longer vulnerable to the 'denial of service attack by manipulating the manifest' + * issue filed here: https://github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh + * Note that we cannot prevent all potential 'small flatgraph file leads to OOM error' attacks. + * Always treat untrusted files with precaution... */ "is no longer vulnerable to manifest size attack" in { val schema = TestSchema.make(1, 0) From a399c871be033e55539a14a2f0e86072aad29024 Mon Sep 17 00:00:00 2001 From: Michael Pollmeier Date: Fri, 11 Jul 2025 16:54:19 +0200 Subject: [PATCH 3/3] also check for manifestSize > Int.MaxValue --- core/src/main/scala/flatgraph/storage/Deserialization.scala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/flatgraph/storage/Deserialization.scala b/core/src/main/scala/flatgraph/storage/Deserialization.scala index 8137c45f..c7382a73 100644 --- a/core/src/main/scala/flatgraph/storage/Deserialization.scala +++ b/core/src/main/scala/flatgraph/storage/Deserialization.scala @@ -194,9 +194,10 @@ object Deserialization { val manifestOffset = header.getLong() val manifestSize = channel.size() - manifestOffset - if (manifestSize > fileSize) { + if (manifestSize > fileSize) throw new DeserializationException(s"corrupt file: manifest size ($manifestSize) cannot be larger than the file's size ($fileSize)") - } + if (manifestSize > Int.MaxValue) + throw new DeserializationException(s"corrupt file: unreasonably large manifest size ($manifestSize)... aborting") val manifestBytes = ByteBuffer.allocate(manifestSize.toInt) readBytes = 0