Miklos, Al, Ulrich, Could you please review the following patch. This is a revised version of my earlier (http://article.gmane.org/gmane.linux.man/129/ ) patch to fix non-conformances in the utimensat() implementation. The patch is against 2.6.22-rc2. Miklos wrote a patch that is already in 2.6.22-rc2 to fix the security issues that he saw from my earlier mail. Miklos's patch also introduced a few spec non-conformances, but provided me with some pointers to how to improve this version of my patch. The following paragraphs summarize the rules that this patch implements: Historical permissions rules for target file (utime(), utimes()): [a] The EACCES error (only) occurs if times is NULL: The times argument is a null pointer and the effective user ID of the process does not match the owner of the file and write access is denied. [b] The EPERM error (only) occurs if times is not NULL (i.e., both times are being changed): The times argument is not a null pointer, the calling process' effective user ID does not match the owner of the file, and the calling process does not have appropriate privileges. (As noted in a thread on the security@ list, the current spec for utimensat() needlessly makes mention of "write access" for the EPERM error. I've raise a bug against the spec, and it's recognized as something that needs to be fixed.) My summary of the rules from the draft spec for utimensat() in the upcoming POSIX.1 revision: [c] No error needs to be generated if times == {UTIME_OMIT,UTIME_OMIT}. [d] The times == {UTIMES_NOW,UTIMES_NOW} case should be treated like times == NULL. [e] The times == {UTIMES_NOW,UTIMES_OMIT} and times == {UTIMES_OMIT,UTIMES_NOW} cases should be treated like times == {m,n}. Further historical Linux rules, for files with "ext2" extended file attributes (see chattr(1)). [f] Append-only attribute set: If times == NULL, and we own the file, then the call should succeed. [g] Immutable attribute set: the call should fail, but the error depends on the value in times. The following table shows the expected results for various cases, and indicates places where 2.6.26-rc2 deviates from those results. The key for the columns is shown at the end of the table. All of these cases (as well as many others) have been tested with my patch, and conform to the rules above. (See http://article.gmane.org/gmane.linux.man/129/ for the test program used.) ==================================================== times Owner? File Expected 2.6.26-rc2 arg Writable? Result deviation N o w success N o !w success N !o w success !N-n-n !o w success N !o !w EACCES [1] !N-n-n !o !w EACCES [1a] !N o w success !N o !w success !N !o w EPERM [2] !N-o-n !o w EPERM [2a] success !N !o !w EPERM [3] !N-o-n !o !w EPERM [3a] EACCES (Append-only attribute set, file writable) N o append success [4] !N-n-n o append success [4a] EPERM !N-n-o o append EPERM [5] (Immutable attribute set, file writable) N o immutable EACCES [6] !N-n-n o immutable EACCES [6a] EPERM !N-n-o o immutable EPERM [7] ==================================================== Key to table columns: times arg: N times is NULL !N times is not NULL, and at least one of the fields is a value other than UTIME_OMIT or UTIME_NOW !N-n-n times == {UTIME_NOW,UTIME_NOW} !N-n-o times == {UTIME_NOW,UTIME_OMIT} || times == {UTIME_OMIT,UTIME_NOW} Owner: o Process EUID is owner of file !o Process EUID is not owner of file File writable w Process has permission to write to file !w Process does not have permission to write to file The "Expected result" column shows either "success" or expected value in errno after failure Labels in [] are provided for my own reference, and in case anyone wants to refer to specific cases in discussing the patch. Cheers, Michael Signed-off-by: Michael Kerrisk <mtk.manpages@xxxxxxxxx> --- linux-2.6.26-rc2/fs/utimes.c 2008-05-15 10:33:20.000000000 +0200 +++ linux-2.6.26-rc2-mtk/fs/utimes.c 2008-05-16 07:33:02.000000000 +0200 @@ -40,14 +40,9 @@ #endif -static bool nsec_special(long nsec) -{ - return nsec == UTIME_OMIT || nsec == UTIME_NOW; -} - static bool nsec_valid(long nsec) { - if (nsec_special(nsec)) + if (nsec == UTIME_OMIT || nsec == UTIME_NOW) return true; return nsec >= 0 && nsec <= 999999999; @@ -106,7 +101,9 @@ newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME; if (times) { error = -EPERM; - if (IS_APPEND(inode) || IS_IMMUTABLE(inode)) + if (!(times[0].tv_nsec == UTIME_NOW && + times[1].tv_nsec == UTIME_NOW) && + (IS_IMMUTABLE(inode) || IS_APPEND(inode))) goto mnt_drop_write_and_out; if (times[0].tv_nsec == UTIME_OMIT) @@ -131,9 +128,10 @@ * UTIME_NOW, then need to check permissions, because * inode_change_ok() won't do it. */ - if (!times || (nsec_special(times[0].tv_nsec) && - nsec_special(times[1].tv_nsec))) { + if (!times || (times[0].tv_nsec == UTIME_NOW && + times[1].tv_nsec == UTIME_NOW)) { error = -EACCES; + if (IS_IMMUTABLE(inode)) goto mnt_drop_write_and_out; @@ -147,7 +145,20 @@ goto mnt_drop_write_and_out; } } + } else if (times && ((times[0].tv_nsec == UTIME_NOW && + times[1].tv_nsec == UTIME_OMIT) + || + (times[0].tv_nsec == UTIME_OMIT && + times[1].tv_nsec == UTIME_NOW))) { + error = -EPERM; + + if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) + goto mnt_drop_write_and_out; + + if (!is_owner_or_cap(inode)) + goto mnt_drop_write_and_out; } + mutex_lock(&inode->i_mutex); error = notify_change(dentry, &newattrs); mutex_unlock(&inode->i_mutex); -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
