Questions about smbd option "strict rename"

[Brian Campbell]

On Fri, Nov 20, 2015 at 4:45 PM, Jeremy Allison <jra at samba.org> wrote:
> On Fri, Nov 20, 2015 at 10:01:46AM +0100, Ralph Boehme wrote:
>> Hi!
>>
>> I stumbled upon an inconsitency in "strict rename".
>>
>> The following set of patches added this option and a test:
>>
>> ---8<---
>> commit 035fd7200d8a025cdb8bfae30c264757aa3cb193
>> Author: Volker Lendecke <vl at samba.org>
>> Date:   Thu Sep 25 01:30:33 2014 +0200
>>
>>     s3:smbd: Don't rename a dir with files open underneath
>>
>>     This is an EXPENSIVE check. We'll have to guard this with an option
>>
>>     Signed-off-by: Volker Lendecke <vl at samba.org>
>>     Reviewed-by: Jeremy Allison <jra at samba.org>
>>     Reviewed-by: Stefan Metzmacher <metze at samba.org>
>>
>> commit 5f60dcc38ca275aedeb1d67611b5acf9b26361d5
>> Author: Jeremy Allison <jra at samba.org>
>> Date:   Fri Oct 24 13:57:04 2014 -0700
>>
>>     selftest:Samba3: use "strict rename = yes"
>>
>>     Signed-off-by: Jeremy Allison <jra at samba.org>
>>     Reviewed-by: Stefan Metzmacher <metze at samba.org>
>>
>> commit b0a434386dc2f77df89811bc3f56c4cc7fb7b16c
>> Author: Jeremy Allison <jra at samba.org>
>> Date:   Fri Oct 24 13:57:04 2014 -0700
>>
>>     s3:param: Add new option "strict rename".
>>
>>     Control whether smbd can rename directories containing
>>         open files. Defaults to "no" (meaning we *can* do
>>           such renames).
>>
>>     Signed-off-by: Jeremy Allison <jra at samba.org>
>>     Reviewed-by: Stefan Metzmacher <metze at samba.org>
>> ---8<---
>>
>> According to the commit message and the manpage description, "strict
>> rename = no" means we won't check for open files when we do a
>> directory rename.
>>
>> If this is correct, we have two bugs:
>
> Just to clarify, you mean: "we have two bugs in our rename path",
> yes ?
>
>> - "strict rename = no": doesn't work, opens are always checked,
>>   regardless of the setting of "strict rename".  can_rename(), the
>>   function where we do this check when renaming a directory, is
>>   missing a check for lp_strict_rename() or similar.
>
> Yep. That's how it was supposed to work. The code in
> can_rename() should probably be the same as the code in
> source/smbd/dir.c which is:
>
>         if (!lp_posix_pathnames() &&
>             lp_strict_rename(SNUM(conn)) &&
>             have_file_open_below(fsp->conn, fsp->fsp_name))
>         {
>                 return NT_STATUS_ACCESS_DENIED;
>         }
>
>
>> - "strict rename = yes": the function called to do the actual check
>>   for open files is file_find_subpath() which only checks for opens in
>>   that process and doesn't traverse locking.tdb
>
> Yep - that was older code and I remember deciding
> (with Volker I think) that per-process checks were
> probably 'good enough'.
>
>> In private conversation metze suggested that "strict rename" is
>> supposed to have completely different semantics:
>>
>> - "strict rename = no": check for opens, but only in the current
>>   process, avoids expensive locking.tdb traversal, deny rename if open
>>   found
>
> Hmmmm. We also need to have 'lp_posix_pathnames()' in the
> mix too I'm afraid.
>
>> - "strict rename = yes": check for opens by traversing locking.tdb,
>>   deny rename if open found
>
> That's the have_file_open_below() semantic I think...
>
>> Either way, I can provide fixes, but first we should agree upon the
>> semantics of "strict rename". :)
>>
>> I am currently pursuing this case, because not allowing directory
>> renames with open files is a major pita for OS X users. OS X expects
>> POSIX rename semantics and that's what OS X SMB server implements. I
>> know this contradicts MS-SMB2/FSA.
>
> How does their client ever work against Windows ? Windows
> does strict renames.

As Ralph says, it's a major PITA :) It basically means that depending
on exactly what you have focused or open in the Finder, you may or
many not be able to move or rename directories, and it prompts you for
an administrator password each time. It's not completely unusable, you
can work around it by being careful, but it is a PITA.

>
>> "strict rename = no" with the semantics described in the manpage,
>> would give OS X users an easy workaround. An alternative would be
>> something like my patch attached to the bugreport that tracks this
>> problem for OS X users:
>
> Hate that patch, sorry :-).
>
>> <https://bugzilla.samba.org/show_bug.cgi?id=11065>
>> <https://attachments.samba.org/attachment.cgi?id=10648>
>
> How about we standardize on the:
>
>         if (!lp_posix_pathnames() &&
>             lp_strict_rename(SNUM(conn)) &&
>             have_file_open_below(fsp->conn, fsp->fsp_name))
>         {
>                 return NT_STATUS_ACCESS_DENIED;
>         }
>
> behavior which does the traverse, and ignore
> the half-assed per-process check (which means
> we can remove the function file_find_subpath()) ?

Is the CIFS_UNIX_POSIX_PATHNAMES_CAP that enables lp_posix_pathnames
set by the OS X client? Are UNIX extensions supported in SMB2/3 yet?

Alternatively, just disabling this globally with the fixed "strict
rename = no" could work. Do you know if there's anything that this
would break for Windows clients, or is this check there just for
compatibility with the spec?

>
> Does that work ? We promise in the 'strict
> rename = no' docs we won't do open checks at all,
> so we should probably stick to that.
>
> Jeremy.
>