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.
