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

[Daniel D. Daugherty]

Volker,

Sorry I didn't get back to this thread before I went on vacation.
I'll try to refresh the thread in my brain and get back to you
guys shortly...

Dan


On 8/7/15 6:39 AM, Volker Simonis wrote:
> 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
>>>>
>>>>
>>>>