Unknown operationName error returning unescaped input

[stephanbakker]

Our security scan exposed our graphql endpoint as xss vulnerable, because it is possible to do someting like

{"something":"US"},"operationName":"getNiceList<sCrIpT tYpE=tExT\/vBsCrIpT>MsgBox(21834)<\/sCrIpT>"}

and it returns this unvalidated/unescaped.

[wincent]

Can you provide a little more info on how you're testing/reproducing this? My initial instinct is that this is a transport-level issue and unlikely to be something with the graphql package itself, but I'd like to know more. What server software are you running on your GraphQL endpoint? Is it express-graphql, for example?

Given the above invalid JSON that you provided, express-graphql behaves as follows:

curl -XPOST -H "Content-Type:application/json"  -d '{"something":"US"},"operationName":"getNiceList<sCrIpT tYpE=tExT\/vBsCrIpT>MsgBox(21834)<\/sCrIpT>"}' http://localhost:54904/graphql
{"errors":[{"message":"POST body sent invalid JSON."}]}

If I turn it into valid JSON, then it does this:

curl -XPOST -H "Content-Type:application/json"  -d '{"query": "query Test { isTest }","operationName":"getNiceList<sCrIpT tYpE=tExT\/vBsCrIpT>MsgBox(21834)<\/sCrIpT>"}' http://localhost:54904/graphql
{"errors":[{"message":"Unknown operation named \"getNiceList<sCrIpT tYpE=tExT/vBsCrIpT>MsgBox(21834)</sCrIpT>\"."}]}

Which looks validly encoded to me.

[stephanbakker]

Thanks for quick answer!

Sorry for not providing a proper working example. But your example is exactly our case. This means that if you would inject the error response as is into your DOM, it would be possible to run the script.

If the response would for example be:

{"errors":[{"message":"Unknown operation named \"foo<script>alert(\"oops\")</script>\"."}]}

And if your frontend would insert this error into the dom (In our application we don't btw ;-), it would be possible to actually execute the script.

You could argue that the frontend is responsible for taking care of this. But it might be nice if the error response would already take care of some risks.

What do you think?

If I look at owasp ( https://www.owasp.org/index.php/Testing_for_Reflected_Cross_site_scripting_(OTG-INPVAL-001) ), they state that:

"When a web application is vulnerable to this type of attack, it will pass unvalidated input sent through requests back to the client"

[wincent]

You could argue that the frontend is responsible for taking care of this. But it might be nice if the error response would already take care of some risks.

Yeah, that's my sense. GraphQL is transport agnostic: the fact that JSON over HTTP is often used is a common convention, but not a requirement. If we start baking in assumptions about how the response will be used (ie. dumped into the DOM as in your example) then it is difficult to know where to draw the boundary. If we sanitize output because it might be dumped into the DOM, will we end up having to apply another sanitization for another non-DOM environment, and another, and another?

I'm not trying to squash your concern; I am just cautious about the risks of imposing an overly rigid set of assumptions here. I'm going to add the "discussion" label to gather some more input here.

[JeffRMoore]

I agree that it would be a mistake to perform any kind of encoding or santization because one doesn't know what context the error might be used and thus cannot provide the proper treatment. Any code that processes errors must treat the error strings as unsafe and process accordingly.

It might, however, be possible for GraphQL to make the error return results easier to process. For example, by returning "variables" in error messages as independent JSON data and by providing stable error codes or types (ala https://tools.ietf.org/html/rfc7807)

Not sure if you're running the scan yourself or being audited. I'll presumed audited.

I'd recommend focusing on proving to auditors 1.) mandated GraphQL client properly handles these types of cases. 2.) Systems are in place to prevent use of the API without the approved client. That will be good enough for most.

Really good auditors will start to ask about how logging works and what code might access stored error messages in logs, but in my experience, that's rare. Still, stored XSS via logs is still something to test for.

If you have to, you can replace the error message returned by the server with a reference #. That's a usability hit though.

If you are self scanning, the scanning software usually allows you to "accept" a result and keep it out of further scans. If you are being repeatedly audited, pre-emptively providing this documentation can keep stuff like this out of the "findings" document.

This reflection would be really bad if the server is sending back the wrong Content-Type, so adding some redundancy around making sure that can't happen might be a good idea.

The best conversations with auditors are the ones where you already have the answers to their questions when they ask them. So, you can say "Thats not a problem because I can prove that we only use client X(run report) and that X handles this correctly (here is the test), we've tested our logging stack for stored XSS (here is the test) and I monitor the endpoint for incorrect HTTP headers (see, no alerts)."

[stephanbakker]

Thanks for your comprehensive answer! You guessed right, it's audited by a security test tool, and we have to convince the auditors per case found by the too, if we think our app is not vulnerable. You gave me lots of things to digest and use for us!

[wincent]

Thanks for the discussion on this. I don't think there is anything immediately actionable for us here so I'm going to close it (want to keep the signal on the tracker high), but please feel welcome continue exploring this topic. If we turn up any concrete actions that should be taken on this topic, let's spin up new issues for those.