Skip to content
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

graphql: don't allocate greedily #27873

Merged
merged 4 commits into from
Aug 8, 2023
Merged

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Aug 7, 2023

The graphql code greedily allocated for the response. A query such as

{ "query": " { blocks(from:-2147483647 to:2147483647) {number hash parent { number hash } }}" }

Would thus instantly allocate space for 4 Bn pointers, going out of memory.
With this change, it will still go out of memory, but much slower.

A more rigorous fix requires us to add limits, so that anything which returns a list of things is capped at some point.


PSA: Graphql is not secure, it should not be exposed in untrusted environments, such as the internet. This has been said before, and apparently cannot be said too often.

@holiman holiman requested a review from s1na as a code owner August 7, 2023 11:36
Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM, will merge on green

@holiman holiman added this to the 1.12.1 milestone Aug 7, 2023
graphql/graphql.go Outdated Show resolved Hide resolved
holiman and others added 2 commits August 7, 2023 09:35
Co-authored-by: Sina Mahmoodi <1591639+s1na@users.noreply.github.com>
@holiman holiman merged commit 0d772b9 into ethereum:master Aug 8, 2023
1 check passed
@gln7
Copy link

gln7 commented Aug 10, 2023

I thought my issue is invalid and wont be fixed.
Some project believe it is safe to expose graphql https://wiki.polygon.technology/docs/pos/reference/default-ports/ , graphql has can-be-public status.

@jsvisa
Copy link
Contributor

jsvisa commented Aug 11, 2023

@gln7 what's your issue? You can submit an issue and then refer to this PR if you think it's relevant

@gln7
Copy link

gln7 commented Aug 11, 2023

@gln7 what's your issue? You can submit an issue and then refer to this PR if you think it's relevant

I reported it to your bug bounty program on Monday, got a response that the bug is invalid. Anyway, i was glad to help.

@holiman
Copy link
Contributor Author

holiman commented Aug 11, 2023

@gln7 Thank you for reporting!
I received your report indirectly, and did not answer you directly -- I am not sure what the exact phrasing was on the response you got.

We never meant that "the bug is invalid". Indeed, it is a valid bug -- what we wanted to convey was that the bug was not in scope for a bounty, because we already consider the graphql surface to be privileged. That is: if an attacker can access the graphql endpoint, the attacker can probably somehow bring the machine down or make it consume resources to make it no longer able to keep up with the chain. Hence why I posted this bug so openly.

See also the bounty rules

Vulnerabilities requiring the user to have publicly exposed an API, such as JSON-RPC or the Beacon API, is out of scope of the bug bounty program.

Sorry for not crediting you in the PR description / commit message. Now that I look closer, I see that I could have found your github-handle via the supplied gist.

@gln7
Copy link

gln7 commented Aug 11, 2023

@holiman Thanks for the answer!

@holiman holiman deleted the graphql_nits branch October 11, 2023 07:26
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Fixes a graphql-dos

---------

Co-authored-by: Sina Mahmoodi <1591639+s1na@users.noreply.github.com>
Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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