Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

maikypedia
Copy link
Contributor

@maikypedia maikypedia commented Sep 29, 2023

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.

@maikypedia maikypedia requested a review from a team as a code owner September 29, 2023 16:40
@github-actions github-actions bot added the JS label Sep 29, 2023
@maikypedia maikypedia marked this pull request as draft September 29, 2023 17:00
Copy link
Contributor

@erik-krogh erik-krogh left a 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",
Copy link
Contributor

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.

Comment on lines 9 to 10
/** A string-valued expression that is interpreted as a Apollo GraphQL query. */
abstract class GraphQLString extends DataFlow::Node { }
Copy link
Contributor

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 { }
Copy link
Contributor

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 {
Copy link
Contributor

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.

Comment on lines 29 to 30
/** Get an instanceof of `gql` */
private API::Node gql() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 {
Copy link
Contributor

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class BadValues extends Source instanceof DataFlow::Node {
class BadValues extends Source {

Adding DataFlow::Node should make no difference here.

@@ -0,0 +1,20 @@
/**
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Comment on lines 19 to 20
select sink.getNode(), source, sink, "$@ misconfiguration due to a $@.", sink.getNode(),
"CORS Origin", source.getNode(), "too permissive or user controlled value"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

QHelp previews:

javascript/ql/src/experimental/Security/CWE-942/CorsPermissiveConfiguration.qhelp

overly CORS configuration

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.

Recommendation

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.

Example

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.

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 server_1 CORS is restrictive so it's not vulnerable to CSRF attacks. And server_2's is using properly sanitized user-controlled data.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants