Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-13662

CommonsMultipartFile.getOriginalFilename() does not strip file path properly

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Complete
    • Affects Version/s: 3.2.15, 4.1.8, 4.2.2
    • Fix Version/s: 3.2.16, 4.1.9, 4.2.3
    • Component/s: Web
    • Labels:

      Description

      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.

        Issue Links

          Activity

          Hide
          juergen.hoeller Juergen Hoeller added a comment - - edited

          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

          Show
          juergen.hoeller Juergen Hoeller added a comment - - edited 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
          Hide
          condorlee@hotmail.com Hua Li added a comment -

          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

          Show
          condorlee@hotmail.com Hua Li added a comment - 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
          Hide
          juergen.hoeller Juergen Hoeller added a comment -

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

          Juergen

          Show
          juergen.hoeller Juergen Hoeller added a comment - Resolved through always truncating at the last separator in the path, no matter which style. Juergen
          Hide
          challarao Challa Rao Ande added a comment -

          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?

          Show
          challarao Challa Rao Ande added a comment - 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?
          Hide
          juergen.hoeller Juergen Hoeller added a comment -

          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 SPR-14613 to reconsider our behavior, in particular towards alignment between CommonsMultipartFile and StandardMultipartFile.

          Show
          juergen.hoeller Juergen Hoeller added a comment - 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 SPR-14613 to reconsider our behavior, in particular towards alignment between CommonsMultipartFile and StandardMultipartFile .
          Hide
          challarao Challa Rao Ande added a comment -

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

          Show
          challarao Challa Rao Ande added a comment - Thanks. My only concern was that it misbehaves with perfectly valid file name (a file name with \ in it) in Linux.

            People

            • Assignee:
              juergen.hoeller Juergen Hoeller
              Reporter:
              condorlee@hotmail.com Hua Li
              Last updater:
              Challa Rao Ande
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                1 year, 26 weeks, 5 days ago