Codeql detects zipslip vulnerability

[laurit]

I think that the only way zipslip could happen is when name contains .. but codeql isn't able to cope with that. Removing the .. check gets rid of the code scanning alert.

[trask]

I think that the only way zipslip could happen is when name contains .. but codeql isn't able to cope with that. Removing the .. check gets rid of the code scanning alert.

where are you seeing the codeql failure?

[breedx-splk]

@laurit Would you mind linking to or screenshotting the codeql output? I couldn't find it in the nightly output.

This fix is counterintuitive to me -- the .. check appeared to be trying to prevent zipslip like content, but with it removed it would be allowed. This also leaves behind the comment about zipslip, even tho the check is removed.

Edit: Ok, after further looking, the .. check isn't maybe robust enough in the first place, and the remaining startsWith fix follows the Java validation pattern shown here. 👍🏻

[breedx-splk]

Suggested change

	.startsWith(tempDirectory.getCanonicalFile().toPath())) {
	.startsWith(tempDirectory.getCanonicalPath() + File.separator)) {

It looks like appending the file separator to the end of the expected path is important in order to prevent writing to a neighboring directory. That is, if the actual temp dir is /tmp and the file coming in from the zip somehow crafts ../../../../tmpOther it could possibly write to /tmpOther which should also be prevented.

[laurit]

where are you seeing the codeql failure?

https://github.com/open-telemetry/opentelemetry-java-instrumentation/security/code-scanning