Cms: [FR] Improve DOS protection defaults in GQL

[FreekVR]

Description

Currently, the Craft implementation of webonyx/graphql-php implements the "defaultRules" of said library. This mostly implements validity checks on whether the query is valid against the schema.

A EVENT_DEFINE_GQL_VALIDATION_RULES-event is available (but not very explicitly documented) to expand on this.

I'd suggest adopting some of the more security-centric rules which are available, by default (on production). Additionally, I think a more well-documented and accessible approach from general.php-config would be helpful.

Some of the rules I'd suggest (mostly to protect against DOS attacks)

• Setting a default "complexion" limit, and defining the complexion for various fields (as per https://webonyx.github.io/graphql-php/security/)
• Setting a default query depth limit
• Disable introspection on production environments (or anything non-development really)
• Setting a default limit (and maximum amount?) of items for "plural" queries (eg sites, entries, users, assets etc)

Perhaps some of these rules are already implemented, but I couldn't find them documented anywhere currently.

[RitterKnightCreative]

I agree with all of the above, though DOS is probably best handled by rate limiting on the server level (eg nginx has a form of it built in, for apache there's mod_security, or using WAF like CloudFlare, StackPath etc.) rather than at the application level.

[FreekVR]

Good point, I figure resource exhaustion would be a better term here ;)

On Tue, 28 Jul 2020, 18:28 RitterKnight, [e-mail] wrote:

I agree with all of the above, though DOS is probably best handled by rate
limiting on the server level (eg nginx has a form of it built in, for
apache there's mod_security, or using WAF like CloudFlare, StackPath etc.)
rather than at the application level.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/craftcms/cms/issues/6466#issuecomment-665141019, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AADF5CB4U3XIYVGGQC72U53R534BDANCNFSM4PKJ4U2A
.

[andris-sevcenko]

This is a great conversation to have, honestly and something I've been mulling about for quite some time.

Before, I had internally dismissed this, because GraphQL eager-loads everything that it knows can be eager-loaded, so I had considered GraphQL to be pretty performant.

Currently, however, there are at least two fields that simply cannot be eager-loaded - prev and next. It's also not a given fact that every plugin-provided relation will be eager-loaded.

So, this leads me to the conclusion that complexity should be factored in.

Here's the plan I have and will be going on ahead with, but it's definitely open for discussion.

• Add maxGraphqlDepth, maxGraphqlComplexity, and maxGraphqlResults config settings. These would default to null to avoid breaking existing queries just by updating to 3.5.
• Set the complexity for prev and next fields on entries to 50, if arguments are used. That should give enough wiggle-room below 50 and I consider 50 to be a pretty good estimate, given that most things are simply properties on objects and cost virtually nothing to access.
• Set the complexity for a few semi-complex fields to 10. Semi-complex properties are, for example, sectionHandle on an entry, where it would cause Craft to load an additional model to resolve the field value.
• Set the complexity for Asset transforms to 200, if the transform used is an unnamed transform. This is because it's an incredibly easy attack vector to just request random transform sizes, which provide inexhaustible variants for a single Asset. Using only named transforms diffuses this vector and can be used a limited number of times (named transform count * asset count).
• In addition to transform complexity, I feel like another config setting should be added - disableGraphqlTransformDirective since complexity cannot be applied to directives, as directives are evaluated post-query-execution.

I'm open to discussion about this, of course, but this is the direction I'm taking for now.

[andris-sevcenko]

Sorry, @FreekVR, I forgot to address one more point:

Disable introspection on production environments (or anything non-development really)

Disabling introspection essentially disables GraphQL API documentation. Also, this is solved rather neatly by enabling the built-int GraphQL caching.

[FreekVR]

Hey @andris-sevcenko ,

Thanks, you make good points and I agree with your solution keeping in mind compatibility. I would suggest maybe shipping some defaults for some of these in 4.x though ;)

With regards to the introspection: It's generally a concern when your GQL-api isn't intended to be consumed by the general public. Sure, you could use the token-based schemas in Craft, but if you're building a single page app, you could easily pick off the token from the XHR request, fire up an introspection query, and easily see all the various things you can get from the API.

It's mostly a "security through obscurity" measure. Making it impossible to see what fields, directives and queries you expose, makes it a lot harder to "guess" what's in there. Perhaps it could be a setting per schema, or otherwise, another config setting like the ones you suggested.

[andris-sevcenko]

I would suggest maybe shipping some defaults for some of these in 4.x though

That definitely makes sense.

It's mostly a "security through obscurity" measure. Making it impossible to see what fields, directives and queries you expose, makes it a lot harder to "guess" what's in there. Perhaps it could be a setting per schema, or otherwise, another config setting like the ones you suggested.

FWIW, the docbloc for the EVENT_DEFINE_GQL_VALIDATION_RULES event has the exact code you would need to add to disable introspection, so I'm not sure if that's worth adding a config setting for.

[andris-sevcenko]

Setting a default limit (and maximum amount?) of items for "plural" queries (eg sites, entries, users, assets etc)

Actually, this does not make much sense.

Consider the following query:

{
  a: entries {
    id
  }

  b: entries {
    id
  }
}

The only sensible way here is to apply the setting to each top-level query, rendering it useless as a security measure, or did you have something else in mind?

[FreekVR]

FWIW, the docbloc for the EVENT_DEFINE_GQL_VALIDATION_RULES event has the exact code you would need to add to disable introspection, so I'm not sure if that's worth adding a config setting for.

Sure, I wasn't sure how many of these rules would make sense for everyone, and I'd be fine to simply write a small plugin/module internally to deal with this too.

The only sensible way here is to apply the setting to each top-level query, rendering it useless as a security measure, or did you have something else in mind?

My main concern is firing a query like that on a huge DB with, say, 300k entries, yes. But it's true that combining multiple queries would work-around that upper-limit per query essentially. But say I define a "100" entry upper limit in my app config, that would still require a large amount of effort and automation rather than just firing entries { id } once to potentially topple the server.

For that particular edge case there are some more measures outlined here though, which would still need to have the complexity and limit implementations to not have a single query be an issue. I'd especially look at the _timeout_ (I think MySQL/PHP settings usually take care of this though) and the _throttling_ chapters here. Also the way Github implements this in their GQL api: https://developer.github.com/v4/guides/resource-limitations/

[andris-sevcenko]

Yeah, I feel like throttling, timeout, and etc. are probably better handled outside of GraphQL layer. At least for the time being.

[andris-sevcenko]

We discussed this internally and decided that we will, also add the maxGraphqlResults option. While it's definitely possible to circumvent this by executing multiple queries, doing so will still count towards the complexity limit and, adding this option, will allow Craft sites to safeguard against queries where the user has simply forgotten to add a limit argument.

Also, we'll be adding the enableGraphqlIntrospection which defaults to true for the simple fact that it's more convenient than doing it with a plugin/module.

[FreekVR]

Hey @andris-sevcenko,

Great, thanks 👍

[andris-sevcenko]

We're pushing this back to Craft 3.6, because we need to bump the webonyx/graphql-php version to 14.x and that requires PHP 7.1. Sorry :/

[andris-sevcenko]

This has been added for Craft on the 3.6 branch.

[brandonkelly]

Craft 3.6 has now been officially released ✨