RFR[ 9u-dev] JDK-8075773 - jps running as root fails with 7u75, works fine with 7u72
Dmitry Samersoff
dmitry.samersoff at oracle.com
Mon May 18 11:39:21 UTC 2015
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
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the serviceability-dev
mailing list