Yet another SSRF query for Javascript

valeria-meli commented 3 years ago

Query

Relevant PR: https://github.com/github/codeql/pull/6714

CVE ID(s)

The vulnerabilities were found in private code and that details will be sent privately to securitylab@github.com

Report

This query finds SSRFs (Server Side Request Forgeries). This is intended to be an alternative to the current Request Forgery query.

About SSRF

A web server is vulnerable to SSRF when it receives some user input, uses it to form a URL and then sends a request to this URL. If the user input isn’t properly checked, an attacker could manipulate the URL and our web server will send the request elsewhere. This request will be done in our app’s name. An attacker can use this to bypass a firewall, since our app will be trusted in the internal network, and read confidential information or execute unauthorized actions.

Why a new query?

We use codeql queries at Mercado Libre to block pull requests. We need them to be very precise. We recommend input validation, not URL escaping, when fixing a SSRF. Some NGINx based routing tend to decode before routing, making our URL escape useless. But input validation always works. So we adapted the original query to this blocking context and to accept different forms of input validation.

What's different?

The original query has a heuristic where this case request(http://example.com/${base}${tainted}); is assumed OK to avoid false positives. We removed such heuristic. Our query flags this same case as SSRF.

On the other hand, we added some sanitizers.

Let’s say your expected input is a number. You could check it with Number.isInteger(tainted) or cast it with Number(tainted). Are you expecting just letters? Alphanumeric? UUID? We support the validator module and its safe checks. If you’re expecting a specific word, you could compare it with ==. Regular expressions are hard to check for correctness, but the original query supports them and we kept it (we have to trust the developer on this one).

We deployed this query, started blocking PRs and received some false positives. Not too many. Analyzing these cases, we found out some people do this:

let valid = tainted ? Number.isInteger(tainted) : false
if (valid) {
        request(“example.com/” + tainted)
}

This is actually safe, but sanitizer guards don’t see it. The classes TernaryOperatorSanitizer and TernaryOperatorSanitizerGuard add support to this case and other variants.

[x] Are you planning to discuss this vulnerability submission publicly? (Blog Post, social networks, etc). We would love to have you spread the word about the good work you are doing

Result(s)

We have lots of cases, but all on private code. We’ll be sending the details to securitylab@github.com

Collaborators

@npesaresi
@luciaromeroML
@alangatti
@valeria-meli