Skip to content

Commit 1822557

Browse files
committed
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.
1 parent 94b2838 commit 1822557

File tree

2 files changed

+55
-6
lines changed

2 files changed

+55
-6
lines changed

core/src/main/scala/flatgraph/storage/Deserialization.scala

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,9 @@ object Deserialization {
173173
}
174174

175175
def readManifest(channel: FileChannel): ujson.Value = {
176-
if (channel.size() < HeaderSize)
177-
throw new DeserializationException(s"corrupt file, expected at least $HeaderSize bytes, but only found ${channel.size()}")
176+
val fileSize = channel.size()
177+
if (fileSize < HeaderSize)
178+
throw new DeserializationException(s"corrupt file: expected at least $HeaderSize bytes, but only found ${channel.size()}")
178179

179180
val header = ByteBuffer.allocate(HeaderSize).order(ByteOrder.LITTLE_ENDIAN)
180181
var readBytes = 0
@@ -185,21 +186,25 @@ object Deserialization {
185186

186187
val headerBytes = new Array[Byte](Keys.Header.length)
187188
header.get(headerBytes)
188-
if (!Arrays.equals(headerBytes, Keys.Header))
189+
if (!Arrays.equals(headerBytes, Keys.Header)) {
189190
throw new DeserializationException(
190191
s"expected header '$MagicBytesString' (`${Keys.Header.mkString("")}`), but found '${headerBytes.mkString("")}'"
191192
)
193+
}
192194

193195
val manifestOffset = header.getLong()
194196
val manifestSize = channel.size() - manifestOffset
195-
val manifestBytes = ByteBuffer.allocate(manifestSize.toInt)
197+
if (manifestSize > fileSize) {
198+
throw new DeserializationException(s"corrupt file: manifest size ($manifestSize) cannot be larger than the file's size ($fileSize)")
199+
}
200+
201+
val manifestBytes = ByteBuffer.allocate(manifestSize.toInt)
196202
readBytes = 0
197203
while (readBytes < manifestSize) {
198204
readBytes += channel.read(manifestBytes, readBytes + manifestOffset)
199205
}
200206
manifestBytes.flip()
201207
ujson.read(manifestBytes)
202-
203208
}
204209

205210
private def readPool(manifest: GraphItem, fileChannel: FileChannel, zstdCtx: ZstdWrapper.ZstdCtx): Array[String] = {

core/src/test/scala/flatgraph/SerializationTests.scala

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,16 @@
11
package flatgraph
22

33
import flatgraph.misc.DebugDump.debugDump
4+
import flatgraph.storage.Deserialization.DeserializationException
45
import flatgraph.storage.{Deserialization, Serialization}
56
import org.scalatest.matchers.should.Matchers
67
import org.scalatest.wordspec.AnyWordSpec
78

8-
import java.nio.file.Files
9+
import java.nio.file.{Files, Path}
10+
import java.io.RandomAccessFile
11+
import java.nio.ByteBuffer
12+
import java.nio.ByteOrder
13+
import scala.util.Using
914

1015
class SerializationTests extends AnyWordSpec with Matchers {
1116

@@ -40,4 +45,43 @@ class SerializationTests extends AnyWordSpec with Matchers {
4045
originalDump shouldBe newDump
4146
}
4247

48+
/* show that we're no longer vulnerable to the denial of service issue filed here:
49+
* https://github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh
50+
*/
51+
"is no longer vulnerable to manifest size attack" in {
52+
val schema = TestSchema.make(1, 0)
53+
val graph = Graph(schema)
54+
val diff = DiffGraphBuilder(schema).addNode(new GenericDNode(0))
55+
DiffGraphApplier.applyDiff(graph, diff)
56+
57+
val storagePath = Files.createTempFile(s"flatgraph-${getClass.getSimpleName}", "fg")
58+
Serialization.writeGraph(graph, storagePath)
59+
patchFile(storagePath)
60+
61+
// when the vulnerability was reported, the following line raised a:
62+
// `java.lang.OutOfMemoryError: Requested array size exceeds VM limit`
63+
intercept[DeserializationException] {
64+
Deserialization.readGraph(storagePath, Option(graph.schema))
65+
}.getMessage should include("corrupt file: manifest size")
66+
}
67+
68+
/** manipulate file as detailed in https: //github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh */
69+
private def patchFile(path: Path): Unit = {
70+
Using.resource(new RandomAccessFile(path.toFile, "rw")) { file =>
71+
// Seek to end and get file size
72+
file.seek(file.length())
73+
val fileSize = file.getFilePointer
74+
75+
// Calculate malicious offset
76+
val maliciousOffset = fileSize - 2147483647L
77+
78+
// Seek to position 8 and write the offset as little-endian long
79+
file.seek(8)
80+
val buffer = ByteBuffer.allocate(8)
81+
buffer.order(ByteOrder.LITTLE_ENDIAN)
82+
buffer.putLong(maliciousOffset)
83+
file.write(buffer.array())
84+
}
85+
}
86+
4387
}

0 commit comments

Comments
 (0)