[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR[ 9u-dev] JDK-8075773 - jps running as root fails with 7u75, works fine with 7u72
From: "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date: 2015-05-21 1:24:59
Message-ID: 555D33EB.9070808 () oracle ! com
[Download RAW message or body]
On 5/18/15 3:59 AM, 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/
Cheleswer,
Sorry for another 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:
200 // Check if the given statbuf is considered a secure directory for
201 // the backing store files. Returns true if the directory is
considered
202 // a secure location. Returns false if the statbuf is a symbolic
link or
203 // if an error occurred.
204 //
205 static bool is_statbuf_secure(struct stat *statp) {
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 // We have an existing directory, check if the permissions are safe.
213 //
214 if ((statp->st_mode & (S_IWGRP|S_IWOTH)) != 0) {
215 // The directory is open for writing and could be subjected
216 // to a symlink or a hard link attack. Declare it insecure.
217 //
218 return false;
219 }
220 //If user is not root then see if the uid of the directory
matches the effective uid of the process.
221 uid_t euid = geteuid();
222 if ((euid !=0) && (statp->st_uid != euid)) {
223 // The directory was not created by this user, declare it
insecure.
224 //
225 return false;
226 }
227 return true;
228 }
The udiffs make it easier to see what was changed:
- // See if the uid of the directory matches the effective uid of the
process.
- //
- if (statp->st_uid != geteuid()) {
+ //If user is not root then see if the uid of the directory matches
the effective uid of the process.
+ uid_t euid = geteuid();
+ if ((euid !=0) && (statp->st_uid != euid)) {
Line 222 allows the root user to by-pass the security check (an
ownership check) on the remainder of the line. This is a very narrow
and focused policy by-pass.
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()
- make_user_tmp_dir()
- mmap_attach_shared()
is_dirfd_secure() is called by:
- open_directory_secure()
open_directory_secure() is called by:
- open_directory_secure_cwd()
- get_user_name_slow()
get_user_name_slow() analysis
-----------------------------
The new security policy by-pass will allow get_user_name_slow()
for the 'root' user:
- to process directory entries in a directory that is writable
by the owner which makes this use subject to a symlink or
hard link attack.
- to process directory entries in a directory that is owned
by another user.
It looks like the get_user_name_slow() code is written safely
enough such that any symlink or hard link attack on the 'root'
user by the directory owner should not cause any issues.
The 'root' user will be able to determine the user name
associated with VMs owned by another user which is exactly
the intent of the policy by-pass.
make_user_tmp_dir() analysis
----------------------------
The new security policy by-pass will allow make_user_tmp_dir()
for the 'root' user:
- to use an existing directory that was created by another
user as the 'root' user's specific temporary directory
- the directory is writable by the owner which makes this use
subject to a symlink or hard link attack.
The caller of make_user_tmp_dir() is create_sharedmem_resources()
and it has to be analyzed to see if properly guards against a
symlink and/or hardlink attack.
create_sharedmem_resources() analysis
-------------------------------------
The new security policy by-pass will allow create_sharedmem_resources()
for the 'root' user:
- to accept use of an existing directory that was created by
another user as the 'root' user's specific temporary directory
(via a call to make_user_tmp_dir() and a call to
open_directory_secure_cwd())
- to accept use of an existing file that was created by another
user as the 'root' user's specific shared memory file
create_sharedmem_resources() guards against symlink attack by
using the O_NOFOLLOW option when it opens the shared memory file.
It guards against hard-link attack by checking for a link count
greater than '1' after opening the file, but before it truncates
the file.
mmap_attach_shared() analysis()
-------------------------------
The new security policy by-pass will allow mmap_attach_shared()
for the 'root' user:
- to use an existing directory that was created by another
user as the 'root' user's specific temporary directory
- the directory is writable by the owner which makes this use
subject to a symlink or hard link attack.
- to accept use of an existing file that was created by another
user as the 'root' user's specific shared memory file
- to process directory entries in a directory that is writable
by the owner which makes this use subject to a symlink or
hard link attack.
- to process directory entries in a directory that is owned
by another user.
mmap_attach_shared() guards against symlink attack by using the
O_NOFOLLOW option when it opens the shared memory file. It guards
against hard-link attack by checking for a link count greater
than '1' 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.
Summary:
This implementation has a much narrower and focused policy
by-pass. The by-pass allows the 'root' user to access and
use the shared memory files (PerfData) for other users. It
also allows the 'root' user to use a shared memory file for
itself that was created by another user. By using PerfData
files for other users and by using a PerfData file for itself
that can be modified by another user, the 'root' user is
exposed to corrupted data and/or OOM attacks.
It looks like a denial-of-service attack could be mounted
here. The attack prevents the 'root' user from using the
'jps' cmd to query the various VMs on the system by a user
hand crafting a corrupted PerfData file that will cause
'jps' to OOM crash.
Again, sorry for the lengthy review.
Dan
>
> 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
>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic