Skip to content

add some docs regarding security #335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bbrehm
Copy link
Contributor

@bbrehm bbrehm commented Jul 8, 2025

This adds some clarifications regarding security goals. Big thanks to https://github.com/xavierpinho ❤️ for mentioning that here https://github.com/joernio/flatgraph/security/advisories/GHSA-jqmx-3x2p-69vh

Any improvement / rewording ideas?

@bbrehm bbrehm requested review from mpollmeier and xavierpinho July 8, 2025 11:33
Copy link
Contributor

@xavierpinho xavierpinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the prompt discussion and clarifications!

Just a typo (double negation) I spotted:

nor should (not/it) cause

@@ -490,6 +490,18 @@ generated schema (as opposed to an ad-hoc schema infered from the graph data).
The build targets JDK8, so that's the minimum version. The build itself requires JDK11+.
However in any case it is highly encouraged to use a modern JVM, such as JDK20.

## What about security / untrusted flatgraph files?
The main potentially security issue is the following situation: You get handed an untrusted, potentially malicious, flatgraph file, and want to handle it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The main potentially security issue is the following situation: You get handed an untrusted, potentially malicious, flatgraph file, and want to handle it.
The main potentially security issue is probably: how can you handle an untrusted - and potentially malicious - flatgraph file?

@@ -490,6 +490,18 @@ generated schema (as opposed to an ad-hoc schema infered from the graph data).
The build targets JDK8, so that's the minimum version. The build itself requires JDK11+.
However in any case it is highly encouraged to use a modern JVM, such as JDK20.

## What about security / untrusted flatgraph files?
The main potentially security issue is the following situation: You get handed an untrusted, potentially malicious, flatgraph file, and want to handle it.
Deserializing a `.fg` file should not pop a shell / cause privilege escalation, nor should not cause excessive filesystem activity. However, it may take an
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Deserializing a `.fg` file should not pop a shell / cause privilege escalation, nor should not cause excessive filesystem activity. However, it may take an
Deserializing a `.fg` file should not be able to open a shell or cause privilege escalation, nor should it cause excessive filesystem activity. However, it may take an

## What about security / untrusted flatgraph files?
The main potentially security issue is the following situation: You get handed an untrusted, potentially malicious, flatgraph file, and want to handle it.
Deserializing a `.fg` file should not pop a shell / cause privilege escalation, nor should not cause excessive filesystem activity. However, it may take an
unbounded amount of time and memory, potentially leading to an OOM crash of the JVM that might not be recoverable from within the JVM by catching some exceptions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unbounded amount of time and memory, potentially leading to an OOM crash of the JVM that might not be recoverable from within the JVM by catching some exceptions.
unbounded amount of time and memory, potentially leading to an OutOfMemoryError in which is typically not recoverable and will bring down the entire JVM.


The easiest malicious but completely valid `.fg` file in that vein is a ZIP-bomb. We take care not to decompress graphs into the filesystem, but we do decompress them into memory.

If you need to handle untrusted `.fg` files, then we recommend some form of sandboxing in order to limit the DoS impact.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you need to handle untrusted `.fg` files, then we recommend some form of sandboxing in order to limit the DoS impact.
If you need to handle untrusted `.fg` files you should really sandbox your process, in order to limit the DoS impact.


If you need to handle untrusted `.fg` files, then we recommend some form of sandboxing in order to limit the DoS impact.

If you do decide against our recommendation to write your own code to "sanity check" potentially malicious `.fg` files before attempting to deserialize them, then we'd be happy for your feedback and PRs. (also beware of potential parser differentials -- e.g. the manifest json can be reached either via the offset from the file header, or via `tail -n 1`, and these may very well be different manifests)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you do decide against our recommendation to write your own code to "sanity check" potentially malicious `.fg` files before attempting to deserialize them, then we'd be happy for your feedback and PRs. (also beware of potential parser differentials -- e.g. the manifest json can be reached either via the offset from the file header, or via `tail -n 1`, and these may very well be different manifests)
You should always sanity check potentially malicious `.fg` files before attempting to deserialize them. If there's anything we can add in flatgraph's Deserializer, we'd be happy for your feedback and PRs.
N.b. also beware of potential parser differentials -- e.g. the manifest json can be reached either via the offset from the file header, or via `tail -n 1`, and these may very well be different manifests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants