diff --git a/javascript/ql/lib/javascript.qll b/javascript/ql/lib/javascript.qll index 207336b1cca..370655df72c 100644 --- a/javascript/ql/lib/javascript.qll +++ b/javascript/ql/lib/javascript.qll @@ -71,7 +71,7 @@ import semmle.javascript.frameworks.ActionsLib import semmle.javascript.frameworks.Angular2 import semmle.javascript.frameworks.AngularJS import semmle.javascript.frameworks.Anser -import semmle.javascript.frameworks.ApolloGraphQL +import semmle.javascript.frameworks.Apollo import semmle.javascript.frameworks.AsyncPackage import semmle.javascript.frameworks.AWS import semmle.javascript.frameworks.Azure diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Apollo.qll b/javascript/ql/lib/semmle/javascript/frameworks/Apollo.qll new file mode 100644 index 00000000000..8c09cca9124 --- /dev/null +++ b/javascript/ql/lib/semmle/javascript/frameworks/Apollo.qll @@ -0,0 +1,36 @@ +/** + * Provides classes for working with Apollo GraphQL connectors. + */ + +import javascript + +/** Provides classes modeling the apollo packages [@apollo/server](https://npmjs.com/package/@apollo/server`) */ +module Apollo { + /** Get an instanceof of `Apollo` */ + private API::Node apollo() { + result = + API::moduleImport([ + "@apollo/server", "@apollo/apollo-server-express", "@apollo/apollo-server-core", + "apollo-server", "apollo-server-express" + ]).getMember("ApolloServer") + } + + /** Get an instanceof of the `gql` function that parses GraphQL strings. */ + private API::Node gql() { + result = + API::moduleImport([ + "@apollo/server", "@apollo/apollo-server-express", "@apollo/apollo-server-core", + "apollo-server", "apollo-server-express" + ]).getMember("gql") + } + + /** A string that is interpreted as a GraphQL query by a `graphql` package. */ + class ApolloServer extends API::NewNode { + ApolloServer() { this = apollo().getAnInstantiation() } + } + + /** A string that is interpreted as a GraphQL query by a `apollo` package. */ + class ApolloGraphQLString extends GraphQL::GraphQLString { + ApolloGraphQLString() { this = gql().getACall().getArgument(0) } + } +} diff --git a/javascript/ql/lib/semmle/javascript/frameworks/ApolloGraphQL.qll b/javascript/ql/lib/semmle/javascript/frameworks/ApolloGraphQL.qll deleted file mode 100644 index 9dea34df3b0..00000000000 --- a/javascript/ql/lib/semmle/javascript/frameworks/ApolloGraphQL.qll +++ /dev/null @@ -1,56 +0,0 @@ -/** - * Provides classes for working with Apollo GraphQL connectors. - */ - -import javascript - -/** Provides classes modeling concepts of Apollo GraphQL. */ -module ApolloGraphQL { - /** A string-valued expression that is interpreted as a Apollo GraphQL query. */ - abstract class GraphQLString extends DataFlow::Node { } - - /** A string-valued expression that is interpreted as a Apollo GraphQL query. */ - abstract class ApolloGraphQLServer extends DataFlow::Node { } -} - -/** - * Provides classes modeling the apollo packages [@apollo/server](https://npmjs.com/package/@apollo/server`) - */ -private module Apollo { - /** Get an instanceof of `Apollo` */ - private API::Node apollo() { - result = - API::moduleImport([ - "@apollo/server", "apollo/server", "@apollo/apollo-server-express", - "@apollo/apollo-server-core", "apollo-server", "apollo-server-express" - ]).getMember("ApolloServer") - } - - /** Get an instanceof of `gql` */ - private API::Node gql() { - result = - API::moduleImport([ - "@apollo/server", "apollo/server", "@apollo/apollo-server-express", - "@apollo/apollo-server-core", "apollo-server", "apollo-server-express" - ]).getMember("gql") - } - - /** A string that is interpreted as a GraphQL query by a `octokit` package. */ - private class ApolloGraphQLString extends GraphQL::GraphQLString { - ApolloGraphQLString() { this = gql().getACall() } - } - - /** A string that is interpreted as a GraphQL query by a `graphql` package. */ - private class ApolloServer extends ApolloGraphQL::ApolloGraphQLServer { - ApolloServer() { this = apollo().getAnInstantiation() } - - predicate isPermissive() { - this.(DataFlow::NewNode) - .getOptionArgument(0, "cors") - .getALocalSource() - .getAPropertyWrite("origin") - .getRhs() - .mayHaveBooleanValue(true) - } - } -} diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/CorsPermissiveConfigurationCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/CorsPermissiveConfigurationCustomizations.qll index dc87acc8a1d..0f56bea4347 100644 --- a/javascript/ql/lib/semmle/javascript/security/dataflow/CorsPermissiveConfigurationCustomizations.qll +++ b/javascript/ql/lib/semmle/javascript/security/dataflow/CorsPermissiveConfigurationCustomizations.qll @@ -27,8 +27,8 @@ module CorsPermissiveConfiguration { RemoteFlowSourceAsSource() { not this instanceof ClientSideRemoteFlowSource } } - /** true and null are considered bad values */ - class BadValues extends Source instanceof DataFlow::Node { + /** An overfly permissive value for `origin` */ + class BadValues extends Source { BadValues() { this.mayHaveBooleanValue(true) or this.asExpr() instanceof NullLiteral } } @@ -37,13 +37,9 @@ module CorsPermissiveConfiguration { */ class CorsApolloServer extends Sink, DataFlow::ValueNode { CorsApolloServer() { - exists(ApolloGraphQL::ApolloGraphQLServer agql | + exists(Apollo::ApolloServer agql | this = - agql.(DataFlow::NewNode) - .getOptionArgument(0, "cors") - .getALocalSource() - .getAPropertyWrite("origin") - .getRhs() + agql.getOptionArgument(0, "cors").getALocalSource().getAPropertyWrite("origin").getRhs() ) } } diff --git a/javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.qhelp b/javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.qhelp new file mode 100644 index 00000000000..fc79eee743b --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.qhelp @@ -0,0 +1,71 @@ + + + + +

+ + A server can use CORS (Cross-Origin Resource Sharing) to relax the + restrictions imposed by the SOP (Same-Origin Policy), allowing controlled, secure + cross-origin requests when necessary. + + A server with an overly permissive CORS configuration may inadvertently + expose sensitive data or lead to CSRF which is an attack that allows attackers to trick + users into performing unwanted operations in websites they're authenticated to. + +

+ + + + +

+ + When the origin is set to true, it signifies that the server + is accepting requests from any origin, potentially exposing the system to + CSRF attacks. This can be fixed using false as origin value or using a whitelist. + +

+

+ + On the other hand, if the origin is + set to null, it can be exploited by an attacker to deceive a user into making + requests from a null origin form, often hosted within a sandboxed iframe. + +

+ +

+ + If the origin value is user controlled, make sure that the data + is properly sanitized. + +

+ + + +

+ + In the example below, the server_1 accepts requests from any origin + since the value of origin is set to true. + And server_2's origin is user-controlled. + +

+ + + +

+ + In the example below, the server_1 CORS is restrictive so it's not + vulnerable to CSRF attacks. And server_2's is using properly sanitized + user-controlled data. + +

+ + + + + +
  • Mozilla Developer Network: CORS, Access-Control-Allow-Origin.
    • +
  • W3C: CORS for developers, Advice for Resource Owners
    • +     +     diff --git a/javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.ql b/javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.ql similarity index 76% rename from javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.ql rename to javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.ql index 1cb01c9fe1c..7655b6dda8d 100644 --- a/javascript/ql/src/Security/CWE-942/CorsPermissiveConfiguration.ql +++ b/javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.ql @@ -16,5 +16,5 @@ import DataFlow::PathGraph from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink where cfg.hasFlowPath(source, sink) -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" diff --git a/javascript/ql/src/experimental/Security/CWE-942/examples/CorsPermissiveConfigurationBad.js b/javascript/ql/src/experimental/Security/CWE-942/examples/CorsPermissiveConfigurationBad.js new file mode 100644 index 00000000000..68317486a99 --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-942/examples/CorsPermissiveConfigurationBad.js @@ -0,0 +1,18 @@ +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 } + }); +}); \ No newline at end of file diff --git a/javascript/ql/src/experimental/Security/CWE-942/examples/CorsPermissiveConfigurationGood.js b/javascript/ql/src/experimental/Security/CWE-942/examples/CorsPermissiveConfigurationGood.js new file mode 100644 index 00000000000..5e084d089b7 --- /dev/null +++ b/javascript/ql/src/experimental/Security/CWE-942/examples/CorsPermissiveConfigurationGood.js @@ -0,0 +1,18 @@ +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 } + }); +}); \ No newline at end of file diff --git a/javascript/ql/test/experimental/Security/CWE-942/CorsPermissiveConfiguration.expected b/javascript/ql/test/experimental/Security/CWE-942/CorsPermissiveConfiguration.expected new file mode 100644 index 00000000000..f6571bd7e87 --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-942/CorsPermissiveConfiguration.expected @@ -0,0 +1,34 @@ +nodes +| tst.js:8:9:8:59 | user_origin | +| tst.js:8:23:8:46 | url.par ... , true) | +| tst.js:8:23:8:52 | url.par ... ).query | +| tst.js:8:23:8:59 | url.par ... .origin | +| tst.js:8:33:8:39 | req.url | +| tst.js:8:33:8:39 | req.url | +| tst.js:8:42:8:45 | true | +| tst.js:8:42:8:45 | true | +| tst.js:11:25:11:28 | true | +| tst.js:11:25:11:28 | true | +| tst.js:11:25:11:28 | true | +| tst.js:21:25:21:28 | null | +| tst.js:21:25:21:28 | null | +| tst.js:21:25:21:28 | null | +| tst.js:26:25:26:35 | user_origin | +| tst.js:26:25:26:35 | user_origin | +edges +| tst.js:8:9:8:59 | user_origin | tst.js:26:25:26:35 | user_origin | +| tst.js:8:9:8:59 | user_origin | tst.js:26:25:26:35 | user_origin | +| tst.js:8:23:8:46 | url.par ... , true) | tst.js:8:23:8:52 | url.par ... ).query | +| tst.js:8:23:8:52 | url.par ... ).query | tst.js:8:23:8:59 | url.par ... .origin | +| tst.js:8:23:8:59 | url.par ... .origin | tst.js:8:9:8:59 | user_origin | +| tst.js:8:33:8:39 | req.url | tst.js:8:23:8:46 | url.par ... , true) | +| tst.js:8:33:8:39 | req.url | tst.js:8:23:8:46 | url.par ... , true) | +| tst.js:8:42:8:45 | true | tst.js:8:23:8:46 | url.par ... , true) | +| tst.js:8:42:8:45 | true | tst.js:8:23:8:46 | url.par ... , true) | +| tst.js:11:25:11:28 | true | tst.js:11:25:11:28 | true | +| tst.js:21:25:21:28 | null | tst.js:21:25:21:28 | null | +#select +| tst.js:11:25:11:28 | true | tst.js:11:25:11:28 | true | tst.js:11:25:11:28 | true | CORS Origin misconfiguration due to a $@. | tst.js:11:25:11:28 | true | too permissive or user controlled value | +| tst.js:21:25:21:28 | null | tst.js:21:25:21:28 | null | tst.js:21:25:21:28 | null | CORS Origin misconfiguration due to a $@. | tst.js:21:25:21:28 | null | too permissive or user controlled value | +| tst.js:26:25:26:35 | user_origin | tst.js:8:33:8:39 | req.url | tst.js:26:25:26:35 | user_origin | CORS Origin misconfiguration due to a $@. | tst.js:8:33:8:39 | req.url | too permissive or user controlled value | +| tst.js:26:25:26:35 | user_origin | tst.js:8:42:8:45 | true | tst.js:26:25:26:35 | user_origin | CORS Origin misconfiguration due to a $@. | tst.js:8:42:8:45 | true | too permissive or user controlled value | diff --git a/javascript/ql/test/experimental/Security/CWE-942/CorsPermissiveConfiguration.qlref b/javascript/ql/test/experimental/Security/CWE-942/CorsPermissiveConfiguration.qlref new file mode 100644 index 00000000000..1e6a39679c0 --- /dev/null +++ b/javascript/ql/test/experimental/Security/CWE-942/CorsPermissiveConfiguration.qlref @@ -0,0 +1 @@ +./experimental/Security/CWE-942/CorsPermissiveConfiguration.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-942/tst.js b/javascript/ql/test/experimental/Security/CWE-942/tst.js similarity index 80% rename from javascript/ql/test/query-tests/Security/CWE-942/tst.js rename to javascript/ql/test/experimental/Security/CWE-942/tst.js index 60040e96537..f55d5dc2c3e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-942/tst.js +++ b/javascript/ql/test/experimental/Security/CWE-942/tst.js @@ -6,28 +6,23 @@ var server = https.createServer(function () { }); server.on('request', function (req, res) { let user_origin = url.parse(req.url, true).query.origin; - // BAD: attacker can choose the value of origin + // BAD: CORS too permissive const server_1 = new ApolloServer({ cors: { origin: true } }); - // BAD: CORS too permissive - const server_2 = new ApolloServer({ - cors: { origin: true } - }); - // GOOD: restrictive CORS - const server_3 = new ApolloServer({ + const server_2 = new ApolloServer({ cors: false }); // BAD: CORS too permissive - const server_4 = new ApolloServer({ + const server_3 = new ApolloServer({ cors: { origin: null } }); // BAD: CORS is controlled by user - const server_5 = new ApolloServer({ + const server_4 = new ApolloServer({ cors: { origin: user_origin } }); }); \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.expected b/javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.expected deleted file mode 100644 index 7862049eeed..00000000000 --- a/javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.expected +++ /dev/null @@ -1,39 +0,0 @@ -nodes -| tst.js:8:9:8:59 | user_origin | -| tst.js:8:23:8:46 | url.par ... , true) | -| tst.js:8:23:8:52 | url.par ... ).query | -| tst.js:8:23:8:59 | url.par ... .origin | -| tst.js:8:33:8:39 | req.url | -| tst.js:8:33:8:39 | req.url | -| tst.js:8:42:8:45 | true | -| tst.js:8:42:8:45 | true | -| tst.js:11:25:11:28 | true | -| tst.js:11:25:11:28 | true | -| tst.js:11:25:11:28 | true | -| tst.js:16:25:16:28 | true | -| tst.js:16:25:16:28 | true | -| tst.js:16:25:16:28 | true | -| tst.js:26:25:26:28 | null | -| tst.js:26:25:26:28 | null | -| tst.js:26:25:26:28 | null | -| tst.js:31:25:31:35 | user_origin | -| tst.js:31:25:31:35 | user_origin | -edges -| tst.js:8:9:8:59 | user_origin | tst.js:31:25:31:35 | user_origin | -| tst.js:8:9:8:59 | user_origin | tst.js:31:25:31:35 | user_origin | -| tst.js:8:23:8:46 | url.par ... , true) | tst.js:8:23:8:52 | url.par ... ).query | -| tst.js:8:23:8:52 | url.par ... ).query | tst.js:8:23:8:59 | url.par ... .origin | -| tst.js:8:23:8:59 | url.par ... .origin | tst.js:8:9:8:59 | user_origin | -| tst.js:8:33:8:39 | req.url | tst.js:8:23:8:46 | url.par ... , true) | -| tst.js:8:33:8:39 | req.url | tst.js:8:23:8:46 | url.par ... , true) | -| tst.js:8:42:8:45 | true | tst.js:8:23:8:46 | url.par ... , true) | -| tst.js:8:42:8:45 | true | tst.js:8:23:8:46 | url.par ... , true) | -| tst.js:11:25:11:28 | true | tst.js:11:25:11:28 | true | -| tst.js:16:25:16:28 | true | tst.js:16:25:16:28 | true | -| tst.js:26:25:26:28 | null | tst.js:26:25:26:28 | null | -#select -| tst.js:11:25:11:28 | true | tst.js:11:25:11:28 | true | tst.js:11:25:11:28 | true | $@ misconfiguration due to a $@. | tst.js:11:25:11:28 | true | CORS Origin | tst.js:11:25:11:28 | true | too permissive or user controlled value | -| tst.js:16:25:16:28 | true | tst.js:16:25:16:28 | true | tst.js:16:25:16:28 | true | $@ misconfiguration due to a $@. | tst.js:16:25:16:28 | true | CORS Origin | tst.js:16:25:16:28 | true | too permissive or user controlled value | -| tst.js:26:25:26:28 | null | tst.js:26:25:26:28 | null | tst.js:26:25:26:28 | null | $@ misconfiguration due to a $@. | tst.js:26:25:26:28 | null | CORS Origin | tst.js:26:25:26:28 | null | too permissive or user controlled value | -| tst.js:31:25:31:35 | user_origin | tst.js:8:33:8:39 | req.url | tst.js:31:25:31:35 | user_origin | $@ misconfiguration due to a $@. | tst.js:31:25:31:35 | user_origin | CORS Origin | tst.js:8:33:8:39 | req.url | too permissive or user controlled value | -| tst.js:31:25:31:35 | user_origin | tst.js:8:42:8:45 | true | tst.js:31:25:31:35 | user_origin | $@ misconfiguration due to a $@. | tst.js:31:25:31:35 | user_origin | CORS Origin | tst.js:8:42:8:45 | true | too permissive or user controlled value | diff --git a/javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.qlref b/javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.qlref deleted file mode 100644 index f8adf3cd38e..00000000000 --- a/javascript/ql/test/query-tests/Security/CWE-942/CorsPermissiveConfiguration.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE-942/CorsPermissiveConfiguration.ql