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)
{
--
[PATCH]: chmod - do inform about using different mode than requested with SGID/SUID/sticky bits without permissions to change them
Hello,
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.
After my patch output will be:
drwxrwxr-x 2 Reset root 4096 24. říj 17.35 tmp
can't change mode of `tmp' to 2755 (rwxr-sr-x), using 0755 (rwxr-xr-x)
mode of `tmp' changed to 0755 (rwxr-xr-x)
drwxr-xr-x 2 Reset root 4096 24. říj 17.35 tmp
That should reduce user's confusion and clarify what's really done by
chmod command.
Greetings,
Ondřej Vašík
From 4eaf35f250ab6eec036b7ab21a482a76289f8303 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 (process_file): Display a message 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 | 37 +++--
2 files changed, 38 insertions(+), 2 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..87f8199 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -259,8 +259,41 @@ process_file (FTS *fts, FTSENT *ent)
if (verbosity != V_off)
{
- 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 */
+ struct stat new_stats;
+ char perms_requested[12];
+ char perms_actual[12];
+
+ if (stat (file, new_stats) != 0)
+{
+ if (!force_silent)
+error (0, errno, _(getting new attributes of %s),
+ quote (file));
+ ok = false;
+}
+
+ 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]);
+
+/* Change mode to actual mode after change for verbose output */
+new_mode = new_stats.st_mode;
+ }
+
if (changed || verbosity == V_high)
{
--
1.5.6.1.156.ge903b
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] 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
