-
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
Merged
erik-krogh
merged 31 commits into
github:main
from
maikypedia:maikypedia/javascript-cors
Jun 28, 2024
Merged
Changes from 26 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
e171123
Add initial query for CWE-942
maikypedia 142ab01
Remove comment line
maikypedia 816eebb
Add `.qhelp` and apply some review changes
maikypedia ed06628
Add documentation string for `CorsPermissiveConfiguration`
maikypedia c0e6d7c
Merge branch 'github:main' into maikypedia/javascript-cors
maikypedia 07ad596
Add coverage for `express`
maikypedia acac534
Forgot `.js`
maikypedia d661f7f
Add Flow Labels
maikypedia 413c111
Move to `/experimental`
maikypedia abd53e9
Fix minor issues
maikypedia 4ef4c92
Move Customizations and Query
maikypedia aa24ce5
Apply suggestions from code review
maikypedia bb6ef72
`getArgument` returns `Cors::Cors`
maikypedia f623db4
Change qldoc
maikypedia 3bcb411
Using `Express::RouteSetup`
maikypedia 6a3cdc9
Add `change-node`
maikypedia e6c7fc0
Fixes CI
maikypedia 83cbbd7
Apply docstring changes
maikypedia 87cac2a
Express Argument has to be Cors
maikypedia 4f68f60
Apply review
maikypedia 191766a
Use `config.getCorsConfiguration().getOrigin())`
maikypedia 7662b2b
format
maikypedia 78e7793
Move to experimental
maikypedia 699d8d4
x
maikypedia c1fd7a6
autoformat
erik-krogh f2d6640
fix ambiguous import. It could refer both to a module or a file
erik-krogh cfd7c7a
move change-note to `javascript/ql/src/change-notes`
maikypedia e96c3a3
Move `Apollo` to experimental
maikypedia 4be5cf4
Update javascript/ql/src/experimental/Security/CWE-942/CorsPermissive…
maikypedia 8ba7ac6
Update javascript/ql/src/experimental/Security/CWE-942/CorsPermissive…
maikypedia d0cf2a9
Merge branch 'main' into maikypedia/javascript-cors
maikypedia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
4 changes: 4 additions & 0 deletions
4
javascript/ql/lib/change-notes/2023-11-27-cors-permissive-configuarion.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
--- | ||
category: minorAnalysis | ||
--- | ||
* Added a new experimental query, `js/cors-misconfiguration`, which detects misconfigured CORS HTTP headers in the `cors` and `apollo` libraries. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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") | ||
} | ||
|
||
/** An instantiation of an `ApolloServer`. */ | ||
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) } | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 the [cors](https://npmjs.com/package/cors) library. */ | ||
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 the `origin` option used to configure this Cors instance. */ | ||
DataFlow::Node getOrigin() { result = this.getOptionArgument(0, "origin") } | ||
} | ||
} |
71 changes: 71 additions & 0 deletions
71
javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.qhelp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> |
20 changes: 20 additions & 0 deletions
20
javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.ql
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" |
94 changes: 94 additions & 0 deletions
94
...script/ql/src/experimental/Security/CWE-942/CorsPermissiveConfigurationCustomizations.qll
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
/** | ||
* Provides default sources, sinks and sanitizers for reasoning about | ||
* overly permissive CORS configurations, as well as | ||
* extension points for adding your own. | ||
*/ | ||
|
||
import javascript | ||
import Cors::Cors | ||
|
||
/** 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(CorsConfiguration config | this = config.getCorsConfiguration().getOrigin()) | ||
} | ||
} | ||
|
||
/** | ||
* An express route setup configured with the `cors` package. | ||
*/ | ||
class CorsConfiguration extends DataFlow::MethodCallNode { | ||
Cors corsConfig; | ||
|
||
CorsConfiguration() { | ||
exists(Express::RouteSetup setup | this = setup | | ||
if setup.isUseCall() | ||
then corsConfig = setup.getArgument(0) | ||
else corsConfig = setup.getArgument(any(int i | i > 0)) | ||
) | ||
} | ||
|
||
/** Gets the expression that configures `cors` on this route setup. */ | ||
Cors getCorsConfiguration() { result = corsConfig } | ||
} | ||
} |
46 changes: 46 additions & 0 deletions
46
javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfigurationQuery.qll
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 } | ||
} |
18 changes: 18 additions & 0 deletions
18
javascript/ql/src/experimental/Security/CWE-942/examples/CorsPermissiveConfigurationBad.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 } | ||
}); | ||
}); |
18 changes: 18 additions & 0 deletions
18
javascript/ql/src/experimental/Security/CWE-942/examples/CorsPermissiveConfigurationGood.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 } | ||
}); | ||
}); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.