Re: [PATCH] chown: prevent multiple --from options
On Sun, 29 Sep 2019 at 14:36, Pádraig Brady wrote:
>
> I'm inclined to agree with this patch,
Unfortunately I messed it up as the second from still erases the
content of the first... Another solution that would make the code
simpler would be to just die at the beginning of the second from
even if it sets a different id than the first one. Anyway, here is a
more correct version than the initial patch.
Thanks for your time!
* src/chown.c: prevent multiple --from options
* tests/chown/basic.sh: prevent multiple --from options
---
src/chown.c | 16 ++--
tests/chown/basic.sh | 9 +
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/src/chown.c b/src/chown.c
index c1db307ab..8fe581192 100644
--- a/src/chown.c
+++ b/src/chown.c
@@ -227,11 +227,23 @@ main (int argc, char **argv)
case FROM_OPTION:
{
-const char *e = parse_user_spec (optarg,
- &required_uid, &required_gid,
+const char *e;
+uid_t parsed_required_uid = -1;
+gid_t parsed_required_gid = -1;
+e = parse_user_spec (optarg,
+ &parsed_required_uid,
+ &parsed_required_gid,
NULL, NULL);
if (e)
die (EXIT_FAILURE, 0, "%s: %s", e, quote (optarg));
+if ((parsed_required_uid != -1 && required_uid != -1) ||
+(parsed_required_gid != -1 && required_gid != -1))
+ die (EXIT_FAILURE, 0, "%s",
+ _("from option provided multiple times"));
+if (parsed_required_uid != -1)
+ required_uid = parsed_required_uid;
+if (parsed_required_gid != -1)
+ required_gid = parsed_required_gid;
break;
}
diff --git a/tests/chown/basic.sh b/tests/chown/basic.sh
index faeeb82aa..2ccfd959d 100755
--- a/tests/chown/basic.sh
+++ b/tests/chown/basic.sh
@@ -56,4 +56,13 @@ chown --no-dereference --from=0:1 2:010 slink || fail=1
# owner/group on the symlink should be changed
set _ $(ls -n slink); shift; test "$3:$4" = 2:10 || fail=1
+# Ensure two from are reported properly as error
+returns_ 1 chown --from=0:0 --from=1:1 0 f > out 2>&1|| fail=1
+printf "chown: from option provided multiple times\n" > exp
+compare exp out || fail=1
+
+# Unless they set different attributes
+chown --from=0 --from=:0 0 f || fail=1
+
+
Exit $fail
--
2.16.4
Re: [PATCH] chown: prevent multiple --from options
On 2019-09-29 02:46, Francois wrote: We can fix by rejecting the cases where --from option is provided multiple times and uid or gid are set twice. An more sophisticated fix is to allow the --from to be given multiple times, but have the resulting range be the intersection of all of the ranges given. Each successive --from applies clipping to the range calculated so far. If a uid or gid are given twice, but match, that should be fine too; why not.
Re: [PATCH] chown: prevent multiple --from options
On 29/09/19 10:46, Francois wrote:
> chown --from option can be used to restrict the list of files
> concerned by the chown.
>
> However this is true only for the latest --from option provided on the
> command line. One can run chown --from=1000:1000 --from=0:0, in that
> case the first --from will be silently discarded.
>
> It is an issue on systems where sysadmins naively provide a sudo rule
> for their users expecting to restrict its usage for a specific owner.
>
> We can fix by rejecting the cases where --from option is provided multiple
> times and uid or gid are set twice.
>
> * src/chown.c: prevent multiple --from options
>
> * tests/chown/basic.sh: prevent multiple --from options
> ---
> src/chown.c | 14 --
> tests/chown/basic.sh | 9 +
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/src/chown.c b/src/chown.c
> index c1db307ab..a06bf6ed4 100644
> --- a/src/chown.c
> +++ b/src/chown.c
> @@ -227,11 +227,21 @@ main (int argc, char **argv)
>
> case FROM_OPTION:
>{
> -const char *e = parse_user_spec (optarg,
> - &required_uid, &required_gid,
> +const char *e;
> +uid_t parsed_required_uid = -1;
> +gid_t parsed_required_gid = -1;
> +e = parse_user_spec (optarg,
> + &parsed_required_uid,
> + &parsed_required_gid,
> NULL, NULL);
> if (e)
>die (EXIT_FAILURE, 0, "%s: %s", e, quote (optarg));
> +if ((parsed_required_uid != -1 && required_uid != -1) ||
> +(parsed_required_gid != -1 && required_gid != -1))
> + die (EXIT_FAILURE, 0, "%s",
> + _("from option provided multiple times"));
> +required_uid = parsed_required_uid;
> +required_gid = parsed_required_gid;
> break;
>}
>
> diff --git a/tests/chown/basic.sh b/tests/chown/basic.sh
> index faeeb82aa..2ccfd959d 100755
> --- a/tests/chown/basic.sh
> +++ b/tests/chown/basic.sh
> @@ -56,4 +56,13 @@ chown --no-dereference --from=0:1 2:010 slink || fail=1
> # owner/group on the symlink should be changed
> set _ $(ls -n slink); shift; test "$3:$4" = 2:10 || fail=1
>
> +# Ensure two from are reported properly as error
> +returns_ 1 chown --from=0:0 --from=1:1 0 f > out 2>&1|| fail=1
> +printf "chown: from option provided multiple times\n" > exp
> +compare exp out || fail=1
> +
> +# Unless they set different attributes
> +chown --from=0 --from=:0 0 f || fail=1
> +
> +
> Exit $fail
> --
> 2.16.4
>
>
I'm inclined to agree with this patch,
as I can't see much use for overriding --from
thanks!
Pádraig
