| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cfd18e0f0804171106r73a0495cpc610acfb7b2ebf7c@mail.gmail.com>
Date: Thu, 17 Apr 2008 20:06:37 +0200
From: "Michael Kerrisk" <mtk.manpages@...glemail.com>
To: "Ulrich Drepper" <drepper@...hat.com>
Cc: "Andrew Morton" <akpm@...ux-foundation.org>,
lkml <linux-kernel@...r.kernel.org>, linux-man@...r.kernel.org
Subject: Re: [PATCH] utimensat() non-conformances and fixes
Ulrich,
Ping! Could you please review this patch.
Cheers,
Michael
On Tue, Apr 8, 2008 at 12:18 AM, Michael Kerrisk
<mtk.manpages@...glemail.com> wrote:
> Ulrich,
>
> While writing a man page for utimensat(2) and futimens(3), I think I've
> found a few bugs in the utimensat() system call (i.e., non-conformances
> with either the specification in the draft POSIX.1-200x revision or
> traditional Linux behavior).
>
> int utimensat(int dirfd, const char *pathname,
> const struct timespec times[2], int flags);
>
> 1. The draft POSIX.1-200x specification for utimensat() says that if a
> times[n].tv_nsec field is UTIME_OMIT or UTIME_NOW, then the value in the
> corresponding tv_sec field is ignored. However the current Linux
> implementation requires the tv_sec value to be zero (or the EINVAL error
> results). This requirement should be removed.
>
> 2. The POSIX.1 draft says that to do anything other than setting both
> timestamps to a time other than the current time (i.e., times is not NULL,
> and both tv_nsec fields are not UTIME_NOW and both tv_nsec fields are not
> UTIME_OMIT -- see lines 32400 to 32403 of draft 5 of the spec), either: the
> caller's effective user ID must match the owner of the file; or the caller
> must have appropriate privileges. If this condition is violated, then the
> error EPERM results. However, the current implementation does not generate
> EPERM if one tv_nsec field is UTIME_NOW while the other is UTIME_OMIT -- it
> should give this error for that case.
>
> 3. Traditionally, utime()/utimes() gives the error EACCES for the case
> where the timestamp pointer argument is NULL (i.e., set both timestamps to
> the current time), and the file is owned by a user other than the effective
> UID of the caller, and the file is not writable by the effective UID of the
> program. utimensat() also gives this error in the same case. However, in
> the same circumstances, when utimensat() is given a 'times' array in which
> both tv_nsec fields are UTIME_NOW, which provides equivalent functionality
> to specifying 'times' as NULL, the call succeeds. I think that it should fail
> with the error EACCES in this case.
>
> 4. A further bug relates to traditional Linux behavior. Traditionally
> (i.e., utime(2) and utimes(2)), the EPERM error could also occur if the
> 'times' argument was non-NULL (i.e., we are setting the timestamps to a
> value other than the current time) and the file is marked as immutable or
> append-only. The current implementation also returns this error if 'times'
> is non-NULL and the tv_nsec fields are both UTIME_NOW. For consistency, the
>
> (times == NULL && times[0].tv_nsec == UTIME_NOW && times[1].tv_nsec ==
> UTIME_NOW)
>
> case should be treated like the traditional utimes() case where 'times'
> is NULL. That is, the call should succeed for a file marked append-only
> and should give the error EACCES if the file is marked as immutable.
>
> The first part of the patch below (made against 2.6.25-rc6, but still
> applies against rc8) addresses bugs 2, 3, and 4 and the second part of the
> patch addresses bug 1. Do you agree with my analyses and fixes?
>
> Following the patch, at the end of this mail is a command-line-driven test
> program that can be used to test the current implementation, and my patch.
> Some test cases below demonstrate the 4 cases described above, showing how
> a vanilla 2.6.25-rcN kernel behaves, and how it behaves with my patch applied.
>
> Finally, with my patch applied, I believe that the following lines could be
> removed from the sys_utimensat() routine (I've done some light testing to verify
> this, but more testing would be in order):
>
> /* Nothing to do, we must not even check the path. */
> if (tstimes[0].tv_nsec == UTIME_OMIT &&
> tstimes[1].tv_nsec == UTIME_OMIT)
> return 0;
>
> Cheers,
>
> Michael
>
> Signed-off-by: Michael Kerrisk <mtk.manpages@...il.com>
>
> Test cases
> ==========
>
> All test cases run as user 'mtk'
>
> Bug 1
> -----
>
> Behavior with vanilla 2.6.25-rcN kernel:
>
> $ ls -l u
> -r-------- 1 mtk users 0 Apr 7 21:39 u
> $ ./t_utimensat u 1 n 1 n
> dirfd is -100
> pathname is u
> tsp is 0xbfb41408; struct = { 1, 1073741823 } { 1, 1073741823 }
> flags is 0
> utimensat: Invalid argument
>
> Behavior with my patch applied:
>
> $ ls -l u
> -r-------- 1 mtk users 0 Apr 7 21:39 u
> $ ./t_utimensat u 1 n 1 n
> dirfd is -100
> pathname is u
> tsp is 0xbfab7378; struct = { 1, 1073741823 } { 1, 1073741823 }
> flags is 0
> utimensat() succeeded
> Last file access: Mon Apr 7 21:39:36 2008
> Last file modification: Mon Apr 7 21:39:36 2008
> Last status change: Mon Apr 7 21:39:36 2008
>
>
> Bug 2
> -----
>
> Behavior with vanilla 2.6.25-rcN kernel:
>
> $ ls -l p
> -rw-r--r-- 1 root root 0 Apr 7 10:34 p
> $ ./t_utimensat p 0 n 0 o
> dirfd is -100
> pathname is p
> tsp is 0xbfb7ac38; struct = { 0, 1073741823 } { 0, 1073741822 }
> flags is 0
> utimensat() succeeded
> Last file access: Mon Apr 7 22:00:51 2008
> Last file modification: Mon Apr 7 10:34:00 2008
> Last status change: Mon Apr 7 22:00:51 2008
>
>
> Behavior with my patch applied:
>
> $ ls -l p
> -rw-r--r-- 1 root root 0 Apr 7 10:34 p
> $ ./t_utimensat p 0 n 0 o
> dirfd is -100
> pathname is p
> tsp is 0xbfc40d08; struct = { 1, 1073741823 } { 1, 1073741822 }
> flags is 0
> utimensat failed with EPERM
>
>
> Bug 3
> -----
>
> Behavior with vanilla 2.6.25-rcN kernel:
>
> $ ls -l p
> -rw-r--r-- 1 root root 0 Apr 7 22:03 p
> $ ./t_utimensat p 0 n 0 n
> dirfd is -100
> pathname is p
> tsp is 0xbfd99658; struct = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat() succeeded
> Last file access: Mon Apr 7 22:03:31 2008
> Last file modification: Mon Apr 7 22:03:31 2008
> Last status change: Mon Apr 7 22:03:31 2008
> $ ./t_utimensat p
> dirfd is -100
> pathname is p
> tsp is (nil)
> flags is 0
> utimensat failed with EACCES
>
> Behavior with my patch applied:
>
> $ ls -l p
> -rw-r--r-- 1 root root 0 Apr 7 10:34 p
> $ ./t_utimensat p 0 n 0 n
> dirfd is -100
> pathname is p
> tsp is 0xbfa98358; struct = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat failed with EACCES
> $ ./t_utimensat p
> dirfd is -100
> pathname is p
> tsp is (nil)
> flags is 0
> utimensat failed with EACCES
>
>
> Bug 4
> -----
>
> Behavior with vanilla 2.6.25-rcN kernel:
>
> $ ls -l fa fi
> -rw-r--r-- 1 mtk users 0 Apr 7 21:43 fa
> -rw-r--r-- 1 mtk users 0 Apr 7 09:11 fi
> $ lsattr -l fa fi
> fa Append_Only
> fi Immutable
> $ ./t_utimensat fa 0 n 0 n
> dirfd is -100
> pathname is fa
> tsp is 0xbfc43508; struct = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat failed with EPERM
> $ ./t_utimensat fi 0 n 0 n
> dirfd is -100
> pathname is fi
> tsp is 0xbfb54c18; struct = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat failed with EPERM
>
>
> Behavior with my patch applied:
>
> $ ls -l fa fi
> -rw-r--r-- 1 mtk users 0 Apr 7 18:24 fa
> -rw-r--r-- 1 mtk users 0 Apr 7 09:11 fi
> $ lsattr -l fa fi
> fa Append_Only
> fi Immutable
> $ ./t_utimensat fa 0 n 0 n
> dirfd is -100
> pathname is fa
> tsp is 0xbfa7e338; struct = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat() succeeded
> Last file access: Mon Apr 7 21:43:23 2008
> Last file modification: Mon Apr 7 21:43:23 2008
> Last status change: Mon Apr 7 21:43:23 2008
> $ ./t_utimensat fi 0 n 0 n
> dirfd is -100
> pathname is fi
> tsp is 0xbfe8d748; struct = { 0, 1073741823 } { 0, 1073741823 }
> flags is 0
> utimensat failed with EACCES
>
>
>
> ==================================================
>
> --- linux-2.6.25-rc6-orig/fs/utimes.c 2008-04-07 22:25:08.000000000 +0200
> +++ linux-2.6.25-rc6/fs/utimes.c 2008-04-07 23:57:41.000000000 +0200
> @@ -95,27 +95,37 @@
>
> /* Don't worry, the checks are done in inode_change_ok() */
> newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
> - if (times) {
> + if (times && ! ((times[0].tv_nsec == UTIME_NOW &&
> + times[1].tv_nsec == UTIME_NOW) ||
> + (times[0].tv_nsec == UTIME_OMIT &&
> + times[1].tv_nsec == UTIME_OMIT))) {
> error = -EPERM;
> if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
> goto dput_and_out;
>
> - if (times[0].tv_nsec == UTIME_OMIT)
> - newattrs.ia_valid &= ~ATTR_ATIME;
> - else if (times[0].tv_nsec != UTIME_NOW) {
> + if (times[0].tv_nsec == UTIME_OMIT) {
> + newattrs.ia_atime = inode->i_atime;
> + newattrs.ia_valid |= ATTR_ATIME_SET;
> + } else if (times[0].tv_nsec != UTIME_NOW) {
> newattrs.ia_atime.tv_sec = times[0].tv_sec;
> newattrs.ia_atime.tv_nsec = times[0].tv_nsec;
> newattrs.ia_valid |= ATTR_ATIME_SET;
> }
>
> - if (times[1].tv_nsec == UTIME_OMIT)
> - newattrs.ia_valid &= ~ATTR_MTIME;
> - else if (times[1].tv_nsec != UTIME_NOW) {
> + if (times[1].tv_nsec == UTIME_OMIT) {
> + newattrs.ia_mtime = inode->i_mtime;
> + newattrs.ia_valid |= ATTR_MTIME_SET;
> + } else if (times[1].tv_nsec != UTIME_NOW) {
> newattrs.ia_mtime.tv_sec = times[1].tv_sec;
> newattrs.ia_mtime.tv_nsec = times[1].tv_nsec;
> newattrs.ia_valid |= ATTR_MTIME_SET;
> }
> +
> + } else if (unlikely(times && times[0].tv_nsec == UTIME_OMIT &&
> + times[1].tv_nsec == UTIME_OMIT)) {
> + newattrs.ia_valid &= ~(ATTR_ATIME | ATTR_MTIME | ATTR_CTIME);
> } else {
> + /* times is NULL, or both tv_nsec fields are UTIME_NOW */
> error = -EACCES;
> if (IS_IMMUTABLE(inode))
> goto dput_and_out;
> @@ -150,14 +160,6 @@
> if (utimes) {
> if (copy_from_user(&tstimes, utimes, sizeof(tstimes)))
> return -EFAULT;
> - if ((tstimes[0].tv_nsec == UTIME_OMIT ||
> - tstimes[0].tv_nsec == UTIME_NOW) &&
> - tstimes[0].tv_sec != 0)
> - return -EINVAL;
> - if ((tstimes[1].tv_nsec == UTIME_OMIT ||
> - tstimes[1].tv_nsec == UTIME_NOW) &&
> - tstimes[1].tv_sec != 0)
> - return -EINVAL;
>
> /* Nothing to do, we must not even check the path. */
> if (tstimes[0].tv_nsec == UTIME_OMIT &&
>
>
> ==================================================
>
> /* t_utimensat.c
>
> Copyright (C) 2008, Michael Kerrisk <mtk.manpages@...il.com>
>
> Licensed under the GPLv2 or later.
>
> A command-line interface for testing the utimensat() system
> call.
>
> 17 Mar 2008 Initial creation
> */
> #define _GNU_SOURCE
> #define _ATFILE_SOURCE
> #include <stdio.h>
> #include <time.h>
> #include <errno.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <fcntl.h>
> #include <string.h>
> #include <sys/stat.h>
>
> #define errExit(msg) do { perror(msg); exit(EXIT_FAILURE); \
> } while (0)
>
>
> #define __NR_utimensat 320 /* x86 syscall number */
>
> # define UTIME_NOW ((1l << 30) - 1l)
> # define UTIME_OMIT ((1l << 30) - 2l)
>
> static inline int
> utimensat(int dirfd, const char *pathname,
> const struct timespec times[2], int flags)
> {
> return syscall(__NR_utimensat, dirfd, pathname, times, flags);
> }
>
> static void
> usageError(char *progName)
> {
> fprintf(stderr, "Usage: %s pathname [atime-sec "
> "atime-nsec mtime-sec mtime-nsec]\n\n", progName);
> fprintf(stderr, "Permitted options are:\n");
> fprintf(stderr, " [-d path] "
> "open a directory file descriptor"
> " (instead of using AT_FDCWD)\n");
> fprintf(stderr, " -w Open directory file "
> "descriptor with O_RDWR (instead of O_RDONLY)\n");
> fprintf(stderr, " -n Use AT_SYMLINK_NOFOLLOW\n");
> fprintf(stderr, "\n");
>
> fprintf(stderr, "pathname can be \"NULL\" to use NULL "
> "argument in call\n");
> fprintf(stderr, "\n");
>
> fprintf(stderr, "Either nsec field can be\n");
> fprintf(stderr, " 'n' for UTIME_NOW\n");
> fprintf(stderr, " 'o' for UTIME_OMIT\n");
> fprintf(stderr, "\n");
>
> fprintf(stderr, "If the time fields are omitted, "
> "then a NULL 'times' argument is used\n");
> fprintf(stderr, "\n");
>
> exit(EXIT_FAILURE);
> }
>
> int
> main(int argc, char *argv[])
> {
> int flags, dirfd, opt, oflag;
> struct timespec ts[2];
> struct timespec *tsp;
> char *pathname, *dirfdPath;
> struct stat sb;
>
> flags = 0;
> dirfd = AT_FDCWD;
> dirfdPath = NULL;
> oflag = O_RDONLY;
>
> while ((opt = getopt(argc, argv, "d:nw")) != -1) {
> switch (opt) {
> case 'd':
> dirfdPath = optarg;
> break;
>
> case 'n':
> flags |= AT_SYMLINK_NOFOLLOW;
> printf("Not following symbolic links\n");
> break;
>
> case 'w':
> oflag = O_RDWR;
> break;
>
> default:
> usageError(argv[0]);
> }
> }
>
> if ((optind + 5 != argc) && (optind + 1 != argc))
> usageError(argv[0]);
>
> if (dirfdPath != NULL) {
> dirfd = open(dirfdPath, oflag);
> if (dirfd == -1) errExit("open");
>
> printf("Opened dirfd");
> printf(" O_RDWR");
> printf(": %s\n", dirfdPath);
> }
>
> pathname = (strcmp(argv[optind], "NULL") == 0) ?
> NULL : argv[optind];
>
> if (argc == optind + 1) {
> tsp = NULL;
>
> } else {
> ts[0].tv_sec = atoi(argv[optind + 1]);
> if (argv[optind + 2][0] == 'n') {
> ts[0].tv_nsec = UTIME_NOW;
> } else if (argv[optind + 2][0] == 'o') {
> ts[0].tv_nsec = UTIME_OMIT;
> } else {
> ts[0].tv_nsec = atoi(argv[optind + 2]);
> }
>
> ts[1].tv_sec = atoi(argv[optind + 3]);
> if (argv[optind + 4][0] == 'n') {
> ts[1].tv_nsec = UTIME_NOW;
> } else if (argv[optind + 4][0] == 'o') {
> ts[1].tv_nsec = UTIME_OMIT;
> } else {
> ts[1].tv_nsec = atoi(argv[optind + 4]);
> }
>
> tsp = ts;
> }
>
> /* For testing purposes, it may have been useful to run this
> program as set-user-ID-root so that a directory file
> descriptor could be opened as root. Now we reset to the
> real UID before making the utimensat() call, so that the
> permission checking is performed under that UID. */
>
> if (geteuid() == 0) {
> uid_t u;
>
> u = getuid();
>
> printf("Resettng UIDs to %ld\n", (long) u);
>
> if (setresuid(u, u, u) == -1)
> errExit("setresuid");
> }
>
> printf("dirfd is %d\n", dirfd);
> printf("pathname is %s\n", pathname);
> printf("tsp is %p", tsp);
> if (tsp != NULL) {
> printf("; struct = { %ld, %ld } { %ld, %ld }",
> (long) tsp[0].tv_sec, (long) tsp[0].tv_nsec,
> (long) tsp[1].tv_sec, (long) tsp[1].tv_nsec);
> }
> printf("\n");
> printf("flags is %d\n", flags);
>
> if (utimensat(dirfd, pathname, tsp, flags) == -1) {
> if (errno == EPERM)
> printf("utimensat failed with EPERM\n");
> else if (errno == EACCES)
> printf("utimensat failed with EACCES\n");
> else
> perror("utimensat");
> exit(EXIT_FAILURE);
> }
>
> printf("utimensat() succeeded\n");
>
> if (stat(pathname, &sb) == -1) errExit("stat");
> printf("Last file access: %s", ctime(&sb.st_atime));
> printf("Last file modification: %s", ctime(&sb.st_mtime));
> printf("Last status change: %s", ctime(&sb.st_ctime));
>
> exit(EXIT_SUCCESS);
> }
>
--
Michael Kerrisk
Maintainer of the Linux man-pages project
http://www.kernel.org/doc/man-pages/
Want to report a man-pages bug? Look here:
http://www.kernel.org/doc/man-pages/reporting_bugs.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists