Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Jim Meyering wrote: Ondřej Vašík [EMAIL PROTECTED] wrote: as reported in https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/187315 by Aaron Toponce , chmod could display confusing messages when used for SGID/SUID/sticky bits without permissions to change them. e.g. with non-root sudoers user following scenario mkdir tmp;sudo chown .root tmp;ls -ld tmp;chmod -v 2755 tmp;ls -ld tmp; would lead to: drwxrwxr-x 2 Reset root 4096 24. říj 17.33 tmp mode of `tmp' changed to 2755 (rwxr-sr-x) drwxr-xr-x 2 Reset root 4096 24. říj 17.33 tmp So user is informed that sticky bit was set even if it was not. So, rather than trying to fix --changes, I'm leaning towards starting the process to remove it altogether. From chmod, chown, and chgrp. This may seem extreme, until you realize that the option is inherently inaccurate in some cases. And the only way it can be accurate is if PROG --changes ... were to stat each file after operating on it. Of course, I wouldn't do that for real. We'd have to first deprecate the targeted options, making any use provoke a warning, and then -- years later -- un-document and finally, remove them. And I probably wouldn't even deprecate --verbose. What do you think? Throwing out --changes should be ok, if user wants to be informed, he can parse verbose output easily and common case is to use it without verbose mode at all. Anyway --verbose output is affected by this issue as well, so I would prefer to 1) depricate --changes option as it could be substituted by --verbose and grep 2) fix the verbose output to show correct output in every case where it is possible. E.g. if the stat fails, it should inform the user that the mode/owner/whatever is unknown. E.g. SGID bit case or stat failed is the thing where user should be informed in verbose mode about something unusual. Greetings, Ondrej signature.asc Description: Toto je digitálně podepsaná část zprávy ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Ondřej Vašík [EMAIL PROTECTED] wrote: Jim Meyering wrote: Ondřej Vašík [EMAIL PROTECTED] wrote: as reported in https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/187315 by Aaron Toponce , chmod could display confusing messages when used for SGID/SUID/sticky bits without permissions to change them. e.g. with non-root sudoers user following scenario mkdir tmp;sudo chown .root tmp;ls -ld tmp;chmod -v 2755 tmp;ls -ld tmp; would lead to: drwxrwxr-x 2 Reset root 4096 24. říj 17.33 tmp mode of `tmp' changed to 2755 (rwxr-sr-x) drwxr-xr-x 2 Reset root 4096 24. říj 17.33 tmp So user is informed that sticky bit was set even if it was not. So, rather than trying to fix --changes, I'm leaning towards starting the process to remove it altogether. From chmod, chown, and chgrp. This may seem extreme, until you realize that the option is inherently inaccurate in some cases. And the only way it can be accurate is if PROG --changes ... were to stat each file after operating on it. Of course, I wouldn't do that for real. We'd have to first deprecate the targeted options, making any use provoke a warning, and then -- years later -- un-document and finally, remove them. And I probably wouldn't even deprecate --verbose. What do you think? Throwing out --changes should be ok, if user wants to be informed, he can parse verbose output easily and common case is to use it without verbose mode at all. Anyway --verbose output is affected by this issue as well, so I would prefer to 1) depricate --changes option as it could be substituted by --verbose and grep 2) fix the verbose output to show correct output in every case where it is possible. E.g. if the stat fails, it should inform the user that the mode/owner/whatever is unknown. E.g. SGID bit case or stat failed is the thing where user should be informed in verbose mode about something unusual. Actually, I was thinking of making --verbose output *less* informative, so that it can be truthful, albeit less informative, without adding extra stat/lstat calls. Sort of like chcon, which says simply changing security context of FILE While I do see the down-side of doing that, it seems better to print less information, with a guarantee of no lies, than to print usually-true messages along with the occasional untruth. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Jim Meyering wrote: Ondřej Vašík [EMAIL PROTECTED] wrote: Throwing out --changes should be ok, if user wants to be informed, he can parse verbose output easily and common case is to use it without verbose mode at all. Anyway --verbose output is affected by this issue as well, so I would prefer to 1) depricate --changes option as it could be substituted by --verbose and grep 2) fix the verbose output to show correct output in every case where it is possible. E.g. if the stat fails, it should inform the user that the mode/owner/whatever is unknown. E.g. SGID bit case or stat failed is the thing where user should be informed in verbose mode about something unusual. Actually, I was thinking of making --verbose output *less* informative, so that it can be truthful, albeit less informative, without adding extra stat/lstat calls. Sort of like chcon, which says simply changing security context of FILE While I do see the down-side of doing that, it seems better to print less information, with a guarantee of no lies, than to print usually-true messages along with the occasional untruth. For me verbose mode is something useful to track problems. Therefore I do prefer more informations there. Common usage of chown/chmod/chgrp/chcon utilities is without --verbose/--changes mode, so it would affect very low (close to zero) amount of users anyway. I guess burden of of additional stat/lstat calls could be acceptable for some kind of very verbose mode as it doesn't affect common users at all and makes tracking of problems easier. Even for the price of potentially untruth informations in very rare cases - if that risk is mentioned in documentation. Greetings, Ondřej signature.asc Description: Toto je digitálně podepsaná část zprávy ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Ondřej Vašík on 11/20/2008 5:51 AM: For me verbose mode is something useful to track problems. Therefore I do prefer more informations there. Common usage of chown/chmod/chgrp/chcon utilities is without --verbose/--changes mode, so it would affect very low (close to zero) amount of users anyway. Would a double --verbose make sense? 'chown --verbose' merely prints a generic message that it changed a mode, while 'chown --verbose --verbose' re-stats the file and prints the updated mode? - -- Don't work too hard, make some time for fun as well! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkklYpYACgkQ84KuGfSFAYBUJQCgxmL56WzzZqZ7XSff0ppzqWmJ uiEAn0xawzIQ2rzDFGxy2zppXEwkfHp+ =FO7X -END PGP SIGNATURE- ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Eric Blake [EMAIL PROTECTED] wrote: According to Ondřej Vašík on 11/20/2008 5:51 AM: For me verbose mode is something useful to track problems. Therefore I do prefer more informations there. Common usage of chown/chmod/chgrp/chcon utilities is without --verbose/--changes mode, so it would affect very low (close to zero) amount of users anyway. Would a double --verbose make sense? 'chown --verbose' merely prints a generic message that it changed a mode, while 'chown --verbose --verbose' re-stats the file and prints the updated mode? I like it. Sounds like a fine compromise. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Eric Blake [EMAIL PROTECTED] writes: Would a double --verbose make sense? --verbose=2 Andreas. -- Andreas Schwab, SuSE Labs, [EMAIL PROTECTED] SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Andreas Schwab wrote: Eric Blake [EMAIL PROTECTED] writes: Would a double --verbose make sense? --verbose=2 Rather than some level-numbers I do prefer --verbose and --verbose=high ... Ondřej signature.asc Description: Toto je digitálně podepsaná část zprávy ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Ondřej Vašík [EMAIL PROTECTED] wrote: as reported in https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/187315 by Aaron Toponce , chmod could display confusing messages when used for SGID/SUID/sticky bits without permissions to change them. e.g. with non-root sudoers user following scenario mkdir tmp;sudo chown .root tmp;ls -ld tmp;chmod -v 2755 tmp;ls -ld tmp; would lead to: drwxrwxr-x 2 Reset root 4096 24. říj 17.33 tmp mode of `tmp' changed to 2755 (rwxr-sr-x) drwxr-xr-x 2 Reset root 4096 24. říj 17.33 tmp So user is informed that sticky bit was set even if it was not. This is a good excuse to start the process. Note this TODO item: remove or adjust chown's --changes option, since it can't always do what it currently says it does. Almost from the beginning, I've wanted to remove support for --changes (-c) from chmod, chown and chgrp. Note that the original chcon implementation had that option, but I removed it as part of the big SELinux-support-adding changes. So, rather than trying to fix --changes, I'm leaning towards starting the process to remove it altogether. From chmod, chown, and chgrp. This may seem extreme, until you realize that the option is inherently inaccurate in some cases. And the only way it can be accurate is if PROG --changes ... were to stat each file after operating on it. I.e., the current implementations, even with your patch, can still produce misleading output. FYI, it's telling to see that when I ripped out all support for chmod's --verbose and --changes options (just to see), all tests still passed. Of course, I wouldn't do that for real. We'd have to first deprecate the targeted options, making any use provoke a warning, and then -- years later -- un-document and finally, remove them. And I probably wouldn't even deprecate --verbose. What do you think? ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Hello, thanks for review and objections. Paul Eggert wrote: Ondřej Vašík [EMAIL PROTECTED] writes: - bool changed = (chmod_succeeded - mode_changed (file, old_mode, new_mode)); + bool mode_change = mode_changed (file, old_mode, new_mode); + bool changed = (chmod_succeeded mode change); Sorry, that change was obviously redundant... + + if (chmod_succeeded ((old_mode ^ new_mode) CHMOD_MODE_BITS)) +{ + + /* Changed to another mode than requested */ This doesn't look right to me. First, surely there's no need to invoke mode_changed if chmod_succeeded is false. Second, ((old_mode ^ new_mode) CHMOD_MODE_BITS) doesn't tell us whether we changed to another mode than requested; mode_change tells us that. About first you are right, that change was accidently added for other approach and I forgot to remove it. About second - that's not everytime true - as shown in the example - SUID, SGID and sticky bits could be ignored in mode changed. That's documented in info changes. Anyway user is then mistakenly informed that mode was changed e.g. to 2755, but the real change was to 0755 - and that's imho wrong. + struct stat new_stats; + char perms_requested[12]; + char perms_actual[12]; + + if (stat (file, new_stats) != 0) Third, this means we've invoked 'stat' twice on the file afterwards, once here, and once in mode_changed. We should invoke 'stat' only in mode_changed. You are right, that stat() call easy to reduce. So better solution for that issue would be to check everything in mode_changed() function. Proposed amended patch is attached, should solve all your objections about the previous patch. Greetings, Ondřej Vašík From ea697d3aa266f2d281b098b8c0b98157ed10e9b8 Mon Sep 17 00:00:00 2001 From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= [EMAIL PROTECTED] Date: Fri, 24 Oct 2008 17:24:09 +0200 Subject: [PATCH] chmod: inform in verbose if used mode for chmod was different than requested * chmod (mode changed): Display a message in verbose when SUID, SGID or sticky bit change was requested but not performed. Suggested by Aaron Toponce * NEWS: Mention that change --- NEWS|3 +++ src/chmod.c | 28 ++-- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/NEWS b/NEWS index 3fc0349..86f415b 100644 --- a/NEWS +++ b/NEWS @@ -23,6 +23,9 @@ GNU coreutils NEWS-*- outline -*- Rm was improved directly, while the others inherit the improvement from the newer version of fts in gnulib. + chmod now displays a message when SUID, SGID or sticky bit change was + requested, but not performed. + comm now verifies that the inputs are in sorted order. This check can be turned off with the --nocheck-order option. diff --git a/src/chmod.c b/src/chmod.c index 80fc363..3cdbf61 100644 --- a/src/chmod.c +++ b/src/chmod.c @@ -110,9 +110,9 @@ static struct option const long_options[] = The old mode was OLD_MODE, but it was changed to NEW_MODE. */ static bool -mode_changed (char const *file, mode_t old_mode, mode_t new_mode) +mode_changed (char const *file, mode_t old_mode, mode_t *new_mode) { - if (new_mode (S_ISUID | S_ISGID | S_ISVTX)) + if (*new_mode (S_ISUID | S_ISGID | S_ISVTX)) { /* The new mode contains unusual bits that the call to chmod may have silently cleared. Check whether they actually changed. */ @@ -125,11 +125,27 @@ mode_changed (char const *file, mode_t old_mode, mode_t new_mode) error (0, errno, _(getting new attributes of %s), quote (file)); return false; } - - new_mode = new_stats.st_mode; + if (((new_stats.st_mode ^ *new_mode) CHMOD_MODE_BITS) + (verbosity == V_high) ((old_mode ^ *new_mode) CHMOD_MODE_BITS)) +{ + /* Do we ignore SUID/SGID/sticky bit? Inform user in verbose! */ + strmode (*new_mode, perms_requested); + perms_requested[10] = '\0'; + strmode (new_stats.st_mode, perms_actual); + perms_actual[10] = '\0'; + printf( +_(can't change mode of %s to %04lo (%s), using %04lo (%s)\n), +quote (file), +(unsigned long int) (*new_mode CHMOD_MODE_BITS), +perms_requested[1], +(unsigned long int) (new_stats.st_mode CHMOD_MODE_BITS), +perms_actual[1]); +} + + *new_mode = new_stats.st_mode; } - return ((old_mode ^ new_mode) CHMOD_MODE_BITS) != 0; + return ((old_mode ^ *new_mode) CHMOD_MODE_BITS) != 0; } /* Tell the user how/if the MODE of FILE has been changed. @@ -260,7 +276,7 @@ process_file (FTS *fts, FTSENT *ent) if (verbosity != V_off) { bool changed = (chmod_succeeded - mode_changed (file, old_mode, new_mode)); + mode_changed (file, old_mode, new_mode)); if (changed || verbosity == V_high) { --
Re: [PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Ondřej Vašík [EMAIL PROTECTED] writes: - bool changed = (chmod_succeeded -mode_changed (file, old_mode, new_mode)); + bool mode_change = mode_changed (file, old_mode, new_mode); + bool changed = (chmod_succeeded mode change); + + if (chmod_succeeded ((old_mode ^ new_mode) CHMOD_MODE_BITS)) +{ + + /* Changed to another mode than requested */ This doesn't look right to me. First, surely there's no need to invoke mode_changed if chmod_succeeded is false. Second, ((old_mode ^ new_mode) CHMOD_MODE_BITS) doesn't tell us whether we changed to another mode than requested; mode_change tells us that. + struct stat new_stats; + char perms_requested[12]; + char perms_actual[12]; + + if (stat (file, new_stats) != 0) Third, this means we've invoked 'stat' twice on the file afterwards, once here, and once in mode_changed. We should invoke 'stat' only in mode_changed. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils