CommonsMultipartFile.getOriginalFilename() does not strip file path properly [SPR-13662]

[spring-projects-issues]

Hua Li opened SPR-13662 and commented

Note
I found the issue in the latest code of master branch here: https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/multipart/commons/CommonsMultipartFile.java
I assume it applies to the latest 4.2.2 version.

getOriginalFilename() tries to strip file path from while file path name string and returns only the file name part.
It has been coded to be adaptive - looking for Linux path separator char "/" first, if fail then looking for Windows path separator char "".

But this adaptive logic is buggy - if Spring is running on a Windows computer and if attacker provides a path name like "/......\malicious_directory\malicious_file" then the getOriginalFilename() method will return "......\malicious_directory\malicious_file" which is not a bare file name but contains both path and file name.

Then if application layer code assumes it is a bare file name and use it as a bare file name, critical path traversal issue can happen.

I think the right logic is - using File.separator to find and strip the path and get bare file name.

---

Affects: 3.2.15, 4.1.8, 4.2.2

Reference URL: https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/multipart/commons/CommonsMultipartFile.java

Issue Links:

• StringUtils.getFilename(String) should parse any kind of file separator [SPR-5284] #9957 StringUtils.getFilename(String) should parse any kind of file separator
• CommonsMultipartFile.getOriginalFilename() should be able to preserve header-specified filename as-is [SPR-14613] #19180 CommonsMultipartFile.getOriginalFilename() should be able to preserve header-specified filename as-is

Referenced from: commits 8d7d4ce, df49b11, 5d9d88c

Backported to: 4.1.9, 3.2.16

[spring-projects-issues]

Juergen Hoeller commented

Good catch; this should get revised.

That said, please note that getOriginalFilename() is not intended to refer to any file on the server in the first place, so application code should not treat it as a sanitized filename that can be applied to the server's filesystem. It's simply the filename information that the browser sends when performing the upload; this may serve as a default naming suggestion for something stored on the server but not really more than that. This is the reason why we're providing a best-effort attempt to remove path information from the filename: simply to make naming-oriented processing of the filename hint easier. Application code that applies it as-is to the filesystem could accidentally overwrite other files on the server etc, even for plain filenames within the same directory. From that perspective, application code needs to be coded in a way where the original filename does not have filesystem semantics to begin with.

Juergen

[spring-projects-issues]

Hua Li commented

Yes, Juergen. I totally agree with you. It is simply the file name provided from the browser / client side. Web server developers should not use that name as local file name unless they are fully aware of the security implications. While on the other hand we can also expect developers may have this kind of misuse from time to time. That's the reality, sadly. So fixing this on Spring side will likely mitigate quit a few of potential security issues in web applications that utilizing Spring framework.

And thanks for the quck response - I was impressed :)

[spring-projects-issues]

Juergen Hoeller commented

Resolved through always truncating at the last separator in the path, no matter which style.

Juergen

[spring-projects-issues]

Challa Rao Ande commented

Ubuntu(and I believe even Linux) allows "" in the filename, and if user uploads from a browser from linux having filename such as "hello\file.txt", #getOriginalFilename() will only give "file.txt".

Also, do typical clients(browser clients in particular) send client filepath along with the name? If not do we need this piece of code to strip the filename?

[spring-projects-issues]

Juergen Hoeller commented

Older browsers such as Opera sent path information but I'm not sure that's still the case. Also, programmatic HTTP clients may end up including some path information depending on how they build their Content-Disposition header. Our explicit stripping of path information follows Commons FileUpload guidelines and is just meant for defensiveness so that server-side processing code does not unintentionally traverse directories when building target paths there.

In any case, I've raised #19180 to reconsider our behavior, in particular towards alignment between CommonsMultipartFile and StandardMultipartFile.

[spring-projects-issues]

Challa Rao Ande commented

Thanks. My only concern was that it misbehaves with perfectly valid file name (a file name with \ in it) in Linux.