RFR(S): 8130910: linux/unix: hsperfdata file created in cwd and not cleaned up if /tmp/hsperfdata_<username> has wrong permissions

[Daniel D. Daugherty]

On 7/17/15 4:58 PM, Volker Simonis wrote:
>
>
> On Saturday, July 18, 2015, Daniel D. Daugherty 
> <daniel.daugherty at oracle.com <mailto:daniel.daugherty at oracle.com>> wrote:
>
>     On 7/16/15 9:18 AM, Langer, Christoph wrote:
>
>         Hi all,
>
>
>
>         I don't know if this mailing list is the right one for this
>         issue. Let me know if I should post it elsewhere.
>
>
>
>         I have prepared a fix for an issue with /tmp/hsperfdata files.
>
>
>
>         Please review this change.  I also need a sponsor.
>
>
>
>         Bug: https://bugs.openjdk.java.net/browse/JDK-8130910
>
>         Change:
>         http://cr.openjdk.java.net/~simonis/webrevs/2015/8130910/
>         <http://cr.openjdk.java.net/%7Esimonis/webrevs/2015/8130910/>
>
>
>     src/os/aix/vm/perfMemory_aix.cpp
>         No comments.
>
>     src/os/bsd/vm/perfMemory_bsd.cpp
>         No comments.
>
>     src/os/linux/vm/perfMemory_linux.cpp
>         No comments.
>
>     src/os/solaris/vm/perfMemory_solaris.cpp
>         No comments.
>
>
>     I think I'm OK with the code changes above, but I need more
>     time to mull on them. I also need time to mull on the stuff
>     down below.
>
> Hi Dan,
>
> thanks for looking at this change.
>
> Maybe I can add some more context here. Christoph detected this issue 
> when somebody who ran the VM in a security context with a umask which 
> masked out all the executable bits of newly created files complained 
> about the stale  files in the current working directory.

Thanks for the context.


>
> I think that Chritoph's change is good

I concur that the change is good. I'm simply mulling on potential
side effects.


> and I also share his doubts about changing the current working 
> directory. I think this is questionable because it can have non-local 
> side effects but I think this should be fixed in a follow up change.

Changing the working directory can definitely have non-local side
effects, but the change of directory is a necessary part of the
secure file system object creation protocol. Jerry has a good
comment that explains what APIs we would rather use than changing
the current working directory, but those APIs don't exist on all
platforms (yet).

  348 // NOTE: The code below uses fchdir(), open() and unlink() because
  349 // fdopendir(), openat() and unlinkat() are not supported on all
  350 // versions.  Once the support for fdopendir(), openat() and 
unlinkat()
  351 // is available on all supported versions the code can be changed
  352 // to use these functions.
  353
  354 // Open the directory of the given path, validate it and set the
  355 // current working directory to it.
  356 // Return a DIR * of the open directory and the saved cwd fd.
  357 //
  358 static DIR *open_directory_secure_cwd(const char* dirname, int 
*saved_cwd_fd) {

The protocol is basically:

   open_directory_secure_cwd()
   do operations to files in the directory using relative paths
   close_directory_secure_cwd()

The alternative of using full paths is difficult to do safely without
exposure to attacks along exposed path elements.

Dan


>
> Thanks,
> Volker
>
>     Will get back to this next week.
>
>     Dan
>
>
>
>
>
>         The problem is that for creating a file in
>         /tmp/hsperfdata_<username>, an fchdir to this directory is
>         attempted. In case there's no execute permission on
>         /tmp/hsperfdata_<username>, the fchdir fails. As its return
>         code is not checked up to now, the process would stay in its
>         current working directory and create the file in there. The
>         location stored for cleaning up at the end of the VM is also
>         remembered as /tmp/hsperfdata... so the file would remain left
>         over eventually. There are several checks for the hsperfdata
>         location in place but nothing would hit and prevent the fchdir
>         attempt beforehand in this case.
>
>
>
>         I fixed it by handling fchdir return code and in case of
>         failure closing the handles and returning NULL. In that case
>         we wouldn't have shared PerfMemory but I think that is ok then.
>
>
>
>         I thought about a testcase but it would involve messing around
>         with /tmp/hsperfdata permissions which can have effects on
>         other running JVMs, too, which I consider a bit dangerous.
>
>
>
>         Generally I don't know if it is a good idea to do an fchdir at
>         all but I don't know so much about the backgrounds of it...
>         maybe someone wants to comment on that as well.
>
>
>
>         Thanks and best regards,
>
>         Christoph
>
>
>
>