Potential path traversal when using filename from Zip archive

[h3xstream]

Description

The class ZipEntry describe one file inside a Zip archive. getName() return the file name from this file. It could contain a malicious string such as "../../../". If the value is used to build a file path, it could lead to a path traversal attack. An attacker would be able to create file or override files with the content of his own.

Good References:

• https://snyk.io/research/zip-slip-vulnerability
• https://blog.ripstech.com/2019/hidden-flaws-of-archives-java/
• CWE: https://cwe.mitre.org/data/definitions/23.html

Vulnerable code

Enumeration entries = zip.entries();
while (entries.hasMoreElements()) {
    ZipEntry zipEntry = entries.nextElement();
    File file = new File(outputDir, zipEntry.getName())
[...]

API to cover :

• java.util.zip.ZipEntry.getName()
• org.apache.commons.compress.archivers.ar.ArArchiveEntry.getName()
• org.apache.commons.compress.archivers.arj.ArjArchiveEntry.getName()
• org.apache.commons.compress.archivers.cpio.CpioArchiveEntry.getName()
• org.apache.commons.compress.archivers.dump.DumpArchiveEntry.getName()
• org.apache.commons.compress.archivers.jar.JarArchiveEntry.getName()
• org.apache.commons.compress.archivers.sevenz.SevenZArchiveEntry.getName()
• org.apache.commons.compress.archivers.tar.TarArchiveEntry.getName()
• org.apache.commons.compress.archivers.zip.ZipArchiveEntry.getName()
• org.apache.commons.compress.archivers.ArchiveEntry.getName()

Similar detector

If anybody is interested in the implementation of such rule. It is very similar to the rule to detect FileItem.getName() from multipart upload.

find-sec-bugs/findsecbugs-plugin/src/main/java/com/h3xstream/findsecbugs/file/FileUploadFilenameDetector.java

Lines 31 to 51 in f86e2f2

	public class FileUploadFilenameDetector extends OpcodeStackDetector {
	private static String FILE_UPLOAD_FILENAME_TYPE = "FILE_UPLOAD_FILENAME";
	private BugReporter bugReporter;
	public FileUploadFilenameDetector(BugReporter bugReporter) {
	this.bugReporter = bugReporter;
	}
	@Override
	public void sawOpcode(int seen) {
	if (seen == Const.INVOKEINTERFACE &&
	(getClassConstantOperand().equals("org/apache/wicket/util/upload/FileItem") ||
	getClassConstantOperand().equals("org/apache/commons/fileupload/FileItem")) &&
	getNameConstantOperand().equals("getName")) {
	bugReporter.reportBug(new BugInstance(this, FILE_UPLOAD_FILENAME_TYPE, Priorities.NORMAL_PRIORITY) //
	.addClass(this).addMethod(this).addSourceLine(this));
	}
	}

[h3xstream]

Bonus : Could be implemented in the same enhancements..
zip4j is a popular library. I think that the equivalent concept would be AbstractFileHeader.getFileName()

[thiyagu-7]

Can I take this up? :)

[h3xstream]

@thiyagu-7 Yes

[thiyagu-7]

I modified one of the existing test classes adding

Enumeration<? extends ZipEntry> entries = zip.entries();
while (entries.hasMoreElements()) {
    ZipEntry e = entries.nextElement();
    File f = new File("C:/Code/", e.getName());
}

This raised PATH_TRAVERSAL_IN bug as implemented by PathTraversalDetector.

New Bug Instance: [1]
  message=SECPTI: This API (java/io/File.<init>(Ljava/lang/String;Ljava/lang/String;)V) reads a file whose location might be specified by user input
  bugType=PATH_TRAVERSAL_IN  priority=Medium  category=S
  class=testcode.FileUploadCommon  method=handleFile  line=33
  sources=[[java/util/zip/ZipEntry.getName()Ljava/lang/String;]]

Would this then also cover the ZipEntry cases too? 🤔

[h3xstream]

@thiyagu-7 Yes the fact that there is both a PATH_TRAVERSAL_IN and the new ZIP_ENTRY_NAME? are being raised is perfect.

Both have a different purpose. ZipEntry.getName() is the risky source. File() is the method from which malicious value should not reach unfiltered (sink).

[h3xstream]

This issue is up for grab.

[akshaycbor]

Can I take it?

[akshaycbor]

How do I detect that the value read in getName reaches the File() (sink) unfiltered?

[h3xstream]

Can I take it?

@akshaycbor Yes

How do I detect that the value read in getName reaches the File() (sink) unfiltered?

In this specific case, you can do two things.

• Report a Low Bug instance (informational) to highlight to the reviewer that it is a risky source.
• You could mark the return value as tainted. (Example) This has the potention of rising the severity from Normal to High for a path traversal bug.

[akshaycbor]

@h3xstream Maybe a silly question but how do I get a single test to run. Whenever I try to run a single test using mvn -Dtest=FileUploadFilenameDetectorTest test it fails

[h3xstream]

@akshaycbor Running the test individually also failed on my side. I did not find a solution to run a single test with Maven.

I have mostly use Maven to run the complete suite and use my IDE IntelliJ to run the test individually.

I will investigated this later.

[Ayan07]

@h3xstream I 'm interested to work on this issue.
I see this issue is currently unassigned.
Can I take this up?