MultiPart Part.write(String fileName) - Write method used unexpected path

[hrabosch]

Hi,
In MultiPartInputStreamParser class is method public void write(String fileName) throws IOException where original JavaDoc says:

param fileName the name of the file to which the stream will be written. The file is created relative to the location as specified in the MultipartConfig

From Spring Framework is it called by class StandardMultipartHttpServletRequest in transferTo method which looks like:

@Override
public void transferTo(File dest) throws IOException, IllegalStateException {
	this.part.write(dest.getPath());
}

And in my case, there is absolutePath.
And in Jetty implementation of write method is:

_file = new File (_tmpDir, fileName);

Result is that there is duplicated path because _tmpDir contains path already path to tmp files.
There should be checking if variable fileName is absolute, shouldn't be?

[joakime]

Seems like we are following the spec.

javax.servlet.http.Part.write(String filename);

http://docs.oracle.com/javaee/7/api/javax/servlet/http/Part.html#write-java.lang.String-

The name of the file to which the stream will be written. The file is created relative to the location as specified in the MultipartConfig

[hrabosch]

@joakime Yes, that's true, but please see this issue for Spring: https://jira.spring.io/browse/SPR-15257

[joakime]

@hrabosch I added some more details about this at that spring issue.

Spring has its implementation wrong, seems it was relying on the forked/non-standard javadoc from Tomcat about how the Part.write(String) method works.

[janbartel]

I'll add some checks to see if the path given is absolute or not. If absolute we will throw an exception, as that is incorrect according to the spec.

[gregw]

I agree, I don't think it is against the spec to pass a URI starting with /. The only question is what is that relative to? The spec is pretty clear, but if other containers have been more flexible then we need to raise it with the EG.

[janbartel]

The issue has been raised with the servlet EG, just waiting on some considered, official response.

[janbartel]

The EG engaged in some discussion on this, however, the outcome is not entirely clear. I think a public draft is coming in the near future, so it may be prudent to wait to see the exact wording in the javadoc.

[joakime]

I believe Servlet 4.0 (in Jetty 10.x) will change this behavior.

[joakime]

While I can't find the EG discussion, I was able to find the issue tracking the decision at javaee/servlet-spec#172

The updated Part.write(String) details:

https://github.com/javaee/servlet-spec/blob/4.0.0/src/main/java/javax/servlet/http/Part.java#L93-L113

    /**
     * A convenience method to write this uploaded item to disk.
     * 
     * <p>This method is not guaranteed to succeed if called more than once for
     * the same part. This allows a particular implementation to use, for
     * example, file renaming, where possible, rather than copying all of the
     * underlying data, thus gaining a significant performance benefit.
     *
     * @param fileName The location into which the uploaded part should
       be stored. The value may be a file name or a path.  The actual
       location of the file in the filesystem is relative to {@link
       javax.servlet.MultipartConfigElement#getLocation()}.  Absolute
       paths are used as provided and are relative to
       <code>getLocation()</code>.  Note: that this is a system
       dependent string and URI notation may not be acceptable on all
       systems. For portability, this string should be generated with
       the File or Path APIs.
     *
     * @throws IOException if an error occurs.
     */
    public void write(String fileName) throws IOException;

Perhaps internally we should use the updated Part.write(String path) interpretation in the following way.

1. MultipartConfig.location should be tracked as a java.nio.file.Path location.
2. Any provided Part.write(String) parameter should be handed the above Path location via the Path.resolve(String) method and let Java inform of errors/issues.

That seems to honor the new javadoc language correctly, and put the onus on developers to use the updated Part.write(String) properly (eg: thinking about OS path separator specifics, etc)

[joakime]

Found EG discussion archive about this.

http://download.oracle.com/javaee-archive/servlet-spec.java.net/jsr369-experts/2017/03/0558.html

[janbartel]

As you can see by the discussion, the final wording did not address the difference in implementation behaviour of tomcat: as the javadoc currently reads, an "absolute" path must be interpreted relative to the configured multipart location (ie write to location + path) , whereas tomcat will really treat it as absolute (ie write to just path).

As the wording says that even absolute paths must be interpreted relative to multipart location, it is not clear what should be done if the path uses .. to write outside of the multipart location - throwing an exception was discussed in the EG, but not specified in the javadoc.

[joakime]

This entire reply is just my opinion.

There's 2 parts of the updated javadoc I'd like to focus on ...

