New CS: GraphQL

[bigshebang]

This PR covers issue #421

This is a new PR with a different branch in my fork

[mackowski]

In general very good work. I would like to have more practical advices that is to summarise some of the linked articles and link to them for further reference. That way the cheatsheet will give the essence and if someone is more interested in details he/she have links to go deeper.

[mackowski]

I would delete _ I'm not sure if there is a good/trusted open source project that does this._.

[bigshebang]

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!

[mackowski]

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.

[mackowski]

It is fine IMO. Short introduction and link to the other CS.

[PauloASilva]

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.

• Use specific GraphQL data types such as scalars or enums. Write custom GraphQL validators for more complex validations.
• Define schemas for mutations input.
• Use a single internal character encoding and whitelist allowed characters.
• Gracefully rejected invalid input.

[mackowski]

We should check if timeouts are nativley supported. If not then delete It doesn’t seem.

[PauloASilva]

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.

[mackowski]

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.

[PauloASilva]

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.

• Define and enforce maximum size of data on all incoming parameters and payloads such as maximum length for strings and maximum number of elements in arrays.
• Add proper validation for request parameters controlling the number of resources to be returned in the response.
• Implement pagination mechanism to limit the amount of data to be returned in a single request/response.

[mackowski]

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 :)

[PauloASilva]

GraphQL Interfaces and Unions, as well as Query Resolvers are good recommendations to tackle access control issues:

• Interfaces and Unions allows to create more structured and "incremental" data types which can be used to return more or less object properties, according to requestor rights
• Query Resolvers a good candidate to perform access controls maybe using some RBAC middleware.

[PauloASilva]

Nice work you've done here: congrats!

I left my thoughts throughout the content.
In general I think we should make sections more actionable: an introduction paragraph and a list of recommendations. In some cases I drafted something similar to this.

This is a first review of what has been already done. We (OWASP API Security Project) plan to contribute further, after hearing your thoughts about the suggestions made here.

Cheers,
Paulo A. Silva

[PauloASilva]

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:

1. we describe real API attacker scenarios
2. there's a "How To Prevent" section

[bigshebang]

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.

[PauloASilva]

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.

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.

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."

[inonshk]

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

[PauloASilva]

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.

• Use specific GraphQL data types such as scalars or enums. Write custom GraphQL validators for more complex validations.
• Define schemas for mutations input.
• Use a single internal character encoding and whitelist allowed characters.
• Gracefully rejected invalid input.

[PauloASilva]

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.

• Use prepared statements for querying as a way to prevent SQL Injection.
• Escape/Encode input data according to the target interpreter e.g. OS, LDAP.
• Choose a well-documented and actively maintained escaping/encoding library.

[PauloASilva]

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.

[bigshebang]

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.

[PauloASilva]

Ok, now I see your point, nevertheless I still think it is risky to say something like that.

• "Internal-only" is something hard to define and someone may lower defenses based on a wrong premise.
• If internal-only APIs interact with external services, then those services are potential threat agents.
• Sometimes threat agents belong to the organization.
• Compromised systems may allow access to internal-only APIs.

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.

[PauloASilva]

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.

• Define and enforce maximum size of data on all incoming parameters and payloads such as maximum length for strings and maximum number of elements in arrays.
• Add proper validation for request parameters controlling the number of resources to be returned in the response.
• Implement pagination mechanism to limit the amount of data to be returned in a single request/response.

[PauloASilva]

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.

[PauloASilva]

GraphQL Interfaces and Unions, as well as Query Resolvers are good recommendations to tackle access control issues:

• Interfaces and Unions allows to create more structured and "incremental" data types which can be used to return more or less object properties, according to requestor rights
• Query Resolvers a good candidate to perform access controls maybe using some RBAC middleware.

[PauloASilva]

I do agree with the general idea, but we need to make this more actionable.

• Use Interfaces and Unions to create hierarchical data types.
  Then, based on requester's profile/permissions, return the most appropriate data type.
• Use Mutation Resolvers to update data, and perform access control validation, may be passing some middleware
• Always validate whether requester is authorized to execute the requested mutation (RBAC).
  Access Control validation may be performed passing some middleware to the mutation function.

[PauloASilva]

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.

[bigshebang]

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.

[PauloASilva]

In OWASP API Security Top 10 2019 we decided to split IDOR back in two different categories:

• API1:2019 Broken Object Level Authorization
• API5:2019 Broken Function Level Authorization

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.

[mackowski]

@bigshebang do you need help with resolving these comments?
@PauloASilva I saw that you have a lot of good ideas here, if you want please suggest changes (you can use suggest change feature from GitHub for it)

[mackowski]

@bigshebang are you still around? this is amazing work that just need to be finished!

[bigshebang]

@mackowski I am still around! I'm hoping to come back to this in the next week or two

[mackowski]

@mackowski I am still around! I'm hoping to come back to this in the next week or two

Awesome! Thanks for reply!

[bigshebang]

New changes are up! Please give your feedback :)

[bigshebang]

Would love some help from anyone with more GraphQL experience on this.

[PauloASilva]

Are you talking about something like this?

query {
  author(id: "abc") {
    posts(first: 99999999999) {
      title
    }
  }
}

