RFR[ 9u-dev] JDK-8075773 - jps running as root fails with 7u75, works fine with 7u72
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon May 18 22:59:53 UTC 2015
Cheleswer,
This review is going to require another detailed code path analysis
to see what the root user would be exposed to with this policy
by-pass.
Short version of what I'm worried about:
The root user is potentially accessing the perf-data directory
and/or perf-data files that it does not own. The root user may
be exposed to attacks by the owner(s) of the perf-data directory
and/or perf-data files.
I'll need to redo a wider version of the code path analysis in
order to see if there is exposure here.
Dan
On 5/18/15 5:39 AM, Dmitry Samersoff wrote:
> Cheleswer,
>
> 1. perfMemory_linux.cpp:222 missed space in !=0)
>
> 2. We have couple of other perfMemory_*.cpp files that have to be
> updated as well.
>
> -Dmitry
>
> On 2015-05-18 12:59, cheleswer sahu wrote:
>> 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
>
More information about the serviceability-dev
mailing list