Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New CS: GraphQL #434
New CS: GraphQL #434
Changes from 5 commits
ca64aac
c32fd1e
904b814
d67346a
bf8ccf8
5d0c975
6b41373
b6fc880
0166339
b025b22
0547d00
045a21f
0b3844a
506a287
c483d37
c734132
934c422
c86ef61
5fa8cb1
2613cf6
d43e786
5afd00a
d891cc6
4092bbe
67d47bb
2f8b358
5d865f5
3f960b1
dc5486a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to avoid narrowing injections to SQL injection, since GraphQL is widely used with NoSQL databases.
I am afraid someone reading this may think: "I am not using SQL so I am safe". We shall not neglect LDAP queries, OS commands, XML parsers, and ORM/ODM injections.
Linking to API8:2019 Injection makes sense since:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much agree with this feedback! But as I work on this rewrite, I have a problem calling out "ORM injection" as an issue. I have less experience with ORMs so maybe I'm off base here, but it seems to me that when a developer uses an ORM properly it will parameterize their queries and prevent injection entirely. Where devs would get into trouble is using functionality of an ORM that allows for non-parameterized queries like raw query execution/manipulation or similar.
So I think calling it "ORM injection" isn't really accurate; it's actually just plain old SQL injection but using an ORM, which is just a different driver/connection than is traditionally used. Instead I think the advice to devs should be to use parameterized queries with their ORM and not to use their ORM to mess with raw queries.
Any thoughts on that? We don't necessarily need to agree on terminology of whether ORM injection is its own attack, but we should agree on the recommendation to devs using ORMs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point.
The ORM Injection is a little different from a plain SQL Injection, since the attacker will target the ORM itself or the SQL generated by the ORM, instead of the DBMS.
A developer may choose an ORM that offers parameterized queries as well as a fluent querying API to programmatically build queries (example). Although the parameterized queries API is safe, attackers may trick the ORM to generate unsafe SQL.
I am OK recommending ORMs/ODMs as long as we add something like: "ORMs/ODMs may introduce other issues such as ORM Injection: always audit your dependencies."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to add one more point: when exploiting SSRF, we don't really break or the change the syntax of any command/query/code.
Hence, I wouldn't say that SSRF is a type of injection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine IMO. Short introduction and link to the other CS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to go with a general recommendation and an items list with the appropriate references.
Shorter sentences look more actionable and 1) GraphQL is well-documented and 2) other CSs already have most of what's needed to address the issue.
Example:
Validate incoming data using sufficient filters to only allow valid values for each input parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I don't like the idea of narrowing Injections to SQL Injection.
GraphQL is widely used with NoSQL databases and LDAP queries, OS commands, XML parsers, and ORM/ODM injections can not be neglected.
While looking for public API incidents to include in the OWAS API Security Top 10, we found several OS commands.
When handling input meant to be passed to another interpreter (e.g. SQL/NoSQL, OS, LDAP), always prefer safe APIs with support for parameterized statements. If such APIs are not available, always escape/encode input data according to the target interpreter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely we should not say "Because of this, it may be better to only expose your API internally".
I am not sure whether I understood your thoughts about resources managements.
My suggestion
Not properly limiting the amount of resource your API can use (e.g. CPU or memory), may compromise your API responsiveness and availability, leaving it vulnerable to DoS attacks. Containerization platforms tend to make this task much easier: see how to limit memory, CPU, number of restarts, file descriptors, and processes using Docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doc was originally made for my company which has APIs that are internal-only and externally-facing. So I was trying to basically say if the API is not exposed externally we don't have to be as careful with preventing DoS given the low likelihood of exploitation and significant cost to add protections against DoS. I don't know if we want to add this line of thinking to a CS; it's basically recommending to accept the risk because it's low enough. Seems out of place in a CS but is valuable info to engineering teams.
There are a ton of things that can be done to prevent DoS, for sure. We could link to the DoS CS, but it seems in need of work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I see your point, nevertheless I still think it is risky to say something like that.
I don't think that we should advice accepting even low enough risks.
This is something engineering teams should discuss and define with the business: what's the risk it is willing to accept.
Let's see what other think about this subject.
Regarding the work needed in the DoS CS, may be we can open a new issue to put it in the agenda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would delete _ I'm not sure if there is a good/trusted open source project that does this._.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah from my quick searching I couldn't really find anything legit or trusted. We can def delete this. It seems like the code to do this is pretty simple though; would be cool to see an official OWASP project tackle this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you feel that it is worth to have a short code snippet here we can think about it.
We try to not have longer code samples because they are hard to maintain but short snippet or pseudocode is a good idea sometimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about shortly summarising these articles and link to them only for further reference. Now it is impossible to use it without reading more articles that can disappear in some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion
The amount of resources required to satisfy a request greatly depends on the user input and endpoint business logic. As long as your API accepts concurrent requests, what is generally true, then these requests compete for available resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check if timeouts are nativley supported. If not then delete It doesn’t seem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware of any native timeouts.
Such feature would be implementation dependent. No reference to timeouts was found in the popular Apollo Server documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should link to our RBAC cheatsheet https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Access_Control_Cheat_Sheet.md#role-based-access-control-rbac and if we are missing something in that CS let improve it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphQL Interfaces and Unions, as well as Query Resolvers are good recommendations to tackle access control issues:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with the general idea, but we need to make this more actionable.
Then, based on requester's profile/permissions, return the most appropriate data type.
Access Control validation may be performed passing some middleware to the mutation function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphiQL is quite often found on production systems.
We should also add some recommendation to do not deploy such tools in production environments, nor make them publicly accessible on other environments (e.g. QA, staging, dev).
May be this should has it's own category e.g. "Security Misconfigurations", but it came up to my mind while reviewing the Introspection section, because it's GraphiQL first query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't heard of GraphiQL. Great call out! I think we should add a general note about not providing utilities like this in production and specifically name GraphiQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In OWASP API Security Top 10 2019 we decided to split IDOR back in two different categories:
Insecure Direct Object Reference isn't a very accurate name: there's nothing insecure about using object's unique identifier as long as proper access control validations are performed. Creating some identifiers indirection won't solve the real issue unless the translation mechanism performs the required access control validations.
How are these recommendations different from "Query Access" ones?
I think we should remove this section and add the IDOR CS as a reference to the Query Access section.