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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
JS: Add Permissive CORS query (CWE-942) #14342
base: main
Are you sure you want to change the base?
Conversation
...ipt/ql/lib/semmle/javascript/security/dataflow/CorsPermissiveConfigurationCustomizations.qll
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a first review. I've only really looked at the implementation, and I haven't taken a good look at the actual vulnerability yet.
| private API::Node apollo() { | ||
| result = | ||
| API::moduleImport([ | ||
| "@apollo/server", "apollo/server", "@apollo/apollo-server-express", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is apollo/server a package? I see that @apollo/server is definitely, but searching npm I don't see a apollo/server package.
| /** A string-valued expression that is interpreted as a Apollo GraphQL query. */ | ||
| abstract class GraphQLString extends DataFlow::Node { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not using this class, I think you can just delete it.
Further down you're using the GraphQLString class from the GraphQL file (as you should).
| abstract class GraphQLString extends DataFlow::Node { } | ||
|
|
||
| /** A string-valued expression that is interpreted as a Apollo GraphQL query. */ | ||
| abstract class ApolloGraphQLServer extends DataFlow::Node { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have this in a separate module, instead of inside the Apollo module?
| /** | ||
| * Provides classes modeling the apollo packages [@apollo/server](https://npmjs.com/package/@apollo/server`) | ||
| */ | ||
| private module Apollo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this module is correctly named.
However, I think you should consider renaming the file to Apollo.qll, because that follows how the other files are named.
| /** Get an instanceof of `gql` */ | ||
| private API::Node gql() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /** Get an instanceof of `gql` */ | |
| private API::Node gql() { | |
| /** Get an instanceof of the `gql` function that parses GraphQL strings. */ | |
| private API::Node gql() { |
Optional suggestion. I feel that that the doc-string deserves more detail.
| } | ||
|
|
||
| /** A string that is interpreted as a GraphQL query by a `graphql` package. */ | ||
| private class ApolloServer extends ApolloGraphQL::ApolloGraphQLServer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private class ApolloServer extends ApolloGraphQL::ApolloGraphQLServer { | |
| private class ApolloServer extends ApolloGraphQL::ApolloGraphQLServer, API::NewNode { |
By adding API::NewNode you should no longer need the inline cast you have in this file and in CorsPermissiveConfigurationCustomizations.qll.
|
|
||
| import javascript | ||
|
|
||
| module CorsPermissiveConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add a documentation string to this module.
I know it feels redundant, and it largely is.
You can see how we do in similar files.
| } | ||
|
|
||
| /** true and null are considered bad values */ | ||
| class BadValues extends Source instanceof DataFlow::Node { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| class BadValues extends Source instanceof DataFlow::Node { | |
| class BadValues extends Source { |
Adding DataFlow::Node should make no difference here.
| @@ -0,0 +1,20 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the query (and the implementation) should perhaps be in the experimental folder for now: https://github.com/github/codeql/tree/main/javascript/ql/src/experimental/Security
You have also not yet added a .qhelp file that can help explain what the vulnerability is about and how to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
| select sink.getNode(), source, sink, "$@ misconfiguration due to a $@.", sink.getNode(), | ||
| "CORS Origin", source.getNode(), "too permissive or user controlled value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| select sink.getNode(), source, sink, "$@ misconfiguration due to a $@.", sink.getNode(), | |
| "CORS Origin", source.getNode(), "too permissive or user controlled value" | |
| select sink.getNode(), source, sink, "CORS Origin misconfiguration due to a $@.", source.getNode(), "too permissive or user controlled value" |
The automated warning is saying that the alert-location is already the sink, so adding a link to the same location as the alert location doesn't really make sense.
This suggestion removes that link.
|
QHelp previews: javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.qhelpoverly CORS configurationA server can use RecommendationWhen the On the other hand, if the If the ExampleIn the example below, the import { ApolloServer } from 'apollo-server';
var https = require('https'),
url = require('url');
var server = https.createServer(function () { });
server.on('request', function (req, res) {
// BAD: origin is too permissive
const server_1 = new ApolloServer({
cors: { origin: true }
});
let user_origin = url.parse(req.url, true).query.origin;
// BAD: CORS is controlled by user
const server_2 = new ApolloServer({
cors: { origin: user_origin }
});
});In the example below, the import { ApolloServer } from 'apollo-server';
var https = require('https'),
url = require('url');
var server = https.createServer(function () { });
server.on('request', function (req, res) {
// GOOD: origin is restrictive
const server_1 = new ApolloServer({
cors: { origin: false }
});
let user_origin = url.parse(req.url, true).query.origin;
// GOOD: user data is properly sanitized
const server_2 = new ApolloServer({
cors: { origin: (user_origin === "https://allowed1.com" || user_origin === "https://allowed2.com") ? user_origin : false }
});
});References
|
This pull request adds a query for Permissive CORS to prevent CSRF attacks for Apollo Server. I plan to add a couple more libraries, so I'll leave it in draft for now. 馃榿
Looking forward to your suggestions.