Java: Arbitrary user-controlled read/write on user-controlled path

[intrigus-lgtm]

Still early work, but posting nevertheless to prevent further bounty collisions.

[Marcono1234]

Isn't an equals check strong? E.g.:

if (s.equals("C:/test")) {
    Path.of(s);
}

[Marcono1234]

Suggested change

	/** Models additional taint steps like `file.toPath()`, `path.toFile()`, `new FileInputStream(..)`, `Files.readAll{Bytes|Lines}(...)`, and `new File(...)`. */
	/** Models additional taint steps like `file.toPath()`, `path.toFile()`, `new FileInputStream(...)`, `Files.readAll{Bytes|Lines}(...)`, and `new File(...)`. */

[Marcono1234]

Java 11 added Files.readString​(Path) and Files.readString(Path, Charset), would probably good to cover them as well

And Files.lines​(Path) and Files.lines(Path, Charset) might be relevant as well

Edit: Though note that this predicate duplicates logic of FileReadWrite.qll

[Marcono1234]

Should this maybe also cover the Files.new... methods? I.e.:

• Files.newBufferedReader
• Files.newByteChannel
• Files.newInputStream

[Marcono1234]

Would it make sense to consider methods which access the contents of the folder?
I.e.:

• Files.find
• Files.list
• Files.newDirectoryStream​
• Files.walkFileTree

Though I am not sure if that would taint objects retrieved from the return values as well (e.g. Files.list(...).forEach(p -> Files.delete(p))).

Maybe it would make sense to include the java.io.File listing files as well.

[Marcono1234]

Maybe consider the following as well?

• Files.setAttributes
• Files.setOwner
• Files.setPosixFilePermissions

And same for respective java.io.File methods.

[Marcono1234]

Path.resolve(Path) / Path.resolveSibling(Path) is probably a taint step as well, right?
Maybe also Path.relativize(Path)

And the other Path methods returning a Path as well?

And same for java.io.File methods returning File or String representation of file?

[Marcono1234]

This might cause false negatives, e.g.

String s = ...;
Path ROOT = Path.of("/my-app/data);
if (!s.contains("..")) {
  Files.delete(ROOT.resolve(Path.of(s));
  // Or
  Files.delete(ROOT.resolve(s));
)

Would still be vulnerable if s is absolute.

[intrigus-lgtm]

Thank you for the early review @Marcono1234 :)
The biggest part of https://github.com/github/codeql/pull/3794/files#diff-babd1a1e86909a409ee3a0ff2ee648f2R1-R82 is not my code, but code copied from a similar query that only looks for path injections.
The ContainsDotDotSanitizer class is also from there.

I'll have to merge the common Path stuff somewhere.

I don't know whether I want to add the additional path creations like Path.of() in this query.
That would probably make evaluation harder, but I'll prepare a separate PR that adds them.

The other suggestions I will have a look at probably next week.

[Marcono1234]

Does this depend on #3881 now?

[Marcono1234]

Maybe also apply this change to #3881?