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

New CS: GraphQL #434

Merged
merged 29 commits into from Oct 30, 2020
Merged

New CS: GraphQL #434

merged 29 commits into from Oct 30, 2020

Conversation

bigshebang
Copy link
Contributor

This PR covers issue #421

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

@ThunderSon ThunderSon changed the title New cs 421 New CS: GraphQL Jun 24, 2020
Copy link
Collaborator

@mackowski mackowski left a comment

Choose a reason for hiding this comment

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

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.


#### Depth Limiting

This is limiting the depth of incoming queries so that the user cannot supply unnecessarily deep queries that could consume a lot of resources. See [this](https://www.apollographql.com/blog/securing-your-graphql-api-from-malicious-queries-16130a324a6b) and [this blog post](https://www.howtographql.com/advanced/4-security/) as well as [this Github repo](https://github.com/stems/graphql-depth-limit) (old, potentially unsupported) for more details on implementing this control. I'm not sure if there is a good/trusted open source project that does this.
Copy link
Collaborator

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

Copy link
Contributor Author

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!

Copy link
Collaborator

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.


#### General Practices

> We can probably nix this section and just link to the Input Validation Cheat Sheet. I'll leave for now in case there is anything of value somebody else sees and thinks should be left in.
Copy link
Collaborator

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.

Copy link
Contributor

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.


#### Timeouts

Adding timeouts can be a simple way to limit how much resources any single request can consume. Timeout requirements will differ by API and data fetching mechanism; there isn't one timeout value that will work across the board. It doesn't seem like GraphQL natively supports timeouts so this would require custom code (non-trivial) or adding a timeout on an HTTP server, reverse proxy, or load balancer (easy). See [this](https://www.howtographql.com/advanced/4-security/) and [this blog post](https://medium.com/workflowgen/graphql-query-timeout-and-complexity-management-fab4d7315d8d) about timeouts with GraphQL.
Copy link
Collaborator

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.

Copy link
Contributor

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.


#### Amount Limiting

This is limiting the quantity that can be requested for a given resource/object. See [this](https://www.apollographql.com/blog/securing-your-graphql-api-from-malicious-queries-16130a324a6b) and [this blog post](https://www.howtographql.com/advanced/4-security/) as well as [this Github repo](https://github.com/joonhocho/graphql-input-number) for more details on implementing this control. I'm not sure if there is a good/trusted open source project that does this.
Copy link
Collaborator

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.

Copy link
Contributor

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.

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


#### Query Access

As part of a GraphQL API there will be various data fields that can be returned. One thing to consider is if you want different levels of access around these fields. For example, you may only want certain consumers to be able to fetch certain data fields rather than allowing all consumers to be able to retrieve all available fields. This can be done by adding a check in the code to ensure that the requestor should be able to read a field they are trying to fetch. This may be best implemented with simple [role-based access control](https://auth0.com/docs/authorization/concepts/rbac) ([RBAC](https://en.wikipedia.org/wiki/Role-based_access_control)): by creating roles which have access to certain fields and then attaching the correct roles to individual consumers/users. ABAC or other access control methods could also work.
Copy link
Collaborator

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

Copy link
Contributor

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:

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

cheatsheets_draft/GraphQL_Cheat_Sheet.md Show resolved Hide resolved
Copy link
Contributor

@PauloASilva PauloASilva left a comment

Choose a reason for hiding this comment

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

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


### Input Validation

Adding strict input validation can help prevent against SSRF, SQL injection, and DoS. The main design for GraphQL is that an identifier is given and the backend has a number of fetchers making HTTP, DB, or other calls. This means that user input will be included in HTTP requests, DB queries, or other requests/calls and there is opportunity for injection that could lead to SSRF or SQL injection. Some pages that may be helpful here are OWASP Cheat Sheets for generic [Injection Prevention](https://cheatsheetseries.owasp.org/cheatsheets/Injection_Prevention_Cheat_Sheet.html) or [Java Injection Prevention](https://cheatsheetseries.owasp.org/cheatsheets/Injection_Prevention_Cheat_Sheet_in_Java.html).
Copy link
Contributor

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:

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

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

Copy link

@inonshk inonshk Aug 26, 2020

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


#### General Practices

> We can probably nix this section and just link to the Input Validation Cheat Sheet. I'll leave for now in case there is anything of value somebody else sees and thinks should be left in.
Copy link
Contributor

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.


#### SQL Injection Prevention

Anytime a DB or similar query is being made, any input should be properly parameterized with prepared statements or stored procedures in order to prevent SQL/DB injection. See the [OWASP Cheat Sheet for SQL Injection Prevention](https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html) for more info.
Copy link
Contributor

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.

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


> I assume we don't want the "internal vs external" API talk in here?

For a GraphQL API that receives input more or less directly from an external party, it can be difficult to limit the possibilities a malicious user can use to exhaust the API's resources to cause slow downs or indefinite outages. It is easier to protect an API that is only used by other internal services. Because of this, it may be better to only expose your API internally. See the table below for which of these controls should be added to each type of GraphQL API.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

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


#### Amount Limiting

This is limiting the quantity that can be requested for a given resource/object. See [this](https://www.apollographql.com/blog/securing-your-graphql-api-from-malicious-queries-16130a324a6b) and [this blog post](https://www.howtographql.com/advanced/4-security/) as well as [this Github repo](https://github.com/joonhocho/graphql-input-number) for more details on implementing this control. I'm not sure if there is a good/trusted open source project that does this.
Copy link
Contributor

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.

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


#### Timeouts

Adding timeouts can be a simple way to limit how much resources any single request can consume. Timeout requirements will differ by API and data fetching mechanism; there isn't one timeout value that will work across the board. It doesn't seem like GraphQL natively supports timeouts so this would require custom code (non-trivial) or adding a timeout on an HTTP server, reverse proxy, or load balancer (easy). See [this](https://www.howtographql.com/advanced/4-security/) and [this blog post](https://medium.com/workflowgen/graphql-query-timeout-and-complexity-management-fab4d7315d8d) about timeouts with GraphQL.
Copy link
Contributor

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.


#### Query Access

As part of a GraphQL API there will be various data fields that can be returned. One thing to consider is if you want different levels of access around these fields. For example, you may only want certain consumers to be able to fetch certain data fields rather than allowing all consumers to be able to retrieve all available fields. This can be done by adding a check in the code to ensure that the requestor should be able to read a field they are trying to fetch. This may be best implemented with simple [role-based access control](https://auth0.com/docs/authorization/concepts/rbac) ([RBAC](https://en.wikipedia.org/wiki/Role-based_access_control)): by creating roles which have access to certain fields and then attaching the correct roles to individual consumers/users. ABAC or other access control methods could also work.
Copy link
Contributor

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:

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


#### Mutation Access

GraphQL supports mutation, or manipulation of data, in addition to its most common use case of data fetching. If your service implements/allows mutation then there may need to be access controls put in place to restrict which consumers, if any, can modify data through the API. Setups requiring mutation access control include APIs where only read access is intended or where only certain parties should be able to modify certain fields. This should be implemented similarly to Query access: use [RBAC](https://auth0.com/docs/authorization/concepts/rbac) and have the code only allow certain roles to perform mutation on approved data fields.
Copy link
Contributor

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.

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


GraphQL supports mutation, or manipulation of data, in addition to its most common use case of data fetching. If your service implements/allows mutation then there may need to be access controls put in place to restrict which consumers, if any, can modify data through the API. Setups requiring mutation access control include APIs where only read access is intended or where only certain parties should be able to modify certain fields. This should be implemented similarly to Query access: use [RBAC](https://auth0.com/docs/authorization/concepts/rbac) and have the code only allow certain roles to perform mutation on approved data fields.

#### Introspection
Copy link
Contributor

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.

Copy link
Contributor Author

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.


The safest and usually easiest approach is to just disable introspection system-wide. See [this page](https://lab.wallarm.com/why-and-how-to-disable-introspection-query-for-graphql-apis/) or consult your GraphQL implementation's documentation to learn how to disable introspection altogether. If your implementation does not natively support disabling introspection or if you would like to allow some consumers/roles to have this access you can build a filter in your service to only allow approved consumers to access the introspection system.

### IDOR Protection
Copy link
Contributor

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.

@mackowski
Copy link
Collaborator

@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
Copy link
Collaborator

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

@bigshebang
Copy link
Contributor Author

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

@mackowski
Copy link
Collaborator

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

Awesome! Thanks for reply!

@bigshebang
Copy link
Contributor Author

New changes are up! Please give your feedback :)

}
```

> We should have an example of someone taking advantage of no amount limiting and requesting N of an object. I'm not sure if a query can ask for N of an object directly or if the query has to use aliases and make N separate requests within the query for the same object.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
Co-authored-by: PauloASilva <pauloasilva@gmail.com>

[GraphQL batching attack](https://lab.wallarm.com/graphql-batching-attack/)?

> This is an interesting attack that we should add. It seems like Amount Limiting will prevent it but I am not sure if that is the case. Needs some investigation. This may also belong in a different section since it's not DoS, it's brute force.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
  }
}

Co-authored-by: PauloASilva <pauloasilva@gmail.com>
Copy link
Contributor

@PauloASilva PauloASilva left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@PauloASilva PauloASilva left a comment

Choose a reason for hiding this comment

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

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

cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved

When handling input meant to be passed to another interpreter (e.g. SQL/NoSQL/ORM, OS, LDAP, XML):

- Always prefer safe tools with support for parameterized statements
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
- Always prefer safe tools with support for parameterized statements
- Always choose libraries/modules/packages offering safe programming interfaces, such as parameterized statements.

cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
}
```

> We should have an example of someone taking advantage of no amount limiting and requesting N of an object. I'm not sure if a query can ask for N of an object directly or if the query has to use aliases and make N separate requests within the query for the same object.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
bigshebang and others added 2 commits August 22, 2020 20:19
Co-authored-by: PauloASilva <pauloasilva@gmail.com>
Co-authored-by: PauloASilva <pauloasilva@gmail.com>
@mackowski
Copy link
Collaborator

Hey @inonshk @nikitastupin, are you still around? @bigshebang answered some of your comments after the review can you answer back and continue the discussion or resolve the conversation?
I would like to merge this PR at some pont, there is a lot of good stuff. Please review it.


> This blurb is from nikitastupin. Would be great if we knew the name of this "hint" feature and link to documentation to disable it. I couldn't find anything from a quick Google.

Keep in mind that even if introspection is disabled you can still guess fields by brute forcing them. Furthermore, some GraphQL implementations give you a hint when a field name you provide is similar to an existing field (e.g. you provide `usr` and the response may ask if you meant the valid `user` field instead). You should consider disabling this feature.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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!

cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Show resolved Hide resolved
@PauloASilva
Copy link
Contributor

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

Cheers,
Paulo A. Silva

Copy link
Contributor

@ThunderSon ThunderSon left a comment

Choose a reason for hiding this comment

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

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.


## Introduction

Below are some quick high-level ideas to keep in mind when building a secure API with GraphQL.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
Below are some quick high-level ideas to keep in mind when building a secure API with GraphQL.

- Strict input validation is highly recommended and easy
- Expensive queries can easily lead to Denial of Service (DoS); there are several defenses ranging from simple to complex
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
- 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.


- Strict input validation is highly recommended and easy
- Expensive queries can easily lead to Denial of Service (DoS); there are several defenses ranging from simple to complex
- It is very important to implement proper access control (authorization), but this can be tricky
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
- It is very important to implement proper access control (authorization), but this can be tricky
- Ensure proper access control checks are there.

- Strict input validation is highly recommended and easy
- Expensive queries can easily lead to Denial of Service (DoS); there are several defenses ranging from simple to complex
- It is very important to implement proper access control (authorization), but this can be tricky
- Some default configurations (Introspection, GraphiQL, excessive errors) should be disabled/changed before releasing an API to production
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
- 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.).

cheatsheets_draft/GraphQL_Cheat_Sheet.md Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved
cheatsheets_draft/GraphQL_Cheat_Sheet.md Outdated Show resolved Hide resolved

By default, most GraphQL implementations have some insecure default configurations which should be changed:

- Disable/restrict introspection + GraphiQL
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
- Disable/restrict introspection + GraphiQL
- Disable or restrict based on your needs Introspection and GraphiQL which should only be used for development purposes.

bigshebang and others added 6 commits October 22, 2020 21:48
Co-authored-by: ThunderSon <32433575+ThunderSon@users.noreply.github.com>
Co-authored-by: ThunderSon <32433575+ThunderSon@users.noreply.github.com>
Co-authored-by: ThunderSon <32433575+ThunderSon@users.noreply.github.com>
@bigshebang
Copy link
Contributor Author

@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
ThunderSon previously approved these changes Oct 23, 2020
Copy link
Contributor

@ThunderSon ThunderSon left a comment

Choose a reason for hiding this comment

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

Amazing job! 😄

Co-authored-by: PauloASilva <pauloasilva@gmail.com>
Copy link
Collaborator

@mackowski mackowski left a comment

Choose a reason for hiding this comment

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

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.

@mackowski mackowski merged commit 8dddd88 into OWASP:master Oct 30, 2020
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.

None yet

6 participants