JS: Add Permissive CORS query (CWE-942)

[maikypedia]

trafficstars

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.

[github-actions[bot]]

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

• Mozilla Developer Network: CORS, Access-Control-Allow-Origin.
• W3C: CORS for developers, Advice for Resource Owners
• Common Weakness Enumeration: CWE-942.

[erik-krogh]

The QLDoc check is still complaining about missing documentation string on CorsPermissiveConfigurationCustomizations::CorsPermissiveConfiguration. It's one of the review comments I gave above.
Can you fix that? It feels better reviewing something when the CI check is green.

[maikypedia]

Since I used null and true as sources for taint track (apollo), is there any "not-ugly" way to include * as source for express?

[maikypedia]

^ express cors library covered

[erik-krogh]

Since I used null and true as sources for taint track (apollo), is there any "not-ugly" way to include * as source for express?

What do you mean by *?

[maikypedia]

Since I used null and true as sources for taint track (apollo), is there any "not-ugly" way to include * as source for express?

What do you mean by *?

// BAD: CORS too permissive 
var app3 = express();
var corsOption3 = {
    origin: '*'
};
app3.use(cors(corsOption3));

The value of origin, wildcard includes any origin

[erik-krogh]

Since I used null and true as sources for taint track (apollo), is there any "not-ugly" way to include * as source for express?

What do you mean by *?

// BAD: CORS too permissive 
var app3 = express();
var corsOption3 = {
    origin: '*'
};
app3.use(cors(corsOption3));

The value of origin, wildcard includes any origin

Hmm. Yes you can, but it's not simple, so maybe you should just skip it.
You can use flow-labels to track multiple kinds of values that are relevant for different kinds of sinks.
There is some documentation here: https://codeql.github.com/docs/codeql-language-guides/using-flow-labels-for-precise-data-flow-analysis/

---

Also, you should move the rest of your implementation into the experimental/ folder.
Your query is extremely close to our existing js/cors-misconfiguration-for-credentials query, but with some new library models (which is your "real" contribution).
So I think we want to do something else before we move it out of experimental.

[maikypedia]

Sorry for the delay. Should I move CorsPermissiveConfigurationCustomizations.qll and CorsPermissiveConfigurationQuery.qll to /codeql/javascript/ql/experimental

[erik-krogh]

Sorry for the delay. Should I move CorsPermissiveConfigurationCustomizations.qll and CorsPermissiveConfigurationQuery.qll to /codeql/javascript/ql/experimental

Yes please.

[maikypedia]

Moved 👍

[erik-krogh]

Our QL-for-QL analysis found some minor issues, some of that is also causing the CI errors.
You should see them by scrolling through the changed files: https://github.com/github/codeql/pull/14342/files

I'll give a review after you've fixed those.

[erik-krogh]

The CI is failing for two reasons:

• The Express::Express::CorsConfiguration::getArgument predicate does not have a docstring.
• Cors.qll is not formatted correctly. (You can use codeql query format -i <path-to-file> to fix it).

[erik-krogh]

The javascript/ql/lib/semmle/javascript/frameworks/Apollo.qll file is failing the autoformat check.

You can use codeql query format -i <path-to-file> to run the autoformatter.

[erik-krogh]

One last thing, and then we can merge.

The Cors.qll file and the CorsConfiguration class in Express.qll are only used in your new experimental query.
Could you move those to experimental/?
The CorsConfiguration class fits in CorsPermissiveConfigurationCustomizations.qll, and you can move the Cors.qll file to the javascript/ql/src/experimental/Security/CWE-942/ folder.

[erik-krogh]

The CorsPermissiveConfiguration.ql query failed to compile (that's the CI failure).
I'm not sure why, but I suspect you need to add/update an import after your recent change.

[maikypedia]

It should compile, I don't know why it didn't, let's try again.

[erik-krogh]

The error was due to a warning which was caused by Cors being ambiguous.
I fixed the issue and pushed to this PR.

Lets see if the CI goes green.

[maikypedia]

Now it's working! Thanks 😄

[maikypedia]

ping @erik-krogh :)

[erik-krogh]

A CI job keeps failing. I think that is from your branch being quite out-of-date, can you do a merge with main?
(I tried myself, but I can't push to your branch).

Sorry it's taking so long.