False positive with javasecurity:S2083 when used with user-controlled Classloader resource names

Rules and Languages Report False-positive / False-negative...
1

There is the following false positive raised for javasecurity:S2083 when a user-controlled parameter is used with Class.getResource(String userControlledParam).

This can be seen in SonarQube Cloud with the open source project at GitHub - Netcentric/accesscontroltool: Rights and roles management for AEM made easy · GitHub.

The problematic line is

final URL url = getClass().getResource(resourcePath);

Where resource path is “only” sanitized like this

String resourcePath = req.getRequestURI().substring(basePath.length());
if (resourcePath.startsWith("/res/")) {
    ... code calling getClass().getResource(resourcePath)
}

I would argue that since path traversal is not possible in the String method argument of Class (Java Platform SE 8 ) the prefix checking should suffice here.

The rule description for javasecurity:S2083 explicitly mentions path traversal as attack vector:

A user with malicious intent would inject specially crafted values, such as ../, to change the initial intended path. The resulting path would resolve somewhere in the filesystem where the user should not normally have access to.

Also the “How would I fix this” section does not apply here as it is not a filesystem path but just a resource name!

Compliant code:

File file = new File(targetPath + filename);

        if (!file.toPath().normalize().startsWith(targetPath)) {
            throw new IOException("Entry is outside of the target directory");
        }
5

Hi @kwin,

First of all thanks for reporting this!

Regarding the FP

We investigated this further and confirmed that we do not consider this a false positive as the check performed using startsWith would be not enough to prevent an injection.

Take as an example the case where an attacker injects the tainted value:

/safepath/../notsafepath/secretresource.txt

If you just check that it startsWith("/safepath/") , that’s not enough because without canonicalization, the attacker can still traverse to a dangerous path by using ..

For that reason, a prefix check alone is not sufficient. The path should first be normalized (or canonicalized where applicable) and then checked against the allowed base path.

For example, if you canonicalize

/safepath/../notsafepath/secretresource.txt

it becomes

/notsafepath/secretresource.txt

And then if you invoke a startsWith() afterwards, you can consider it validated.

Regarding the rule description

Your point about the rule documentation is fair: the current “How to fix” example is File-based and does not clearly illustrate the resource-name case.

We are updating the documentation to add a more relevant example.

Thanks a lot for spotting and reporting this, it is a valuable input to help us improving our product!