Subject: Re: [netatalk-admins] netatalk's handling of file permissions is broken...

From: Jeff Wiegley <[email-masked]>
Date: Thu, 06 Nov 1997 18:21:10 -0800

approx. seven steps ahead of you...

Roland Schulz wrote:
> 
> Hi,
> 
> > A for instance:
> >
> > I want *all* my directories to have a chmod of 2770.
> > This way any files a user creates inherits the group
> > that the directory belongs to and thus any members of
> > that group can access the files in this directory.
> >
> > and files which are created should get a mode of at
> > least 0660.
> >
> 

I've already attacked the source code. and there are several problems
with this approach...

mostly the authors extremely abstracting use of:

int     (**afp_switch)() = preauth_switch;

int     (*postauth_switch[])() = {
    NULL, afp_bytelock, afp_closevol, afp_null,
    afp_closefork, afp_copyfile, afp_createdir, afp_createfile, /*   0
-   7 */
    afp_delete, afp_enumerate, afp_flush, afp_flushfork,
    afp_null, afp_null, afp_getforkparams, afp_null,            /*   8
-  15 */
    afp_getsrvrparms, afp_getvolparams, afp_login, afp_logincont,
    afp_logout, afp_mapid, afp_mapname, afp_moveandrename,      /*  16
-  23 */
    afp_openvol, afp_null, afp_openfork, afp_read,
    afp_rename, afp_setdirparams, afp_setfilparams, afp_setforkparams,
                                                                /*  24
-  31 */
    afp_null, afp_write, afp_getfildirparams, afp_setfildirparams,
    afp_changepw, afp_null, afp_getsrvrmesg, afp_null
/*afp_createid*/,         /*  32 -  39 */
    afp_null /*afp_deleteid*/, afp_null /*afp_resolveid*/, afp_null
/*afp_exchangefiles*/, afp_null,
    afp_null, afp_null, afp_null, afp_null,                     /*  40
-  47 */
    afp_opendt, afp_null, afp_null, afp_geticon,
    afp_geticoninfo, afp_addappl, afp_rmvappl, afp_getappl,     /*  48
-  55 */
    afp_addcomment, afp_rmvcomment, afp_getcomment, NULL,

in etc/afpd/switch.c makes it nearly impossible to figure out what is
being
called which makes tracing what netatalk is trying to do very hard.

but besides that.  Taking the high road I've spent all of today trying
exactly
what you mentioned below...

> Then how about patching the source in libatalk/adouble/ad_open.c,
> namely ad_mkdir?
> 
>  int
>  ad_mkdir( path, mode )
>      char                *path;
>      int                 mode;
>  {
> !    return mkdir( path, ad_mode( path, mode ) );
>  }
> 
> => return mkdir( path, ad_mode( path, mode ) | <yourniftybitsofchoice> );
> 
> That wasn't hard at all, agreed? Since afpd does only ad_mkdir(), not mkdir()
> itself you can put anything you like in there and it will be used for all
> calls in afpd. So now about files, this is a bit harder, since ad_open uses

oh yes, very easy ben kanobi...

have you tried this?

as it turns out ad_mkdir is called by afp_createdir() which, if you put
the proper...

   stat(...);
   syslog(...);
   exit(-1);

just before the final return in afp_create not only does stat report
that the
directory just created has st_mode of 042770 when the function (and
therefore
afpd) dies the directory left in your filesystem has the proper mode:
drwxrws---

But!!

something, somewhere (after the call to afp_createdir has run and
completed its
task) is modifying the permissions of the newly created directory.
effectively
wiping out the sgid bit.

I've put syslog calls at every point in the program that either calls
chmod or
chown (the only two system functions I can think of that has the
abilities to
change permissions) and neither of these is called in reference to the
newly
created directory.  sever chown/chmod calls are made to the AppleDouble
hierarchy but not the directory you just created.

and As I've mentioned the silly "switch.c" implementation makes it
impossible
to tell what major afp functions are being called next after the
directory
creation has completed.

So I can't find what is responsible for the bits changing after the
completion
of directory creation.

> more than one call to open which can create a file:
> 
>      if ( adflags & ADFLAGS_DF ) {
>          if (( ad->ad_df.adf_fd =
> !                open( path, oflags, ad_mode( path, mode ) )) < 0 ) {
>              return( -1 );
>          }
> 
> =>               open( path, oflags, ad_mode( path, mode ) | <yourniftybitsofchoice> )) < 0 ) {
> 
> and
> 
>     if ( adflags & ADFLAGS_HF ) {
>         ad_p = ad_path( path, adflags );
> !       admode = ad_mode( ad_p, mode );
> 
> =>      admode = ad_mode( ad_p, mode ) | <yourniftybitsofchoice>;

files are not a huge problem.  If the directories are modified such that
they
retain the gid bit that they inherit then files will inherit the proper
group
even in subfolders that have been created.

> 
> An alternative way would be to patch ad_mode() itself, but then you'd need
> to check from there if it's a file or a directory.
> 
> Sorry for the fake patches, this is directly from the source and therefore
> without any tests and guarantees :-)

Oh, I certainly don't mind the fake patches, they make perfect sense.
However
as I mentioned these aren't the places in the code entirely responsible
for
resetting a directories permissions.

> Use the source, Luke!

I am Ben but its a lot more complex than this and I'm a little
dissapointed
that the original authors designed netatalk to ignore the importance of
Unix
permission bits.  Mac's never had decent file permissions why would you
limit
netatalk to the Mac's crippled implementation?

SAMBA has it right.  you have a method of configuring samba to allows
clear
certain bits and always add certain bits to either files or directories.
Anything
not specified by these two masks gets inherited from either the client
or the
parent directory.

If I learn enough about the convolutions that netatalk is going through
when
dealing with permissions I'll write a patch that implements this same
behavior
in netatalk.

> Roland Schulz
> [email-masked]

- Jeff Wiegley

---

From: [email-masked]
Date: Thu, 06 Nov 1997 21:33:37 -0500

> From:    Jeff Wiegley <[email-masked]>
> To:      Roland Schulz <[email-masked]>

> task) is modifying the permissions of the newly created directory.

