[SCM] Samba Shared Repository - branch master updated - release-4-0-0alpha7-1924-gff736df

[Jeremy Allison]

On Tue, Jun 02, 2009 at 01:45:42PM +0200, Volker Lendecke wrote:
> On Sat, May 30, 2009 at 03:28:21PM -0500, Jeremy Allison wrote:
> 
> >     Fix bug #6421 - POSIX read-only open fails on
> >     read-only shares.  The change to smbd/trans2.c opens
> >     up SETFILEINFO calls to POSIX_OPEN only. The change to
> >     first smbd/open.c closes 2 holes that would have been
> >     exposed by allowing POSIX_OPENS on readonly shares,
> >     and their ability to set arbitrary flags permutations.
> >     The O_CREAT -> O_CREAT|O_EXCL change removes an
> >     illegal combination (O_EXCL without O_CREAT) that
> >     previously was being passed down to the open syscall.
> >     Jeremy.
> 
> Just from my comment in bugzilla:
> 
> To be honest, I'm not really happy with that patch. Using a
> setfileinfo call for a read-only open is just a broken
> protocol, and having to put some complicated code paths for
> security-relevant stuff in makes me feel very, very uneasy.
> 
> There is no other way like a qfileinfo for the r/o variant
> of posix open?

>From my comments in bugzilla. :-).

-----------------------------------------------------------------------

We can't change the protocol at this point. It is what it is, and setfileinfo
is what is used for the open.

The patch opens one info level on a read-only share, and the inability to
create/write depends on the same protections in the open path that NTCreateX
uses to protect read-only shares from modification. The only reason that the
changes are needed to the open code path, is to protect against the flags that
can be used via raw posix open that aren't allowed by NTCreateX.

Its a very minimal patch to fix something that will cause a lot of complaints
once the new Linux kernel code is released generally.

Just read it again, it really isn't that bad :-).

Jeremy.

------- Comment #9 From Jeremy Allison 2009-06-02 12:32:52 CST [reply] -------

Alright, I'll go through the changes line by line, to make sure you're happy
with them.

Inside trans2.c the only real change is here:

+               if (info_level != SMB_POSIX_PATH_OPEN) {

which only returns NT_STATUS_ACCESS_DENIED automatically on a setfilepathinfo
call if it's not a SMB_POSIX_PATH_OPEN info level. i.e. it opens
setfilepathinfo to this info level on a read-only share.

Note that no other changes are needed, as it's simply going into a standard
code path. As an example, look inside the reply_open() function in
smbd/reply.c. Note there are *no* checks here for a read-only share, the checks
are all done inside the open code path.

In open.c this is the main change:

        if (!CAN_WRITE(conn)) {
                /* It's a read-only share - fail if we wanted to write. */
-               if(accmode != O_RDONLY) {
+               if(accmode != O_RDONLY || (flags & O_TRUNC) || (flags & O_APPEND)) {
                        DEBUG(3,("Permission denied opening %s\n", path));
                        return NT_STATUS_ACCESS_DENIED;

This closes the "modify filesystem" path to open requests with flags of O_TRUNC
and O_APPEND, which are the only two flags which can't be explicitly passed
down to the low level POSIX open via the NTCreateX codepath. POSIX open calls
could pass those flags down, so this is why the extra check is needed here.

All other flags that could modify the underlying filesystem are already taken
care of as they could have been passed down via the existing NTCreateX
codepaths (O_CREAT) for example.

The change :

-                       flags &= ~O_CREAT;
-                       local_flags &= ~O_CREAT;
+                       flags &= ~(O_CREAT|O_EXCL);
+                       local_flags &= ~(O_CREAT|O_EXCL);

simply prevents an illegal combination of flags being passed down (O_EXCL only
has meaning if O_CREAT is set, and in the read-only share case we're removing
the O_CREAT flag, so we should remove the O_EXCL flag also).

Hope this helps explain the change and why I think it's a safe one for 3.3/3.2.

Jeremy.