Sanitize file upload data

[hayageek]

Path Traversal via Unsanitized Filename and Upload ID in FolderUploadsFileManager

Attribute	Value
Severity	Critical
CVSS 3.1	9.1 — CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:N
CWE	CWE-22: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')

Summary

FolderUploadsFileManager in Apache Wicket does not validate or sanitize the uploadFieldId parameter or the clientFileName before constructing file paths, allowing an unauthenticated attacker to write arbitrary files outside the intended upload directory or read files from arbitrary locations on the server.

Description

• Type: Path Traversal — Arbitrary File Write / Arbitrary File Read
• Source: HTTP multipart upload request. The uploadId query parameter flows into save() as uploadFieldId (line 66). The clientFileName in getFile() comes from client-controlled JSON in the AJAX callback (UploadInfo.fromJson()).
• Sink (Write): FolderUploadsFileManager.save() at line 66–70:

  File uploadFieldFolder = new File(getFolder(), uploadFieldId);  // line 66 — unsanitized
  uploadFieldFolder.mkdirs();
  IOUtils.copy(fileItem.getInputStream(),
      new FileOutputStream(new File(uploadFieldFolder, fileItem.getClientFileName())));  // line 70

• Sink (Read): FolderUploadsFileManager.getFile() at line 81:

  return new File(new File(getFolder(), uploadFieldId), clientFileName);  // unsanitized

• Impact: An attacker can write arbitrary files anywhere on the filesystem that the application server process has write access to. This enables remote code execution (e.g., writing JSP webshells), configuration tampering, or denial of service. The getFile() method also permits reading arbitrary files when the attacker manipulates the clientFileName in the AJAX response JSON.

Attack Vectors

1. uploadFieldId traversal: The uploadId HTTP query parameter is passed directly to new File(getFolder(), uploadFieldId) without validation. An attacker sends ?uploadId=../../webapps/ROOT to write files into the web root.
2. clientFileName traversal via getFile(): The clientFileName field in the AJAX filesInfo JSON is deserialized by UploadInfo.fromJson() and passed to getFile() without sanitization, allowing path traversal reads.
3. clientFileName traversal via save(): While FileUpload.getClientFileName() strips / and \ path separators, the FolderUploadsFileManager class has no defense-in-depth. Any caller or subclass providing an unsanitized filename bypasses the intended protection.

Affected

• Package: org.apache.wicket:wicket-core
• File: wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java
• Lines: 66–70 (save), 79–82 (getFile)
• Introduced: When FolderUploadsFileManager was added

For POC, check here: https://gist.github.com/hayageek/3dda5923bbda483954dd84cfb5651534

[hayageek]

Hi @martin-g ,
I have implemented the code review comments.

1. mkdirs() result ignored
   Replaced uploadFieldFolder.mkdirs() (whose boolean return is silently ignored) with Files.createDirectories(target.toPath().getParent()), which throws IOException on failure — making errors impossible to miss.

2. Extract duplicated code
   The identical validation block that appeared in both save() and getFile() is now a single private helper:
   Both save() and getFile() now simply call resolveTargetFile(...).

3. Unit tests — already done in the previous round (5 tests total covering all three cases for both save() and getFile()).

4. IOException silently swallowed in getPathForComparison()
   Changed the broad catch (IOException e) to catch (NoSuchFileException e). The fallback to toAbsolutePath().normalize() is only valid for the case where the directory doesn't exist yet (sub-folder created during save()). All other IO errors (permissions, device failures, etc.) now propagate correctly instead of being hidden.

[reiern70]

• FolderUploadsFileManager

Thanks for fixing this

[hayageek]

@reiern70 , Can you merge the PR

[hayageek]

🙇

[hayageek]

@martin-g Can I request a CVE for this ?

[martin-g]

You can!

But I won't do it myself for two reasons:

1. the CVE creation process is very unfriendly.
   It used to be a copy/paste of a template and adaptation of few placeholders like the title, description and severity. Now it is a big complex web form with many fields which I have no idea how to populate correctly

2. you reported the issue publicly and thus made it a 0-day vulnerability ... So, it is a bit late for this.
   The proper way is to report it to the maintainers first - https://wicket.apache.org/help/#security & https://www.apache.org/security/

[solomax]

CVE creation is much easier this days :)
You only need description and version range

[reiern70]

Cherry pick this to 10.x branch?

[reiern70]

Cherry pick this to 10.x branch?

Ok. I see it was already done.

[reiern70]

You can!

But I won't do it myself for two reasons:

1. the CVE creation process is very unfriendly.
   It used to be a copy/paste of a template and adaptation of few placeholders like the title, description and severity. Now it is a big complex web form with many fields which I have no idea how to populate correctly
2. you reported the issue publicly and thus made it a 0-day vulnerability ... So, it is a bit late for this.
   The proper way is to report it to the maintainers first - https://wicket.apache.org/help/#security & https://www.apache.org/security/

I think we are possibly one of the few users of this functionality,and we use anther version of IUploadsFileManager, thus I doubt this has impact for many users. But letting them know via email might be good.