JS: Add Permissive CORS query (CWE-942)

[maikypedia]

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.

[erik-krogh]

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.

[erik-krogh]

Suggested change

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

Adding DataFlow::Node should make no difference here.

[github-actions]

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]

This characteristic predicate holds for any .use() call, you haven't restricted it to only be the ´.use() ´ calls that are configured with the Cors package.

I think you should add Cors::Cors as a field on this class. That should make it easier to implement the restriction.

Additionally (as I have mentioned before).
The cors package can also be used like: app.get('/products/:id', cors(corsOptions) ....
With RouteSetup, I think it should be easy to model that.
So if setup.isUseCall(), then the first argument should be the call to Cors.
And if setup.isUseCall() doesn't hold, then the later arguments can be the call to Cors.

[maikypedia]

Could it be simplified to just setup.getAnArgument() instanceof Cors::Cors?

[erik-krogh]

Not quite.
Your Cors::Cors getArgument() predicate on the CorsConfiguration class doesn't work, because that predicate just assumes that the first argument is the cors configuration.

I've added some suggestions to use a field such that the result of getArgument depends on the routesetup in the characteristic predicate.

[erik-krogh]

Not quite.
Your Cors::Cors getArgument() predicate on the CorsConfiguration class doesn't work, because that predicate just assumes that the first argument is the cors configuration.

I've added some suggestions to use a field such that the result of getArgument depends on the routesetup in the characteristic predicate.

[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]

ping @erik-krogh :)

Sorry, I've had a lot to look at.
One more file move, and then I think we'll be there.

[erik-krogh]

This file is only used within your experimental query, so it should probably be moved to the same folder.

[maikypedia]

done 👍

[erik-krogh]

Oh yeah, this change-note should probably be in the javascript/ql/src/change-notes folder instead.

[erik-krogh]

Looks good, but now there is a nit about imports.
You are importing from a file named Apollo, which imports a module also named Apollo, which means that there could be confusion about which Apollo is referenced at the import, and that is what the compiler-warning is about.

I added suggestions to fix it, by not directly importing anything named Apollo.

[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.