Deserialization: verify manifest size #336
Conversation
mpollmeier
requested review from
bbrehm,
johannescoetzee and
ml86
and removed request for
ml86
| originalDump shouldBe newDump | ||
| } | ||
|
|
||
| /* show that we're no longer vulnerable to the denial of service issue filed here: |
There was a problem hiding this comment.
This comment suggests that we're no longer vulnerable to the denial of service attack "small flatgraph file causes OOM". This is not correct. The fix only addresses the case where the manifest offset is changed, which is one specific instance of that general issue -- and we currently do not intend to address the general issue at all.
Can you be clearer on that? Otherwise we'll get justified "oh, we can bypass that check" vuln reports, and will implicitly condone deployments for which this is an actual security vuln.
There was a problem hiding this comment.
👍🏻 added more details
| throw new DeserializationException(s"corrupt file: manifest size ($manifestSize) cannot be larger than the file's size ($fileSize)") | ||
| } | ||
|
|
||
| val manifestBytes = ByteBuffer.allocate(manifestSize.toInt) |
There was a problem hiding this comment.
If you want to do this check at all, then please also handle the case where manifestSize overflows the 32 bit integer.
bbrehm
left a comment
bbrehm
left a comment
There was a problem hiding this comment.
Please consider addressing the comments; but if you disagree, it's OK to merge as-is.
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.
mpollmeier
force-pushed
the
michael/memory-allocation-fix
branch
from
1822557 to
cf6f7b2
Compare
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.