Unintended security issues with query filter params and schema augmentation

[roschaefer]

Problem

So we have a graphql type User like this:

type User {
  id: ID!
  name: String
  email: String!
  password: String!
}

If you do schema augmentation, you get this nice little filter feature. It let's you do things like:

query {
  User(filter: {email_contains: "something"}) {
    name
  }
}

So even if we make sure that the email stays private by checking if the authenticated user owns the email, an attacker can guess character-by-character the email of a user, simply by checking if the query returns a non-empty result.

Even worse. You could also the same with password. So you can basically make a character-by-character guess of the password hash. Yikes!

We will fix this on our side by simply checking in the augmented schema into version control and then disable schema augmentation altogether. It's just to dangerous to get unpleasant surprises.

So I want to create this issue and ask you if you might consider a re-design of the schema augmentation? Maybe a command line utility which augments the schema and then outputs a file would make sense? Just anything more explicit than the current state.

I actually figured that it is quite beneficial to go through your schema attribute by attribute and really decide what should go into a Update or Create mutation, what should be part of the query and what data should be exposed through the response type.

Opinions?

I also explained the issue on our side, see here.

[Pruxis]

If you want these security concerns to be dealt with why don't you wrap your resolver with some logic? I think that's the best way to solve this because adding this functionality in the neo4j-graphql-js package might cause other filters to work not as intended.

[benjamin-rood]

Hi Robert, you do know that you also can omit certain types from having auto-generated schema functions, correct?

You are absolutely right that it's a potential security risk, and perhaps the immediate solution is to announce that users of the library need to be conscious of how the autogeneration works and that all fields will be exposed as a filtering parameter. This also suggests the need for a directive which blocks it from being included like @noexpose. Libraries like PostGraphile have this sort of thing (it uses @omit, but as a smart comment inside the postgres schema).

But I agree, you have to also take security measures on top, just like you do for any Graphql service.

[roschaefer]

Thank you for the response @benjamin-rood @Pruxis! I know that I can exclude certain types from auto-generated schema functions and I know that I could also try to fix the security issues in the resolver.

From our current experience, it's just the cleanest and probably securest solution to define the graphql schema on our own. We simply have too many write-only (password, RSA privateKey), read-only (createdAt, updatedAt, disabled, deleted) or transient attributes like imageUpload. We can still use neo4jgraphql in the resolvers, if we follow it's naming schema. The downside of this approach is we have to keep the schema up-to-date whenever this library gets updated.

I just wanted to give this user feedback. As far as I know you're still looking for feedback. We're planning to disable schema augmentation at runtime but it would be still helpful to have some schema augmentation, say, at build time. Therefore the suggestion with the command line tool.

If you think this issue is addressed, feel free to close this issue. It looks like you think it's up to the user to avoid unintended exposure of secret attributes.

[benjamin-rood]

@roschaefer I think it's a very worthy thing to consider! Please don't misunderstand, I just wanted to make sure you were aware of the options and the context. Also, I'm not on the Neo team!

I'm just another "Joe Schmoe" developer in the same boat as you. We also disable autogenerated schema in any public staging area, because it's too permissive (and also not tailored enough for us).

The downside of this approach is we have to keep the schema up-to-date whenever this library gets updated.

True. :disappointed:

[emregency]

Hi @roschaefer , I completely understand your concern and it is a very valid point. I have arrived to a similar concern when I realized that my serverless API could be queried millions of times or with recursive nestedness. I decided to limit the number of queries and the depth of query preventing partially a DOS attack. I think the same approach can also help you to prevent from this guessing attack.

This is a nice blog post giving some good practices on securing graphql api.

And another one, more costly probably but also in my option partially securing through whitelisting your client requests with a provider such as Apollo Engine

I am curious to hear how you would be solving it. Let us know.

[johnymontana]

Hey @roschaefer - thanks for pointing this out, I hadn't really considered this possibility when we added filters. I'll update the docs to point this out and options for mitigating (excluding types from the augmentation process and as you are doing, using manually edited augmented type definitions)

I also think your suggestion of a command line tool for generating the augmented type definitions is a great idea. I can see a few other potential use cases for a command line tool, such as:

1. writing augmented type definitions to file
2. writing generated graphql type definitions from existing Neo4j instance to file (wrapping inferSchema)
3. enforcing Neo4j database constraints from type definitions
4. enabling data import

[michaeldgraham]

https://github.com/neo4j-graphql/neo4j-graphql-js/issues/608