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