That's the mac.  Take a look at afp_setdirparams() -> setdirparams() ->
setdirmode().  The first two are in directory.c, the last in is
unix.c.  Which is appropriate, since the change you're trying to make
is really quite OS specific.  So, be sensative as you make changes...

:wes

---

From: [email-masked]
Date: Thu, 06 Nov 1997 21:36:27 -0500

> To:      Roland Schulz <[email-masked]>

[Oh, and....]

> to tell what major afp functions are being called next after the

Ah, sorry.  The Mac's doing the calling, remember.  You let me know a
better implementation, and I'll definitely consider your suggestions.

:wes

---

From: a sun <[email-masked]>
Date: Fri, 7 Nov 1997 09:21:33 -0800 (PST)

umm, the netatalk code isn't that hard to follow. i believe i told you
that the mode changes were coming from the chmod()'s in unix.c. the
easiest way of fixing the problem would involve stat()'ing directories
and then using the special bits of the umode during the chmod.

-a

---

From: Edan Idzerda <[email-masked]>
Date: Fri, 7 Nov 1997 14:04:03 -0500 (EST)

> and then using the special bits of the umode during the chmod.

I haven't tried to follow all of the code, but I agree that figure
out where it might be changing the permissions is not terribly tricky. 

Since the Mac has no way of changing the setuid, setgid, or sticky
bits of a directory, doesn't it make sense for afpd to never
change them?  And chmod (dir, mode) will do that, unfortunately.

I haven't patched netatalk yet, but I think that changing the
mode used at line 520 of unix.c will do the trick.
Just stat() the directory about to be changed, and use 
the mode bits that the Mac has no notion of.  I think the
below masking should do the trick:

    mode = (stbuf.st_mode & 007000) | mode;

My current opinion is that afpd *is* actually broken in
this sense.  I will patch my copy of 1.4b2 soon and if 
successful I will post my results to the list--but if 
someone else beats me I'll try not to clutter :)

- edan

--
Edan Idzerda	<[email-masked]>
System Administrator --  Michigan Technological University, Houghton MI USA

---

From: a sun <[email-masked]>
Date: Fri, 7 Nov 1997 11:24:16 -0800 (PST)

although there are actually a number of places where you need to do
that, i was planning on doing something similar.

-a