bug#8391: chmod setuid & setguid bits

[Ondrej Vasik]

On Fri, 2012-02-24 at 08:01 -0800, Paul Eggert wrote:
> On 02/24/2012 04:53 AM, Ondrej Vasik wrote:
> > address@hidden by default keeps the set-user-ID and set-group-ID bits
> > +of @var{mode} of a directory when the mode is specified as an octal digit,
> > +unless the mode length is 5 digits with leading double zero.
> 
> Wait a minute: 00755 works, but 000775 doesn't?  Isn't that odd?
> Also, what about modes like 0000?  They have two leading zeros --
> shouldn't they clear the setuid bits too?

0000 was discussed before - not explicit enough.
6+ digits ... first, I had the patch done same way as Eric simplified
way - so clear bits for every 5+ octal digits mode, but after in-house
discussion with few colleagues I changed that to 5 digits only. But it
seems that Eric is ok with 5+ digits check (and no check for 0 at
begining, as this is checked in mode validation), so will change it that
way.

> The more I think about it, the more-confusing the double-leading-zero
> notation see,s.  How about using a more-obvious notation instead?
> Say, a leading "="?  For example, "=755" would mean "exactly 755"
> and would clear the setuid bit.  mode_compile could implement this.

Leading = probably makes sense, however the request in
https://bugzilla.redhat.com/show_bug.cgi?id=691466 was talking about
support for octal digits only (and leading = seems to me like a hybrid
mode - which would make the required changes in legacy scripts for
compatibility with old chmod even harder) - and would definitely mean
some changes to gnulib's mode_compile().

> Regardless, documentation about this notation should be be in the
> section "Directories and the Set-User-ID and Set-Group-ID Bits";
> that's where it belongs.

I missed that section somehow - probably because it was in separate
perm.texi file. I'll use the Eric's wording and move it to that section
in next version.

> +        mode_adjust (old_mode, (S_ISDIR (old_mode) != 0) && keepdirbits,
> +                     0, change, NULL);
> 
> This change depends on internal details of mode_adjust, and doesn't
> feel right.  The second argument of mode_adjust means that the argument
> is a directory, and is also used to interpret modes like +X.
> The code above will work, but it's not clean.  It'd be better
> to make the second argument of mode_adjust an int 'flags' argument,
> with two flags, one flag saying that it's a directory and one flag saying
> whether it should ignore requests to clear UID and GID bits.

You are right, it is really not 100% clean, I used that primarily to
avoid need for gnulib modification. Making second param of mode_adjust()
int flag would be cleaner approach... but this would mean changing all
the calls of mode_adjust to reflect that.

Probably adding some optional flags int param to the mode_adjust() would
be better in this case ... or using mode_change flag (and special mode)
for that.
Which approach do you prefer?

> Or better yet, leave the call to mode_adjust alone, and have mode_compile
> figure this stuff out.

But mode_compile() still computes the correct mode from the octal
digits, mode_adjust() cleans the setgid/setuid bits from it (based on
the dir boolean).

Will wait with amending the patch until this will be clarified.

Greetings,
         Ondrej Vasik