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

[Volker Simonis]

On Mon, Jul 20, 2015 at 4:18 PM, Daniel D. Daugherty
<daniel.daugherty at oracle.com> wrote:
> On 7/17/15 4:58 PM, Volker Simonis wrote:
>
>
>
> On Saturday, July 18, 2015, Daniel D. Daugherty
> <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/
>>
>>
>> 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.
>

Hi Dan,

have you come to a conclusion?

Regards,
Volker

>
> 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
>>>
>>>
>>>
>>
>