-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JS: Add Permissive CORS query (CWE-942) #14342
Changes from 17 commits
e171123
142ab01
816eebb
ed06628
c0e6d7c
07ad596
acac534
d661f7f
413c111
abd53e9
4ef4c92
aa24ce5
bb6ef72
f623db4
3bcb411
6a3cdc9
e6c7fc0
83cbbd7
87cac2a
4f68f60
191766a
7662b2b
78e7793
699d8d4
c1fd7a6
f2d6640
cfd7c7a
e96c3a3
4be5cf4
8ba7ac6
d0cf2a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
--- | ||
category: minorAnalysis | ||
--- | ||
* Added a new experimental query, `js/cors-misconfiguration` covering `cors` and `apollo`. | ||
maikypedia marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
/** | ||
* Provides classes for working with Apollo GraphQL connectors. | ||
*/ | ||
|
||
import javascript | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is only used within your experimental query, so it should probably be moved to the same folder. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done 👍 |
||
|
||
/** Provides classes modeling the apollo packages [@apollo/server](https://npmjs.com/package/@apollo/server`) */ | ||
module Apollo { | ||
maikypedia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** Get a reference to the `ApolloServer` class. */ | ||
private API::Node apollo() { | ||
result = | ||
API::moduleImport([ | ||
"@apollo/server", "@apollo/apollo-server-express", "@apollo/apollo-server-core", | ||
"apollo-server", "apollo-server-express" | ||
]).getMember("ApolloServer") | ||
} | ||
|
||
/** Gets a reference to 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. */ | ||
maikypedia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
class ApolloServer extends API::NewNode { | ||
ApolloServer() { this = apollo().getAnInstantiation() } | ||
} | ||
|
||
/** A string that is interpreted as a GraphQL query by a `apollo` package. */ | ||
private class ApolloGraphQLString extends GraphQL::GraphQLString { | ||
ApolloGraphQLString() { this = gql().getACall().getArgument(0) } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/** | ||
* Provides classes for working with Cors connectors. | ||
*/ | ||
|
||
import javascript | ||
|
||
/** Provides classes modeling [cors](https://npmjs.com/package/cors) library. */ | ||
maikypedia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
module Cors { | ||
/** | ||
* An expression that creates a new CORS configuration. | ||
*/ | ||
class Cors extends DataFlow::CallNode { | ||
Cors() { this = DataFlow::moduleImport("cors").getAnInvocation() } | ||
|
||
/** Get the options used to configure Cors */ | ||
DataFlow::Node getOptionsArgument() { result = this.getArgument(0) } | ||
|
||
/** Holds if cors is using default configuration */ | ||
predicate isDefault() { this.getNumArgument() = 0 } | ||
|
||
/** Gets the value of origin */ | ||
maikypedia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
DataFlow::Node getOrigin() { result = this.getOptionArgument(0, "origin") } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
import javascript | ||
import semmle.javascript.frameworks.HTTP | ||
import semmle.javascript.frameworks.ExpressModules | ||
import semmle.javascript.frameworks.Cors | ||
private import semmle.javascript.dataflow.InferredTypes | ||
private import semmle.javascript.frameworks.ConnectExpressShared::ConnectExpressShared | ||
|
||
|
@@ -1071,4 +1072,23 @@ module Express { | |
|
||
override predicate definitelyResumesDispatch() { none() } | ||
} | ||
|
||
/** | ||
* An express route setup configured with the `cors` package. | ||
*/ | ||
class CorsConfiguration extends DataFlow::MethodCallNode { | ||
CorsConfiguration() { exists(Express::RouteSetup setup | this = setup | setup.isUseCall()) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This characteristic predicate holds for any I think you should add Additionally (as I have mentioned before). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could it be simplified to just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite. I've added some suggestions to use a field such that the result of |
||
|
||
/** Gets the cors argument */ | ||
Cors::Cors getArgument() { result = this.getArgument(0) } | ||
|
||
/** Gets the options used to configure `cors`. */ | ||
DataFlow::Node getCorsArgument() { result = this.getArgument().getOptionsArgument() } | ||
|
||
/** Holds if cors is using its default configuration. */ | ||
predicate isDefault() { this.getArgument().isDefault() } | ||
|
||
/** Gets the `origin` option that the call to `cors` is configured with. */ | ||
DataFlow::Node getOrigin() { result = this.getArgument().getOrigin() } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
<!DOCTYPE qhelp PUBLIC | ||
"-//Semmle//qhelp//EN" | ||
"qhelp.dtd"> | ||
<qhelp> | ||
|
||
<overview> | ||
<p> | ||
|
||
A server can use <code>CORS</code> (Cross-Origin Resource Sharing) to relax the | ||
restrictions imposed by the <code>SOP</code> (Same-Origin Policy), allowing controlled, secure | ||
cross-origin requests when necessary. | ||
|
||
A server with an overly permissive <code>CORS</code> configuration may inadvertently | ||
expose sensitive data or lead to <code>CSRF</code> which is an attack that allows attackers to trick | ||
users into performing unwanted operations in websites they're authenticated to. | ||
|
||
</p> | ||
|
||
</overview> | ||
|
||
<recommendation> | ||
<p> | ||
|
||
When the <code>origin</code> is set to <code>true</code>, it signifies that the server | ||
is accepting requests from <code>any</code> origin, potentially exposing the system to | ||
CSRF attacks. This can be fixed using <code>false</code> as origin value or using a whitelist. | ||
|
||
</p> | ||
<p> | ||
|
||
On the other hand, if the <code>origin</code> is | ||
set to <code>null</code>, it can be exploited by an attacker to deceive a user into making | ||
requests from a <code>null</code> origin form, often hosted within a sandboxed iframe. | ||
|
||
</p> | ||
|
||
<p> | ||
|
||
If the <code>origin</code> value is user controlled, make sure that the data | ||
is properly sanitized. | ||
|
||
</p> | ||
</recommendation> | ||
|
||
<example> | ||
<p> | ||
|
||
In the example below, the <code>server_1</code> accepts requests from any origin | ||
since the value of <code>origin</code> is set to <code>true</code>. | ||
And <code>server_2</code>'s origin is user-controlled. | ||
|
||
</p> | ||
|
||
<sample src="examples/CorsPermissiveConfigurationBad.js"/> | ||
|
||
<p> | ||
|
||
In the example below, the <code>server_1</code> CORS is restrictive so it's not | ||
vulnerable to CSRF attacks. And <code>server_2</code>'s is using properly sanitized | ||
user-controlled data. | ||
|
||
</p> | ||
|
||
<sample src="examples/CorsPermissiveConfigurationGood.js"/> | ||
</example> | ||
|
||
<references> | ||
<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin">CORS, Access-Control-Allow-Origin</a>.</li> | ||
<li>W3C: <a href="https://w3c.github.io/webappsec-cors-for-developers/#resources">CORS for developers, Advice for Resource Owners</a></li> | ||
</references> | ||
</qhelp> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/** | ||
* @name overly CORS configuration | ||
* @description Misconfiguration of CORS HTTP headers allows CSRF attacks. | ||
* @kind path-problem | ||
* @problem.severity error | ||
* @security-severity 7.5 | ||
* @precision high | ||
* @id js/cors-misconfiguration | ||
* @tags security | ||
* external/cwe/cwe-942 | ||
*/ | ||
|
||
import javascript | ||
import CorsPermissiveConfigurationQuery | ||
import DataFlow::PathGraph | ||
|
||
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink | ||
where cfg.hasFlowPath(source, sink) | ||
select sink.getNode(), source, sink, "CORS Origin misconfiguration due to a $@.", source.getNode(), | ||
"too permissive or user controlled value" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/** | ||
* Provides default sources, sinks and sanitizers for reasoning about | ||
* overly permissive CORS configurations, as well as | ||
* extension points for adding your own. | ||
*/ | ||
|
||
import javascript | ||
|
||
/** Module containing sources, sinks, and sanitizers for overly permissive CORS configurations. */ | ||
module CorsPermissiveConfiguration { | ||
/** | ||
* A data flow source for permissive CORS configuration. | ||
*/ | ||
abstract class Source extends DataFlow::Node { } | ||
|
||
/** | ||
* A data flow sink for permissive CORS configuration. | ||
*/ | ||
abstract class Sink extends DataFlow::Node { } | ||
|
||
/** | ||
* A sanitizer for permissive CORS configuration. | ||
*/ | ||
abstract class Sanitizer extends DataFlow::Node { } | ||
|
||
/** A source of remote user input, considered as a flow source for CORS misconfiguration. */ | ||
class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource { | ||
RemoteFlowSourceAsSource() { not this instanceof ClientSideRemoteFlowSource } | ||
} | ||
|
||
/** A flow label representing `true` and `null` values. */ | ||
abstract class TrueAndNull extends DataFlow::FlowLabel { | ||
TrueAndNull() { this = "TrueAndNull" } | ||
} | ||
|
||
TrueAndNull truenullLabel() { any() } | ||
|
||
/** A flow label representing `*` value. */ | ||
abstract class Wildcard extends DataFlow::FlowLabel { | ||
Wildcard() { this = "Wildcard" } | ||
} | ||
|
||
Wildcard wildcardLabel() { any() } | ||
|
||
/** An overly permissive value for `origin` (Apollo) */ | ||
class TrueNullValue extends Source { | ||
TrueNullValue() { this.mayHaveBooleanValue(true) or this.asExpr() instanceof NullLiteral } | ||
} | ||
|
||
/** An overly permissive value for `origin` (Express) */ | ||
class WildcardValue extends Source { | ||
WildcardValue() { this.mayHaveStringValue("*") } | ||
} | ||
|
||
/** | ||
* The value of cors origin when initializing the application. | ||
*/ | ||
class CorsApolloServer extends Sink, DataFlow::ValueNode { | ||
CorsApolloServer() { | ||
exists(Apollo::ApolloServer agql | | ||
maikypedia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this = | ||
agql.getOptionArgument(0, "cors").getALocalSource().getAPropertyWrite("origin").getRhs() | ||
) | ||
} | ||
} | ||
|
||
/** | ||
* The value of cors origin when initializing the application. | ||
*/ | ||
class ExpressCors extends Sink, DataFlow::ValueNode { | ||
ExpressCors() { exists(Express::CorsConfiguration config | this = config.getOrigin()) } | ||
} | ||
maikypedia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/** | ||
* Provides a dataflow taint tracking configuration for reasoning | ||
* about overly permissive CORS configurations. | ||
* | ||
* Note, for performance reasons: only import this file if | ||
* `CorsPermissiveConfiguration::Configuration` is needed, | ||
* otherwise `CorsPermissiveConfigurationCustomizations` should | ||
* be imported instead. | ||
*/ | ||
|
||
import javascript | ||
import CorsPermissiveConfigurationCustomizations::CorsPermissiveConfiguration | ||
|
||
/** | ||
* A data flow configuration for overly permissive CORS configuration. | ||
*/ | ||
class Configuration extends TaintTracking::Configuration { | ||
Configuration() { this = "CorsPermissiveConfiguration" } | ||
|
||
override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) { | ||
source instanceof TrueNullValue and label = truenullLabel() | ||
or | ||
source instanceof WildcardValue and label = wildcardLabel() | ||
or | ||
source instanceof RemoteFlowSource and label = DataFlow::FlowLabel::taint() | ||
} | ||
|
||
override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) { | ||
sink instanceof CorsApolloServer and label = [DataFlow::FlowLabel::taint(), truenullLabel()] | ||
or | ||
sink instanceof ExpressCors and label = [DataFlow::FlowLabel::taint(), wildcardLabel()] | ||
} | ||
|
||
override predicate isSanitizer(DataFlow::Node node) { | ||
super.isSanitizer(node) or | ||
node instanceof Sanitizer | ||
} | ||
} | ||
|
||
private class WildcardActivated extends DataFlow::FlowLabel, Wildcard { | ||
WildcardActivated() { this = this } | ||
} | ||
|
||
private class TrueAndNullActivated extends DataFlow::FlowLabel, TrueAndNull { | ||
TrueAndNullActivated() { this = this } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 } | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 } | ||
}); | ||
}); |
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.
Oh yeah, this change-note should probably be in the
javascript/ql/src/change-notes
folder instead.