RFR[ 9u-dev] JDK-8075773 - jps running as root fails with 7u75, works fine with 7u72

[cheleswer sahu]

Hi,
I have fixed the code and tested. It's working fine. Please review the 
changes.
Web review link: http://cr.openjdk.java.net/~dbuck/8075773/webrev.02/

Regards,
Cheleswer

On 5/15/2015 12:26 PM, cheleswer sahu wrote:
> Dear Dan & Dmitry,
> Thanks for pointing out the security vulnerability in the fix and for 
> your precise review. I am also agree with Dmitry's fix. I will fix the 
> code and resend the review request.
>
> Regards,
> Cheleswer
>
> On 5/14/2015 9:46 PM, Gerald Thornbrugh wrote:
>> Hi Dan,
>>
>> When Cheleswer and I discussed this fix my interpretation had a 
>> slightly different goal:
>>
>> Prior to the initial security fix any user could execute "jps" and 
>> get the user names associated
>> with other user's perf data (i.e. the call to get_user_name_slow() 
>> would succeed.). My initial
>> thought was that this was a regression for all users not just "root" 
>> and this goal led to this fix.
>> At the time I did not see this as a security vulnerability, your 
>> review has changed my mind.
>>
>> I agree that Dmitry's fix is a more secure fix for the issue and I 
>> think we should use it.
>>
>> Let me know if you have any questions.
>>
>> Thanks!
>>
>> Jerry
>>
>>> Hi,
>>> Please review the code changes for
>>> https://bugs.openjdk.java.net/browse/JDK-8075773. I have built and
>>> tested JDK9 with fix successfully. As I do not have an account for
>>> OpenJDK,
>>> David Buck will push the fix into jdk9/hs-rt/.
>>>
>>> Web review link: http://cr.openjdk.java.net/~dbuck/8075773/webrev.01/
>>>
>>> Regards,
>>> Cheleswer
>> Cheleswer,
>>
>> Sorry for the lengthy review, but since this is security related,
>> I have to be complete.
>>
>> The goal: Add a policy by-pass for the 'root' user in order to
>>       fix the regression in jps(1) behavior.
>>
>> The core of this policy by-pass is the change to this function:
>>
>>    205 static bool is_statbuf_secure(struct stat *statp, int mode) {
>>    206   if (S_ISLNK(statp->st_mode) || !S_ISDIR(statp->st_mode)) {
>>    207     // The path represents a link or some non-directory file 
>> type,
>>    208     // which is not what we expected. Declare it insecure.
>>    209     //
>>    210     return false;
>>    211   }
>>    212   // If the directory is going to be opened readonly, we consider
>> this as secure operation
>>    213   // we do not need to do any more checks.
>>    214   //
>>    215   if ((mode & O_ACCMODE) == O_RDONLY) {
>>    216     return true;
>>    217   }
>>    218   // We have an existing directory, check if the permissions 
>> are safe.
>>    219   //
>>    220   if ((statp->st_mode & (S_IWGRP|S_IWOTH)) != 0) {
>>    221     // The directory is open for writing and could be subjected
>>    222     // to a symlink or a hard link attack. Declare it insecure.
>>    223     //
>>    224     return false;
>>    225   }
>>    226   // See if the uid of the directory matches the effective uid of
>> the process.
>>    227   //
>>    228   if (statp->st_uid != geteuid()) {
>>    229     // The directory was not created by this user, declare it
>> insecure.
>>    230     //
>>    231     return false;
>>    232   }
>>    233   return true;
>>    234 }
>>
>> Lines 212-217 are added which allows a caller that passes in O_RDONLY
>> to by-pass the security checks on lines 220-225 and 228-232. This
>> implementation is using an attribute of _how_ the data is accessed
>> to override security policy instead of an attribute of _who_ is
>> accessing the data.
>>
>> Here are the code paths that access the modified policy code:
>>
>> is_statbuf_secure() is called by:
>>
>> - is_directory_secure()
>> - is_dirfd_secure()
>>
>> is_directory_secure() is called by:
>>
>> - get_user_name_slow() with O_RDONLY
>> - make_user_tmp_dir() with O_RDWR
>> - mmap_attach_shared() with (O_RDONLY | O_NOFOLLOW)
>>
>> is_dirfd_secure() is called by:
>>
>> - open_directory_secure() with a mode parameter
>>
>> open_directory_secure() is called by:
>>
>> - open_directory_secure_cwd() with O_RDWR
>> - get_user_name_slow() with O_RDONLY
>>
>> Only the code paths that specify O_RDWR make use of
>> the new policy by-pass code so it looks like only
>>
>> - get_user_name_slow() with O_RDONLY
>> - mmap_attach_shared() with (O_RDONLY | O_NOFOLLOW)
>>
>> are interesting.
>>
>> The new security policy by-pass will allow get_user_name_slow():
>>
>> - to process directory entries in a directory that is writable
>>     which makes this use subject to a symlink or hard link attack.
>> - to process directory entries in a directory that the calling
>>     user does not own; the intent of the policy by-pass is to
>>     allow this for the 'root' user, but this implementation
>>     allows the by-pass for any user.
>>
>> It looks like the get_user_name_slow() code is written safely
>> enough such that any symlink or hard link attack should not
>> cause any issues.
>>
>> The new policy by-pass will allow any user to determine the
>> user name associated with VMs owned by another user. This is
>> a broader policy by-pass than was intended.
>>
>>
>> The new security policy by-pass will allow mmap_attach_shared():
>>
>> - to process directory entries in a directory that is writable
>>     which makes this use subject to a symlink or hard link attack.
>> - to process directory entries in a directory that the calling
>>     user does not own; the intent of the policy by-pass is to
>>     allow this for the 'root' user, but this implementation allows
>>     the by-pass for any user.
>>
>> The mmap_attach_shared() code protects itself from a symlink
>> attack by including the 'O_NOFOLLOW' flag when opening the
>> PerfData file and it protects itself from a hardlink attack by
>> checking the hard link count after opening the file. It does
>> not protect itself against being handed a corrupted file or
>> even a very large file that would cause an OOM when the VM
>> tries to map what is supposed to be a PerfData file.
>>
>> The new policy by-pass will allow any user to access the
>> PerfData file associated with VMs owned by another user. This
>> is a broader policy by-pass than was intended.
>>
>>
>> Summary:
>>
>> This implementation of the new security policy by-pass is using
>> an attribute of _how_ the data is accessed to override security
>> policy instead of an attribute of _who_ is accessing the data.
>> This allows the VM to be exposed to some of the attacks that
>> the following fix was designed to prevent:
>>
>>       JDK-8050807 Better performing performance data handling
>>
>> Dmitry's response to the code review provides a solution that
>> is based on who is accessing the data and that solution or
>> one like it should be considered.
>>
>> Again, sorry for the lengthy review.
>>
>> Dan
>