From de79a8052c319c2e807debe0af948c0da4828542 Mon Sep 17 00:00:00 2001 From: guenther Date: Sat, 13 Dec 2014 20:59:24 +0000 Subject: [PATCH] Correct the handling of symbolic links by chmod/chgrp/chown/chflags, making them more resistant to TOCTOU race conditions too. ok tobias@ --- bin/chmod/chflags.1 | 11 ++----- bin/chmod/chmod.1 | 13 +++----- bin/chmod/chmod.c | 77 +++++++++++++++++++++++++-------------------- 3 files changed, 49 insertions(+), 52 deletions(-) diff --git a/bin/chmod/chflags.1 b/bin/chmod/chflags.1 index b0c2706d240..94b17546f4c 100644 --- a/bin/chmod/chflags.1 +++ b/bin/chmod/chflags.1 @@ -1,4 +1,4 @@ -.\" $OpenBSD: chflags.1,v 1.14 2014/01/28 14:15:03 jmc Exp $ +.\" $OpenBSD: chflags.1,v 1.15 2014/12/13 20:59:24 guenther Exp $ .\" $NetBSD: chflags.1,v 1.4 1995/08/31 22:50:22 jtc Exp $ .\" .\" Copyright (c) 1989, 1990, 1993, 1994 @@ -33,7 +33,7 @@ .\" .\" @(#)chflags.1 8.4 (Berkeley) 5/2/95 .\" -.Dd $Mdocdate: January 28 2014 $ +.Dd $Mdocdate: December 13 2014 $ .Dt CHFLAGS 1 .Os .Sh NAME @@ -138,13 +138,6 @@ For example: .Pp .Dl nouchg the immutable bit should be cleared .Pp -Symbolic links do not have flags, so unless the -.Fl H -or -.Fl L -option is set, -.Nm -on a symbolic link always succeeds and has no effect. The .Fl H , .Fl L , diff --git a/bin/chmod/chmod.1 b/bin/chmod/chmod.1 index 11749ace923..346da0b4fc8 100644 --- a/bin/chmod/chmod.1 +++ b/bin/chmod/chmod.1 @@ -1,4 +1,4 @@ -.\" $OpenBSD: chmod.1,v 1.38 2014/01/25 15:43:40 jmc Exp $ +.\" $OpenBSD: chmod.1,v 1.39 2014/12/13 20:59:24 guenther Exp $ .\" $NetBSD: chmod.1,v 1.8 1995/03/21 09:02:07 cgd Exp $ .\" .\" Copyright (c) 1989, 1990, 1993, 1994 @@ -33,7 +33,7 @@ .\" .\" @(#)chmod.1 8.4 (Berkeley) 3/31/94 .\" -.Dd $Mdocdate: January 25 2014 $ +.Dd $Mdocdate: December 13 2014 $ .Dt CHMOD 1 .Os .Sh NAME @@ -80,13 +80,8 @@ change the mode of the directory and all the files and directories in the file hierarchy below it. .El .Pp -Symbolic links do not have modes, so unless the -.Fl H -or -.Fl L -option is set, -.Nm -on a symbolic link always succeeds and has no effect. +Symbolic links have modes, +but those modes have no effect on the kernel's access checks. The .Fl H , .Fl L , diff --git a/bin/chmod/chmod.c b/bin/chmod/chmod.c index 4e5872421d0..a1d7c240beb 100644 --- a/bin/chmod/chmod.c +++ b/bin/chmod/chmod.c @@ -1,4 +1,4 @@ -/* $OpenBSD: chmod.c,v 1.32 2014/12/13 10:26:48 tobias Exp $ */ +/* $OpenBSD: chmod.c,v 1.33 2014/12/13 20:59:24 guenther Exp $ */ /* $NetBSD: chmod.c,v 1.12 1995/03/21 09:02:09 cgd Exp $ */ /* @@ -35,6 +35,7 @@ #include #include +#include #include #include #include @@ -61,7 +62,7 @@ main(int argc, char *argv[]) unsigned long val; int oct; mode_t omode; - int Hflag, Lflag, Rflag, ch, fflag, fts_options, hflag, rval; + int Hflag, Lflag, Rflag, ch, fflag, fts_options, hflag, rval, atflags; uid_t uid; gid_t gid; u_int32_t fclear, fset; @@ -99,13 +100,6 @@ main(int argc, char *argv[]) fflag = 1; break; case 'h': - /* - * In System V (and probably POSIX.2) the -h option - * causes chmod to change the mode of the symbolic - * link. 4.4BSD's symbolic links don't have modes, - * so it's an undocumented noop. Do syntax checking, - * though. - */ hflag = 1; break; /* @@ -137,6 +131,12 @@ done: if (argc < 2) usage(); + /* + * We alter the symlink itself if doing -h or -RP, or + * if doing -RH and the symlink wasn't a command line arg. + */ + atflags = AT_SYMLINK_NOFOLLOW; + fts_options = FTS_PHYSICAL; if (Rflag) { if (hflag) @@ -147,8 +147,10 @@ done: if (Lflag) { fts_options &= ~FTS_PHYSICAL; fts_options |= FTS_LOGICAL; + atflags = 0; } - } + } else if (!hflag) + atflags = 0; if (ischflags) { flags = *argv; @@ -235,37 +237,44 @@ done: case FTS_SLNONE: /* * The only symlinks that end up here are ones that - * don't point to anything and ones that we found - * doing a physical walk. + * don't point to anything or that loop and ones + * that we found doing a physical walk. */ - if (ischflags || ischmod || !hflag) + if (!hflag && (fts_options & FTS_LOGICAL)) continue; break; default: break; } - if (ischflags) { - if (oct) { - if (!chflags(p->fts_accpath, fset)) - continue; - } else { - p->fts_statp->st_flags |= fset; - p->fts_statp->st_flags &= fclear; - if (!chflags(p->fts_accpath, p->fts_statp->st_flags)) - continue; - } - warn("%s", p->fts_path); - rval = 1; - } else if (ischmod && chmod(p->fts_accpath, oct ? omode : - getmode(set, p->fts_statp->st_mode)) && !fflag) { - warn("%s", p->fts_path); - rval = 1; - } else if (!ischmod && !ischflags && - (hflag ? lchown(p->fts_accpath, uid, gid) : - chown(p->fts_accpath, uid, gid)) && !fflag) { - warn("%s", p->fts_path); - rval = 1; + + /* + * For -RH, the decision of how to handle symlinks depends + * on the level: follow it iff it's a command line arg. + */ + if (fts_options & FTS_COMFOLLOW) { + atflags = p->fts_level == FTS_ROOTLEVEL ? 0 : + AT_SYMLINK_NOFOLLOW; } + + if (ischmod) { + if (!fchmodat(AT_FDCWD, p->fts_accpath, oct ? omode : + getmode(set, p->fts_statp->st_mode), atflags) + || fflag) + continue; + } + else if (!ischflags) { + if (!fchownat(AT_FDCWD, p->fts_accpath, uid, gid, + atflags) || fflag) + continue; + } else { + if (!chflagsat(AT_FDCWD, p->fts_accpath, oct ? fset : + (p->fts_statp->st_flags | fset) & fclear, atflags)) + continue; + } + + /* error case */ + warn("%s", p->fts_path); + rval = 1; } if (errno) err(1, "fts_read"); -- 2.20.1