Skip to content

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

README.md Outdated

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Our recommendation is to not bother with sanity checks, and do sandboxing instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your initial text says "our recommendation to write your own code to "sanity check" potentially malicious .fg files", which I interpret as 'we do recommend to write code to sanity check.

But yes, fair enough, will provide a new suggestion based on the current version.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, you already updated that part - thanks!

bbrehm and others added 5 commits July 10, 2025 15:04
Co-authored-by: Michael Pollmeier <[email protected]>
Co-authored-by: Michael Pollmeier <[email protected]>
Co-authored-by: Michael Pollmeier <[email protected]>
Co-authored-by: Michael Pollmeier <[email protected]>
@mpollmeier mpollmeier merged commit ca81e55 into master Jul 14, 2025
1 check passed
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.

4 participants