Mark,
I guess that is true, although I suspect there are many applications with a
short and non sanitary path from user data to the argument of write.
So just to really clarify, you are treating the arg as a filename on the
local filesystem, so an application that calls
part.write("mysubdir/filename.txt");
will not be portable, as that is assuming unix directory naming? A
portable app is going to have to write:
part.write(Paths.get("mysubdir","filename.txt").toString());
and I bet most do not do that. The javadoc should really indicate this.
What exactly is the mechanism you use to convert the passed arg to an
absolute file? Is it
pathOfConfigDir.resolve(Paths.get(filenameArg));
?
I definitely think the javadoc needs to be clarified.
If we had our time over, the String argument should have been a URI not a
filename and we could have had a variant of the method that took a File.
regards
On 15 March 2017 at 23:48, Mark Thomas <markt_at_apache.org> wrote:
> On 15/03/17 04:24, Greg Wilkins wrote:
> >
> > Stuart,
> >
> > So do you accept an absolute path that is any where in the file system,
> > or does it have to be within the tree specified by the multipart config?
> >
> > For example, if the configuration has the location as C:\\foo\bar ,
> > then do you accept a call to write with a value of something like
> > C:\\somewhere\else ? or only something like
> C:\\foo\bar\more\specific.txt ?
> >
> > IMHO If absolute filenames can write anywhere on the file system, then
> > that's a bit of a security problem and it makes it a little pointless to
> > check relative paths for .. etc.
>
> If this was a user provided path I'd agree. But it isn't. The
> application is free to write to any part of the file system directly via
> the Java API so why not here?
>
> If the path does originate with the user then it is an application
> responsibility to sanitise it, not a container responsibility.
>
> If the application is not trusted to write to anywhere in the file
> system, that is what a security manager and/or OS file permissions are for.
>
> Finally, it think it is worth repeating that the primary reason Tomcat
> implemented it this way was for ease of migration from Commons
> FileUpload to the new Servlet API. The equivalent method in FileUpload
> allowed absolute paths so Tomcat did to.
>
> Mark
>
> > On 9 March 2017 at 07:20, Stuart Douglas <sdouglas_at_redhat.com
> > <mailto:sdouglas_at_redhat.com>> wrote:
> >
> > For what it is worth we also implement the Tomcat behavior.
> >
> > If we do make this behavior official we should probably put a warning
> > on the javadoc about it being up to the application to make sure the
> > path that is passed in is safe.
> >
> > For relative paths I think we should probably throw an exception if
> > the resulting path is outside the base path (e.g. files like
> > ../../../somefile.txt).
> >
> > Stuart
> >
> > On Wed, Mar 8, 2017 at 6:09 PM, Greg Wilkins <gregw_at_webtide.com
> > <mailto:gregw_at_webtide.com>> wrote:
> > >
> > > All,
> > >
> > > Janb has asked me to seek clarification on the Part.write(String)
> > method.
> > >
> > > The spec for @MultipartConfiguration.location says:
> > >
> > >> The location attribute of the
> > javax.servlet.annotation.MultipartConfig and
> > >> the <location> element of the <multipart-config> is interpreted
> as an
> > >> absolute path and defaults to the value of the
> > >> javax.servlet.context.tempdir. If a relative path is specified,
> > it will be
> > >> relative to the tempdir location. The test for absolute path vs
> > relative
> > >> path MUST be done via java.io.File.isAbsolute.
> > >
> > >
> > > The javadoc for Part.write(String fileName) says:
> > >
> > >> 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
> > >
> > >
> > > It is not clear whether fileName should be interpreted as a Path
> > or not, nor
> > > what should be the result if it specifies an absolute location, or
> > contains
> > > special path elements such as "." or "..".
> > >
> > > Should an absolute filename throw an IAE or should it just be
> > interpreted
> > > relative to the config location? What if the filename is
> > something like
> > > "C:\\foo\bar"?
> > >
> > >
> > > Tomcat have changed the javadoc on their version of the API to say:
> > >
> > >> @param fileName The location into which the uploaded part should
> be
> > >> stored. Relative locations are relative to {_at_link
> > >> javax.servlet.MultipartConfigElement#getLocation()}
> > >
> > >
> > > For which we have raised an issue:
> > > https://bz.apache.org/bugzilla/show_bug.cgi?id=60802
> > <https://bz.apache.org/bugzilla/show_bug.cgi?id=60802>
> > > and Spring Framework at least is expecting the tomcat behaviour as
> > reported
> > > in this Jetty bug:
> > https://github.com/eclipse/jetty.project/issues/1337
> > <https://github.com/eclipse/jetty.project/issues/1337>
> > >
> > >
> > > I don't mind the Tomcat version (modulo some security concerns
> about
> > > allowing the container to try to write to any old path), but would
> > prefer if
> > > the official version could clarify how absolute paths should be
> > handled.
> > >
> > > cheers
> > >
> > >
> > >
> > >
> > > --
> > > Greg Wilkins <gregw_at_webtide.com <mailto:gregw_at_webtide.com>> CTO
> > http://webtide.com
> >
> >
> >
> >
> > --
> > Greg Wilkins <gregw_at_webtide.com <mailto:gregw_at_webtide.com>> CTO
> > http://webtide.com
>
>
--
Greg Wilkins <gregw@webtide.com> CTO http://webtide.com