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

[cheleswer sahu]

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