Since GraphQL resolvers logic is implementation dependent, they can do whatever the business requirement requires e.g. returning the first N posts

[bigshebang]

Yes that seems like the type of query that could be stopped with amount limiting! I wasn't sure what it would look like but this seems like it. I'll add this example in soon.

[bigshebang]

Also would love some help from a more experienced GraphQL person or somebody who has a test setup handy. If not I will eventually spend some time to mess with this myself and do further research.

[bigshebang]

After seeing the example query from PauloASilva for amount limiting I think this mitigation is different. I will be adding this attack and mitigation in soon.

An example would be the below query (which you can test here):

{
  hero {
    name
    appearsIn
  }
  second:hero {
    name
    appearsIn
  }
}

[PauloASilva]

We can use "safe programming interfaces" instead of "safe tools" or "safe APIs".

[PauloASilva]

A full document review.
We're getting closer and closer to a final draft: great work @bigshebang!

[PauloASilva]

Suggested change

	- Always prefer safe tools with support for parameterized statements
	- Always choose libraries/modules/packages offering safe programming interfaces, such as parameterized statements.

[PauloASilva]

Are you talking about something like this?

query {
  author(id: "abc") {
    posts(first: 99999999999) {
      title
    }
  }
}

Since GraphQL resolvers logic is implementation dependent, they can do whatever the business requirement requires e.g. returning the first N posts

[bigshebang]

I looked at this a bit more and it seems like this hint feature is just standard/built-in for GraphQL. I've been looking at how to actually disable this and am not sure how just yet. Gonna keep doing more research but welcome any help. So far these couple pages might have an answer on this:

• https://www.apollographql.com/docs/apollo-server/data/errors/
• https://www.howtographql.com/graphql-java/7-error-handling/
• https://blog.logrocket.com/handling-graphql-errors-like-a-champ-with-unions-and-interfaces/
• https://www.apollographql.com/docs/react/data/error-handling/

[PauloASilva]

GraphQL Apollo server field name hints are provided by GraphQL.js.
It seems that there's no simple and easy way to disable this feature (graphql/graphql-js#2247) other than using some middleware to prevent that information to leak.

Since this is implementation-dependent, I think we're good to go with a general recommendation since it will create the required awareness how the feature can be abused: shapeshifter is just one tool that should be able to do it.

[bigshebang]

I would like to actually test this out to confirm whether or not it works before adding a firm recommendation. Again, any help is appreciated :) If not I will get to it at some point.

We could also publish the CS without this point and have it as an issue to add later.

[bigshebang]

Ah I didn't see your message until now Paulo. I will add the information you gave which will resolve this issue.

[PauloASilva]

I would like to actually test this out to confirm whether or not it works before adding a firm recommendation. Again, any help is appreciated :) If not I will get to it at some point.

We could also publish the CS without this point and have it as an issue to add later.

I did test shapeshifter. Although it provides the feature demonstrated in the video, it only works if introspection is also enabled, what is not required to take advantage of GraphQL server field name hints.

I am OK with your changes regarding this specific topic: I think it brings enough awareness and guidance regarding what should be enabled/disabled in a production environment.

[bigshebang]

I took another look at some stuff and made a change with IDOR. Now there are just 3 issues I want to iron out and this will be ready to go!

[PauloASilva]

I am happy with the overall result/content.
I think it is time to get it merged 🚀

Cheers,
Paulo A. Silva

[ThunderSon]

Other than the review done inline, just 2 more things:

• Cheatsheets links shouldn't be to the cheatsheetseries website. They should just call the other file (I did an example in my suggestions). The engine will resolve it.
• Referencing WSTG shouldn't be done on the "latest" branch. It should either use a version (v41 for example), or "stable".

Well done overall, I am just adding some CS magic touchup, not much. You guys did a fantastic job. As a v1 this is a job well done, it can be refined in next versions as it's better adopted across other guides.

[ThunderSon]

This can be switched up a bit to something saying the following:

• What GraphQL is.
• Why it's growing
• Where it's used

Like just a quick 2-3 liners, very concise and direct, not chit chat. Check out password storage, crypto storage, forgot password. These have the new introduction concept.

[ThunderSon]

Suggested change

	- Expensive queries can easily lead to Denial of Service (DoS); there are several defenses ranging from simple to complex
	- Use simple and straight forward queries. Expensive queries will lead to Denial of Service attacks.

[ThunderSon]

Suggested change

	- It is very important to implement proper access control (authorization), but this can be tricky
	- Ensure proper access control checks are there.

[ThunderSon]

Suggested change

	- Some default configurations (Introspection, GraphiQL, excessive errors) should be disabled/changed before releasing an API to production
	- Disable default configurations (_e.g._ Introspection, GraphiQL, excessive errors, etc.).

[ThunderSon]

Suggested change

	- Disable/restrict introspection + GraphiQL
	- Disable or restrict based on your needs Introspection and GraphiQL which should only be used for development purposes.

[bigshebang]

@ThunderSon Thanks for the great feedback, I've gone through it all and incorporated everything. Please give it another look and give me the final okay.

[ThunderSon]

Amazing job! 😄

[mackowski]

I love it! Thank you all for this work!
If no one have anything against I will merge it tomorrow :) Any other issues can be addressed via separate issues/PR.