GNU bug report logs - #8391
chmod setuid & setguid bits

Previous Next

Full log

Message #56 received at 8391 <at> debbugs.gnu.org (full text, mbox):

From: Ondrej Vasik <ovasik <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Eric Blake <eblake <at> redhat.com>
Cc: chris <at> computersalat.de, 8391 <at> debbugs.gnu.org
Subject: Re: bug#8391: chmod setuid & setguid bits
Date: Fri, 24 Feb 2012 19:27:32 +0100

On Fri, 2012-02-24 at 08:01 -0800, Paul Eggert wrote:
> On 02/24/2012 04:53 AM, Ondrej Vasik wrote:
> > +@command{chmod} 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.