Part 1: The question about "relative to" and "absolute" vs "relative"

                                                           The actual
       location of the file in the filesystem is relative to {@link
       javax.servlet.MultipartConfigElement#getLocation()}.  Absolute
       paths are used as provided and are relative to
       <code>getLocation()</code>.

To me, "is relative to" is equal to Path.resolve(String), where getLocation() is tracked as a Path in our impl, and String is the fileName argument in the Part.write(String fileName) signature.

Note: Absolute vs Relative depends entirely on the OS and FileSystem used.

The problem with testing for "absolute" fileName is that you'd have to make assumptions that are often wrong on different environments.

You cannot assume that a fileName starting with / (or \) is absolute.
(On Windows that is definitely not true for the NTFS and FAT based FileSystems)

import java.nio.file.FileSystems;
import java.nio.file.Path;

public class AbsoluteTest
{
    public static void main(String[] args)
    {
        isAbsolute("/is/it/abs");
        isAbsolute("\\is\\it\\abs");
        isAbsolute("\\\\ceres\\joakim\\abs");
        isAbsolute("D:\\is\\it\\abs");
    }

    private static void isAbsolute(String fileName)
    {
        // FIXME: This is wrong, we cannot assume default FileSystem
        Path path = FileSystems.getDefault().getPath(fileName);
        System.out.printf("path [%s].isAbsolute=%b%n", fileName, path.isAbsolute());
    }
}

results in ...

path [/is/it/abs].isAbsolute=false
path [\is\it\abs].isAbsolute=false
path [\\ceres\joakim\abs].isAbsolute=true
path [D:\is\it\abs].isAbsolute=true

You might be tempted to use Path.isAbsolute() to test for absolute, but you first have have created that Path from the java.nio.file.FileSystem instance that the fileName belongs to. (You might be on a machine with multiple FileSystems)

Part 2: The question about portability and system independence of API

                                           Note: that this is a system
       dependent string and URI notation may not be acceptable on all
       systems. For portability, this string should be generated with
       the File or Path APIs.

To me, this pushes the responsibility for "sane" usage of this API on to the user of the API, and away from the implementation.
It gives more flexibility to the API, but also hands the user a big bazooka to shoot off their entire lower extremities as a consequence of a mistake.

This could also be equivalent of using Path.resolve(String) improperly and getting errors or odd results.

This section also takes the API away from being consistent and compatible on all environments to being environment specific.

Some examples / observations on windows ...
(Notice the strings used in path.resolve(), those would be examples of the fileName argument)

import java.io.File;
import java.nio.file.Path;

public class PathFun
{
    public static void main(String[] args)
    {
        testPath(new File(System.getProperty("user.dir")).toPath().toAbsolutePath());  // PWD/CWD directory
        testPath(new File("D:\\database").toPath());  //  On drive D
        testPath(new File("C:\\projects").toPath());  // On drive C
        testPath(new File("\\\\ceres\\joakim\\archive").toPath());  // A network reference
    }

    public static void testPath(Path path)
    {
        System.out.println("path = " + path);
        // on windows, none of these strings are absolute
        System.out.println("  p1 = " + path.resolve("/is/this/abs")); // starts with '/'
        System.out.println("  p2 = " + path.resolve("\\is\\this\\abs"));  // starts with '\\'
        System.out.println("  p3 = " + path.resolve("is/this/abs")); // does not start with '/' or '\\'
        System.out.println("  p4 = " + path.resolve("is\\this\\abs")); // does not start with '/' or '\\'
        // but these are
        System.out.println("  p5 = " + path.resolve("D:\\alt"));
        System.out.println("  p6 = " + path.resolve("\\\\ceres\\public\\alt"));
    }
}

The output (on windows) ...

path = C:\code\jetty\junk
  p1 = C:\is\this\abs
  p2 = C:\is\this\abs
  p3 = C:\code\jetty\junk\is\this\abs
  p4 = C:\code\jetty\junk\is\this\abs
  p5 = D:\alt
  p6 = \\ceres\public\alt
path = D:\database
  p1 = D:\is\this\abs
  p2 = D:\is\this\abs
  p3 = D:\database\is\this\abs
  p4 = D:\database\is\this\abs
  p5 = D:\alt
  p6 = \\ceres\public\alt
path = C:\projects
  p1 = C:\is\this\abs
  p2 = C:\is\this\abs
  p3 = C:\projects\is\this\abs
  p4 = C:\projects\is\this\abs
  p5 = D:\alt
  p6 = \\ceres\public\alt
path = \\ceres\joakim\archive
  p1 = \\ceres\joakim\is\this\abs
  p2 = \\ceres\joakim\is\this\abs
  p3 = \\ceres\joakim\archive\is\this\abs
  p4 = \\ceres\joakim\archive\is\this\abs
  p5 = D:\alt
  p6 = \\ceres\public\alt

In conclusion:

I think we should move our implementation to use a java.nio.file.Path instance for getLocation() internally, and blindly take any (non-null) submitted fileName against it with the use of Path.resolve(String). Reporting IOException as appropriate (path doesn't exist, permissions issues, filesystem full issues, etc).

The argument made in the mailing list that the security issue with Part.write(String) is no different then just new File(), both APIs are only accessible via the webapp's own code, not the incoming requests itself.

[joakime]

@janbartel @gregw want me to prepare a PR for this?
If so, do we want this back in jetty-9.4.x (and merge up)?
Or starting in jetty-10.0.x (as this behavior change is starting with Servlet 4.0.x)

[gregw]

@joakime I think we should update along the lines you say... but perhaps we need a compliance mode to maintain our existing behaviour?
I'm OK for you to do this... or perhaps you can direct/supervise @lachlan-roberts to work on it, as he has multipart experience and could probably use a break from ws :)

[janbartel]

@joakime so long as the result is that all filenames will be resolved against the configured getLocation(), and we throw an error if you try and write outside of that.

[janbartel]

The spec javadoc on this is still a mess. I've raised another issue at jakarta to try and get some clarity: jakartaee/servlet#274

[janbartel]

Raised PR at jakarta: jakartaee/servlet#276