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
