Questions about smbd option "strict rename"
Michael Adam
obnox at samba.org
Tue Nov 24 11:55:43 UTC 2015
LGTM.
Attached is a version of Jeremy's patch with a minor
change (use struct initializers).
It carries my RB+
But I will leave it up to Ralph to do the final review
and push.
Cheers - Michael
On 2015-11-23 at 14:12 -0800, Jeremy Allison wrote:
> On Mon, Nov 23, 2015 at 01:33:44PM -0800, Jeremy Allison wrote:
> > On Mon, Nov 23, 2015 at 10:22:35PM +0100, Ralph Boehme wrote:
> > > On Mon, Nov 23, 2015 at 12:59:35PM +0100, Stefan Metzmacher wrote:
> > > > Am 22.11.2015 um 13:49 schrieb Ralph Boehme:
> > > > > On Fri, Nov 20, 2015 at 01:45:08PM -0800, Jeremy Allison wrote:
> > > > >> On Fri, Nov 20, 2015 at 10:01:46AM +0100, Ralph Boehme wrote:
> > > > >>> - "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;
> > > > >> }
> > > > >
> > > > > Ok, thanks for clarifying. Does everybody agree? Metze?
> > > >
> > > > I don't agree, sorry.
> > > >
> > > > We should provide the semantics the client asked for
> > > > and try to behave like a windows server.
> > > >
> > > > I think we should fix the manpage and use file_find_subpath()
> > > > if lp_strict_rename() is false.
> > >
> > > I'm leaning towards that interpretation of "strict rename" too.
> >
> > I have a patch in preparation that implements this. It's
> > found an existing bug in files_below_forall() I'll probably
> > log as a separate bug....
>
> OK, here is the separate patch for the new bug:
>
> https://bugzilla.samba.org/show_bug.cgi?id=11615
>
> have_file_open_below() fails to enumerate files open under a directory correctly.
>
> Please review and push !
>
> Jeremy.
> From 8463ab8a2efd0df218d137ca96f9b7bd2d701e31 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Mon, 23 Nov 2015 14:00:56 -0800
> Subject: [PATCH] s3: smbd: have_file_open_below() fails to enumerate open
> files below an open directory handle.
>
> There are three issues:
>
> 1). The memcmp checking that the open file path has the open
> directory path as its parent compares using the wrong length
> (it uses the full open file path which will never compare as
> the same).
>
> 2). The files_below_forall() function doesn't fill in the
> callback function or callback data when calling share_mode_forall(),
> leading to a crash (which we never saw, as the previous issue (1)
> meant the callback function would never be invoked).
>
> 3). When invoking the callback function from files_below_forall_fn()
> we were passing in the wrong private_data pointer (needs to be
> the one from the state, not the private_data passed into
> files_below_forall_fn()).
>
> Found when running the torture test smb2.rename.rename_dir_openfile
> when fixing bug #11065.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11615
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/dir.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> index 3f99f88..cfc1635 100644
> --- a/source3/smbd/dir.c
> +++ b/source3/smbd/dir.c
> @@ -1912,14 +1912,14 @@ static int files_below_forall_fn(struct file_id fid,
> return 0;
> }
>
> - if (memcmp(state->dirpath, fullpath, len) != 0) {
> + if (memcmp(state->dirpath, fullpath, state->dirpath_len) != 0) {
> /*
> * Not a parent
> */
> return 0;
> }
>
> - return state->fn(fid, data, private_data);
> + return state->fn(fid, data, state->private_data);
> }
>
> static int files_below_forall(connection_struct *conn,
> @@ -1942,6 +1942,8 @@ static int files_below_forall(connection_struct *conn,
> return -1;
>
> }
> + state.fn = fn;
> + state.private_data = private_data;
>
> ret = share_mode_forall(files_below_forall_fn, &state);
> TALLOC_FREE(to_free);
> --
> 2.6.0.rc2.230.g3dd15c0
>
-------------- next part --------------
From 4ab77d1e5a38937e73d1a0ab37da2ab19d162fb3 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 23 Nov 2015 14:00:56 -0800
Subject: [PATCH] s3: smbd: have_file_open_below() fails to enumerate open
files below an open directory handle.
There are three issues:
1). The memcmp checking that the open file path has the open
directory path as its parent compares using the wrong length
(it uses the full open file path which will never compare as
the same).
2). The files_below_forall() function doesn't fill in the
callback function or callback data when calling share_mode_forall(),
leading to a crash (which we never saw, as the previous issue (1)
meant the callback function would never be invoked).
3). When invoking the callback function from files_below_forall_fn()
we were passing in the wrong private_data pointer (needs to be
the one from the state, not the private_data passed into
files_below_forall_fn()).
Found when running the torture test smb2.rename.rename_dir_openfile
when fixing bug #11065.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11615
Signed-off-by: Jeremy Allison <jra at samba.org>
Reviewed-by: Michael Adam <obnox at samba.org>
---
source3/smbd/dir.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
index 0972340..9a1378c 100644
--- a/source3/smbd/dir.c
+++ b/source3/smbd/dir.c
@@ -1912,14 +1912,14 @@ static int files_below_forall_fn(struct file_id fid,
return 0;
}
- if (memcmp(state->dirpath, fullpath, len) != 0) {
+ if (memcmp(state->dirpath, fullpath, state->dirpath_len) != 0) {
/*
* Not a parent
*/
return 0;
}
- return state->fn(fid, data, private_data);
+ return state->fn(fid, data, state->private_data);
}
static int files_below_forall(connection_struct *conn,
@@ -1929,7 +1929,10 @@ static int files_below_forall(connection_struct *conn,
void *private_data),
void *private_data)
{
- struct files_below_forall_state state = {};
+ struct files_below_forall_state state = {
+ .fn = fn,
+ .private_data = private_data,
+ };
int ret;
char tmpbuf[PATH_MAX];
char *to_free;
--
2.5.0
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151124/06087f8d/signature.sig>
More information about the samba-technical
mailing list