Add directive limit to prevent overloading

[uvzz]

I have noticed a similar issue to the one in the graphql-java lib (see CVE-2022-37734) - There is no limit on the number of directives that can be given as input inside the GraphQL query.

Adding a large amount of non-existing directives can significantly increase the query's processing time and response size; hence, sending similar requests with the payload simultaneously can cause a DoS condition on the server.

Let's say that you have a valid GraphQL query"

query {
  user {
    name
  }
}

modify it so it will have many non-existing directives, e.g.:

query {
  user @aa @aa @aa ... {
    name
  }
} 

Replace ... with as many @aa as you need. The more you add, the larger the response size and time processed.

As a fix I limited the number of allowed directives to 10, but this can be changed of course.

Once the issue is fixed, could you add an advisory under the security tab?

The CVE ID is CVE-2023-49559

[coveralls]

coverage: 88.667% (-0.02%) from 88.689% when pulling 97b793f8253f1a501cc8510315c662e5bbc5b730 on uvzz:directive-limit-fix-CVE-2023-49559 into bf1736ae2956d714094fd06e76c43888fb598867 on vektah:master.

[StevenACoffman]

Thanks for making this PR!

My concern with this implementation is that there's no way to configure the limit to be more than 10. Also, at a casual glance, I'm not sure how the directive limit counting works. For instance, would a query like this also exceed the 10 limit:

query {
  user @aa {
    name @aa
  }
  me @aa {
    name @aa
  }
 friendsConnection(first: $first) @aa {
    totalCount @aa
    edges @aa {
      node @aa {
        name @aa
        friendliness @aa
        approachableness @aa
      }
    }
  }
} 

Or if the directives were each different, like @aa @ab @ac and so forth?

[uvzz]

Hi,

The current implementation counts the total number of directives in the query, regardless of their uniqueness.

As for the limit, we can use a global variable maybe, that will be configured from a configuration file, also from gqlgen, but I couldn't find such a file currently at use.

We can also use an environment variable, e.g:

limitStr := os.Getenv("GQL_DIRECTIVE_LIMIT")

What do you think?

On Wed, Jan 3, 2024 at 5:38 PM Steve Coffman @.***> wrote:

Thanks for making this PR!

My concern with this implementation is that there's no way to configure the limit to be more than 10. Also, at a casual glance, I'm not sure how the limit would work. For instance, would a query like this also exceed the limit:

query { user @aa { name @aa } me @aa { name @aa } friendsConnection(first: $first) @aa { totalCount @aa edges @aa { node @aa { name @aa friendliness @aa approachableness @aa } } } }

Or if the directives were each different, like @aa @ab @ac and so forth?

— Reply to this email directly, view it on GitHub https://github.com/vektah/gqlparser/pull/287#issuecomment-1875565529, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIRNXV7EP6DFGYR66WRHXDLYMV3PNAVCNFSM6AAAAABBLOZQG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZVGU3DKNJSHE . You are receiving this because you authored the thread.Message ID: @.***>

[benjaminjkraft]

Thanks for the PR! (For context, I'm one of the maintainers of genqlient, which uses gqlparser.)

A few thoughts:

First, why are directives special? Do we have protections against other types of DOS?

For example, what if I just send a very large query query q { a { b { a { b ... } } } }? (I would guess that the majority of GraphQL schemas contain such a loop.) What about many fragments? (Potentially you can even get exponential blowup that way.) What about a query with millions of characters of whitespace? I think the Java folks already have query depth/complexity limits, but we don't (at least not in the parser; gqlgen has some). If we need them, we need them all.

Indeed, it looks like over in Java what they actually did was limit the number of tokens they parse. (See https://github.com/graphql-java/graphql-java/pull/2892. They also have similar limits for comments/whitespace.) That seems a lot simpler and more general. Or, it may even suffice to simply limit the request size. That doesn't need to be in the parser at all -- many HTTP servers already do so or callers could do so via middleware -- so it could just be something to document, or for server libraries to have a simple default which users who want something more complex (e.g. different limits for different users) can replace with such middleware. Of course, public GraphQL servers may also wish to limit query complexity in other ways, as they can do via gqlgen's complexity APIs.

Second, to me this is really the concern of a server library, not a parser. For client libraries, static analysis tools, etc., most users won't care to have any such limits, because they trust their queries. So while it may make sense to implement in the parser (if it needs to be a directive-specific limit), the limit should be configured by callers of the parser (i.e. gqlgen, genqlient, etc.). Many of these tools have their own configuration files, extension points, etc., where they can more cleanly expose this to the user. For server libraries, the default should likely be finite; for client libraries and static analysis tools it may be infinite.

Finally, if we do end up with a default (here, or in gqlgen), I'd suggest making it much much larger. Especially with programmatic queries like those from GraphQL federation, they can get quite large. It looks like Java went for 15k tokens, which seems plausible; if I were choosing I might even go for 100k.

[StevenACoffman]

I think that the parser (parser.go) struct should contain the default limits, and they should default to zero, where zero means no limit. We can expose a method to set this (or these) limits so that gqlgen, Khan/genqlient, infiotinc/gqlgenc, Yamashou/gqlgenc, Code-Hex/gqldoc and all the other programs that use this library can decide what is appropriate to them.

Having an optional directive limit in this way makes it easier for us to add query depth/complexity limits, token limits, and other denial-of-service protections that are useful for publicly exposed APIs, but wouldn't cause a problem for uses where that's not a concern.

WDYT?

[uvzz]

Hi,

I agree that limiting the amount of tokens in parser.go with a default value, and being able to configure it from gqlgen is the right thing to do,

but IMO defaulting the value to 0 (no limit) means that the library will be vulnerable to DoS attacks by default, so it would be better to set a value like 15k.

On Fri, Jan 5, 2024 at 4:36 PM Steve Coffman @.***> wrote:

I think that the parser (parser.go) struct should contain the default limits, and they should default to zero, where zero means no limit. We can expose a method to set this (or these) limits so that gqlgen https://github.com/99designs/gqlgen, Khan/genqlient https://github.com/Khan/genqlient, infiotinc/gqlgenc https://github.com/infiotinc/gqlgenc, Yamashou/gqlgenc https://github.com/Yamashou/gqlgenc, Code-Hex/gqldoc https://github.com/Code-Hex/gqldoc and all the other programs that use this library https://github.com/99designs/gqlgen-contrib can decide what is appropriate to them.

Having an optional directive limit in this way makes it easier for us to add query depth/complexity limits, token limits, and other denial-of-service protections that are useful for publicly exposed APIs, but wouldn't cause a problem for uses where that's not a concern.

WDYT?

— Reply to this email directly, view it on GitHub https://github.com/vektah/gqlparser/pull/287#issuecomment-1878778964, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIRNXV2HQORECYLBJQKOE7LYNAFX5AVCNFSM6AAAAABBLOZQG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZYG43TQOJWGQ . You are receiving this because you authored the thread.Message ID: @.***>