[PATCH] Fix bug #11249 - Mangled names do not work with acl_xattr
Jeremy Allison
jra at samba.org
Mon May 4 13:39:30 MDT 2015
This is an interesting one, was an easy initial fix
and then took me a while to get the regression test
working under 'make test', which showed me a second fix
was needed :-).
Submitter (large OEM) confirms the fix works
on their production systems.
Reproducer is to create a directory called
foo:bar and then try to copy it to the desktop
of a Windows machine with acl_xattr loaded.
The underlying problem (which this patch
*doesn't* solve) is that some of the VFS
operations take a 'const char *' that
has been created post-stream-name processing,
and some of the underlying modules (that
can only be called post-stream-name processing)
call stream-parsing functions on the pathname.
These are internal EA and ACL processing functions
which can't use streams anyway.
Once this fix has gone into master I'll back-port
for 4.2.x and 4.1.x and remove the hunk of the
patch that removes a couple of utility functions
--------------------------------------------
s3: smbd: VFS: Remove vfs_stat_smb_fname() and vfs_lstat_smb_fname().
No longer used or needed.
--------------------------------------------
that aren't used by our current in-tree VFS
modules, but might be being used in out-of-tree
VFS modules from vendors, so we don't make any
ABI changes in shipping code.
Once this is in master, I'll work with Richard
(who has raised this issue before) to move the
'const char *' pathnames being used in the VFS
to use 'const struct smb_filename *' instead,
which allows the underlying code to *know* if
it's being passed a streamname and to just
pick out and use the basename accordingly.
Please review and/or push !
Jeremy.
-------------- next part --------------
From 0d6c8bd86d182397912050251ef270a749e2fd1d Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 1 May 2015 12:50:51 -0700
Subject: [PATCH 1/5] s3: smbd: VFS: Add vfs_stat_smb_basename() - to be called
when we *know* stream name parsing has already been done.
https://bugzilla.samba.org/show_bug.cgi?id=11249
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/proto.h | 2 ++
source3/smbd/vfs.c | 26 ++++++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index a5144d5..5a7d16d 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -1179,6 +1179,8 @@ int vfs_stat_smb_fname(struct connection_struct *conn, const char *fname,
SMB_STRUCT_STAT *psbuf);
int vfs_lstat_smb_fname(struct connection_struct *conn, const char *fname,
SMB_STRUCT_STAT *psbuf);
+int vfs_stat_smb_basename(struct connection_struct *conn, const char *fname,
+ SMB_STRUCT_STAT *psbuf);
NTSTATUS vfs_stat_fsp(files_struct *fsp);
NTSTATUS vfs_chown_fsp(files_struct *fsp, uid_t uid, gid_t gid);
NTSTATUS vfs_streaminfo(connection_struct *conn,
diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
index ebd3440..997fe17 100644
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -1331,6 +1331,32 @@ int vfs_lstat_smb_fname(struct connection_struct *conn, const char *fname,
}
/**
+ * XXX: This is temporary and there should be no callers of this once
+ * smb_filename is plumbed through all path based operations.
+ *
+ * Called when we know stream name parsing has already been done.
+ */
+int vfs_stat_smb_basename(struct connection_struct *conn, const char *fname,
+ SMB_STRUCT_STAT *psbuf)
+{
+ struct smb_filename smb_fname = {
+ .base_name = fname
+ };
+ int ret;
+
+ if (lp_posix_pathnames()) {
+ ret = SMB_VFS_LSTAT(conn, &smb_fname);
+ } else {
+ ret = SMB_VFS_STAT(conn, &smb_fname);
+ }
+
+ if (ret != -1) {
+ *psbuf = smb_fname.st;
+ }
+ return ret;
+}
+
+/**
* Ensure LSTAT is called for POSIX paths.
*/
--
2.2.0.rc0.207.ga3a616c
From b57d659eb7f8e861249f782e7244dc98bfa0803a Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 1 May 2015 13:09:36 -0700
Subject: [PATCH 2/5] s3: smbd: VFS: All the places that are currently calling
vfs_stat_smb_fname() and vfs_lstat_smb_fname() should be calling
vfs_stat_smb_basename().
They are all post-stream name processing.
https://bugzilla.samba.org/show_bug.cgi?id=11249
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/modules/nfs4_acls.c | 4 ++--
source3/modules/vfs_acl_common.c | 19 ++++++++++++++++++-
source3/modules/vfs_acl_tdb.c | 16 +++-------------
source3/modules/vfs_recycle.c | 2 +-
source3/modules/vfs_solarisacl.c | 2 +-
source3/modules/vfs_xattr_tdb.c | 2 +-
6 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/source3/modules/nfs4_acls.c b/source3/modules/nfs4_acls.c
index adc9b37..f7daa8d 100644
--- a/source3/modules/nfs4_acls.c
+++ b/source3/modules/nfs4_acls.c
@@ -316,9 +316,9 @@ static int smbacl4_GetFileOwner(struct connection_struct *conn,
memset(psbuf, 0, sizeof(SMB_STRUCT_STAT));
/* Get the stat struct for the owner info. */
- if (vfs_stat_smb_fname(conn, filename, psbuf) != 0)
+ if (vfs_stat_smb_basename(conn, filename, psbuf) != 0)
{
- DEBUG(8, ("vfs_stat_smb_fname failed with error %s\n",
+ DEBUG(8, ("vfs_stat_smb_basename failed with error %s\n",
strerror(errno)));
return -1;
}
diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index 920c811..625e7cb 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -620,7 +620,24 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
}
psbuf = &fsp->fsp_name->st;
} else {
- int ret = vfs_stat_smb_fname(handle->conn,
+ /*
+ * https://bugzilla.samba.org/show_bug.cgi?id=11249
+ *
+ * We are currently guaranteed that 'name' here is
+ * a smb_fname->base_name, which *cannot* contain
+ * a stream name (':'). vfs_stat_smb_fname() splits
+ * a name into a base name + stream name, which
+ * when we get here we know we've already done.
+ * So we have to call the stat or lstat VFS
+ * calls directly here. Else, a base_name that
+ * contains a ':' (from a demangled name) will
+ * get split again.
+ *
+ * FIXME.
+ * This uglyness will go away once smb_fname
+ * is fully plumbed through the VFS.
+ */
+ int ret = vfs_stat_smb_basename(handle->conn,
name,
&sbuf);
if (ret == -1) {
diff --git a/source3/modules/vfs_acl_tdb.c b/source3/modules/vfs_acl_tdb.c
index 8ee4bd5..e02993b 100644
--- a/source3/modules/vfs_acl_tdb.c
+++ b/source3/modules/vfs_acl_tdb.c
@@ -159,7 +159,7 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx,
status = vfs_stat_fsp(fsp);
sbuf = fsp->fsp_name->st;
} else {
- int ret = vfs_stat_smb_fname(handle->conn, name, &sbuf);
+ int ret = vfs_stat_smb_basename(handle->conn, name, &sbuf);
if (ret == -1) {
status = map_nt_error_from_unix(errno);
}
@@ -282,12 +282,7 @@ static int rmdir_acl_tdb(vfs_handle_struct *handle, const char *path)
struct db_context *db = acl_db;
int ret = -1;
- if (lp_posix_pathnames()) {
- ret = vfs_lstat_smb_fname(handle->conn, path, &sbuf);
- } else {
- ret = vfs_stat_smb_fname(handle->conn, path, &sbuf);
- }
-
+ ret = vfs_stat_smb_basename(handle->conn, path, &sbuf);
if (ret == -1) {
return -1;
}
@@ -347,12 +342,7 @@ static int sys_acl_set_file_tdb(vfs_handle_struct *handle,
struct db_context *db = acl_db;
int ret = -1;
- if (lp_posix_pathnames()) {
- ret = vfs_lstat_smb_fname(handle->conn, path, &sbuf);
- } else {
- ret = vfs_stat_smb_fname(handle->conn, path, &sbuf);
- }
-
+ ret = vfs_stat_smb_basename(handle->conn, path, &sbuf);
if (ret == -1) {
return -1;
}
diff --git a/source3/modules/vfs_recycle.c b/source3/modules/vfs_recycle.c
index 00d7f34..9af78fd 100644
--- a/source3/modules/vfs_recycle.c
+++ b/source3/modules/vfs_recycle.c
@@ -188,7 +188,7 @@ static bool recycle_directory_exist(vfs_handle_struct *handle, const char *dname
{
SMB_STRUCT_STAT st;
- if (vfs_stat_smb_fname(handle->conn, dname, &st) == 0) {
+ if (vfs_stat_smb_basename(handle->conn, dname, &st) == 0) {
if (S_ISDIR(st.st_ex_mode)) {
return True;
}
diff --git a/source3/modules/vfs_solarisacl.c b/source3/modules/vfs_solarisacl.c
index 9b3c4f6..efd2d75 100644
--- a/source3/modules/vfs_solarisacl.c
+++ b/source3/modules/vfs_solarisacl.c
@@ -167,7 +167,7 @@ int solarisacl_sys_acl_set_file(vfs_handle_struct *handle,
* that has not been specified in "type" from the file first
* and concatenate it with the acl provided.
*/
- if (vfs_stat_smb_fname(handle->conn, name, &s) != 0) {
+ if (vfs_stat_smb_basename(handle->conn, name, &s) != 0) {
DEBUG(10, ("Error in stat call: %s\n", strerror(errno)));
goto done;
}
diff --git a/source3/modules/vfs_xattr_tdb.c b/source3/modules/vfs_xattr_tdb.c
index 63a12fd..66c19f8 100644
--- a/source3/modules/vfs_xattr_tdb.c
+++ b/source3/modules/vfs_xattr_tdb.c
@@ -414,7 +414,7 @@ static int xattr_tdb_rmdir(vfs_handle_struct *handle, const char *path)
TALLOC_FREE(frame); return -1;
});
- if (vfs_stat_smb_fname(handle->conn, path, &sbuf) == -1) {
+ if (vfs_stat_smb_basename(handle->conn, path, &sbuf) == -1) {
TALLOC_FREE(frame);
return -1;
}
--
2.2.0.rc0.207.ga3a616c
From 303704e993b2efb8bb4776f17e29407e6963333f Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 1 May 2015 13:13:00 -0700
Subject: [PATCH 3/5] s3: smbd: VFS: Remove vfs_stat_smb_fname() and
vfs_lstat_smb_fname().
No longer used or needed.
https://bugzilla.samba.org/show_bug.cgi?id=11249
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/proto.h | 4 ----
source3/smbd/vfs.c | 55 ----------------------------------------------------
2 files changed, 59 deletions(-)
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 5a7d16d..09a7371 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -1175,10 +1175,6 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname);
NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
const char *fname,
struct smb_request *smbreq);
-int vfs_stat_smb_fname(struct connection_struct *conn, const char *fname,
- SMB_STRUCT_STAT *psbuf);
-int vfs_lstat_smb_fname(struct connection_struct *conn, const char *fname,
- SMB_STRUCT_STAT *psbuf);
int vfs_stat_smb_basename(struct connection_struct *conn, const char *fname,
SMB_STRUCT_STAT *psbuf);
NTSTATUS vfs_stat_fsp(files_struct *fsp);
diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
index 997fe17..b83dfcf 100644
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -1278,61 +1278,6 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname)
/**
* XXX: This is temporary and there should be no callers of this once
* smb_filename is plumbed through all path based operations.
- */
-int vfs_stat_smb_fname(struct connection_struct *conn, const char *fname,
- SMB_STRUCT_STAT *psbuf)
-{
- struct smb_filename *smb_fname;
- int ret;
-
- smb_fname = synthetic_smb_fname_split(talloc_tos(), fname, NULL);
- if (smb_fname == NULL) {
- errno = ENOMEM;
- return -1;
- }
-
- if (lp_posix_pathnames()) {
- ret = SMB_VFS_LSTAT(conn, smb_fname);
- } else {
- ret = SMB_VFS_STAT(conn, smb_fname);
- }
-
- if (ret != -1) {
- *psbuf = smb_fname->st;
- }
-
- TALLOC_FREE(smb_fname);
- return ret;
-}
-
-/**
- * XXX: This is temporary and there should be no callers of this once
- * smb_filename is plumbed through all path based operations.
- */
-int vfs_lstat_smb_fname(struct connection_struct *conn, const char *fname,
- SMB_STRUCT_STAT *psbuf)
-{
- struct smb_filename *smb_fname;
- int ret;
-
- smb_fname = synthetic_smb_fname_split(talloc_tos(), fname, NULL);
- if (smb_fname == NULL) {
- errno = ENOMEM;
- return -1;
- }
-
- ret = SMB_VFS_LSTAT(conn, smb_fname);
- if (ret != -1) {
- *psbuf = smb_fname->st;
- }
-
- TALLOC_FREE(smb_fname);
- return ret;
-}
-
-/**
- * XXX: This is temporary and there should be no callers of this once
- * smb_filename is plumbed through all path based operations.
*
* Called when we know stream name parsing has already been done.
*/
--
2.2.0.rc0.207.ga3a616c
From bd872bd4a3b4f88422f02a5d51a40972297b2121 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 1 May 2015 21:06:20 -0700
Subject: [PATCH 4/5] s3: smbd: VFS: For all EA and ACL calls use
synthetic_smb_fname(), not synthetic_smb_fname_split().
EA's and ACL paths are all post-stream name checks (and shouldn't
get stream names). This one took a *long* time to find.
https://bugzilla.samba.org/show_bug.cgi?id=11249
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/modules/non_posix_acls.c | 2 +-
source3/modules/vfs_fake_acls.c | 2 +-
source3/modules/vfs_xattr_tdb.c | 2 +-
source3/smbd/posix_acls.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/source3/modules/non_posix_acls.c b/source3/modules/non_posix_acls.c
index b1c2420..fca9979 100644
--- a/source3/modules/non_posix_acls.c
+++ b/source3/modules/non_posix_acls.c
@@ -32,7 +32,7 @@ int non_posix_sys_acl_blob_get_file_helper(vfs_handle_struct *handle,
struct xattr_sys_acl_hash_wrapper acl_wrapper = {};
struct smb_filename *smb_fname;
- smb_fname = synthetic_smb_fname_split(frame, path_p, NULL);
+ smb_fname = synthetic_smb_fname(frame, path_p, NULL, NULL);
if (smb_fname == NULL) {
TALLOC_FREE(frame);
errno = ENOMEM;
diff --git a/source3/modules/vfs_fake_acls.c b/source3/modules/vfs_fake_acls.c
index 0e7ebb9..f3c2ebb 100644
--- a/source3/modules/vfs_fake_acls.c
+++ b/source3/modules/vfs_fake_acls.c
@@ -348,7 +348,7 @@ static int fake_acls_sys_acl_delete_def_file(vfs_handle_struct *handle, const ch
TALLOC_CTX *frame = talloc_stackframe();
struct smb_filename *smb_fname;
- smb_fname = synthetic_smb_fname_split(frame, path, NULL);
+ smb_fname = synthetic_smb_fname(frame, path, NULL, NULL);
if (smb_fname == NULL) {
TALLOC_FREE(frame);
errno = ENOMEM;
diff --git a/source3/modules/vfs_xattr_tdb.c b/source3/modules/vfs_xattr_tdb.c
index 66c19f8..2124d38 100644
--- a/source3/modules/vfs_xattr_tdb.c
+++ b/source3/modules/vfs_xattr_tdb.c
@@ -37,7 +37,7 @@ static int xattr_tdb_get_file_id(struct vfs_handle_struct *handle,
TALLOC_CTX *frame = talloc_stackframe();
struct smb_filename *smb_fname;
- smb_fname = synthetic_smb_fname_split(frame, path, NULL);
+ smb_fname = synthetic_smb_fname(frame, path, NULL, NULL);
if (smb_fname == NULL) {
TALLOC_FREE(frame);
errno = ENOMEM;
diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c
index 6a5ec85..8b4c7b1 100644
--- a/source3/smbd/posix_acls.c
+++ b/source3/smbd/posix_acls.c
@@ -4749,7 +4749,7 @@ int posix_sys_acl_blob_get_file(vfs_handle_struct *handle,
};
struct smb_filename *smb_fname;
- smb_fname = synthetic_smb_fname_split(frame, path_p, NULL);
+ smb_fname = synthetic_smb_fname(frame, path_p, NULL, NULL);
if (smb_fname == NULL) {
TALLOC_FREE(frame);
errno = ENOMEM;
--
2.2.0.rc0.207.ga3a616c
From 8e315e210b207d3e6e7b6eda835fecd33214f8f6 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 1 May 2015 21:08:21 -0700
Subject: [PATCH 5/5] s3: torture: Add regression test for bug #11249.
Bug 11249 - Mangled names do not work with acl_xattr
https://bugzilla.samba.org/show_bug.cgi?id=11249
Signed-off-by: Jeremy Allison <jra at samba.org>
---
selftest/target/Samba3.pm | 13 +++++++++++
source3/script/tests/test_smbclient_s3.sh | 38 +++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 758ca6b..bc52263 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -980,6 +980,9 @@ sub provision($$$$$$$$)
my $lease2_shrdir="$shrdir/SMB3_00";
push(@dirs,$lease2_shrdir);
+ my $manglenames_shrdir="$shrdir/manglenames";
+ push(@dirs,$manglenames_shrdir);
+
# this gets autocreated by winbindd
my $wbsockdir="$prefix_abs/winbindd";
my $wbsockprivdir="$lockdir/winbindd_privileged";
@@ -1063,6 +1066,12 @@ sub provision($$$$$$$$)
close(BADNAME_TARGET);
chmod 0666, $badname_target;
+ ##
+ ## create mangleable directory names in $manglenames_shrdir
+ ##
+ my $manglename_target = "$manglenames_shrdir/foo:bar";
+ mkdir($manglename_target, 0777);
+
my $conffile="$libdir/server.conf";
my $nss_wrapper_pl = "$ENV{PERL} $self->{srcdir}/lib/nss_wrapper/nss_wrapper.pl";
@@ -1338,6 +1347,10 @@ sub provision($$$$$$$$)
path = $badnames_shrdir
guest ok = yes
+[manglenames_share]
+ path = $manglenames_shrdir
+ guest ok = yes
+
[dynamic_share]
path = $shrdir/%R
guest ok = yes
diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh
index e786c9f..faf8d98 100755
--- a/source3/script/tests/test_smbclient_s3.sh
+++ b/source3/script/tests/test_smbclient_s3.sh
@@ -863,6 +863,40 @@ test_bad_names()
fi
}
+# Test accessing an share with a name that must be mangled - with acl_xattrs.
+# We know foo:bar gets mangled to FF4GBY~Q with the default name-mangling algorithm (hash2).
+test_mangled_names()
+{
+ tmpfile=$PREFIX/smbclient_interactive_prompt_commands
+ cat > $tmpfile <<EOF
+ls
+cd FF4GBY~Q
+ls
+quit
+EOF
+ cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/manglenames_share -I $SERVER_IP $ADDARGS < $tmpfile 2>&1'
+ eval echo "$cmd"
+ out=`eval $cmd`
+ ret=$?
+ rm -f $tmpfile
+
+ if [ $ret != 0 ] ; then
+ echo "$out"
+ echo "failed accessing manglenames_share with error $ret"
+ false
+ return
+ fi
+
+ echo "$out" | grep 'NT_STATUS'
+ ret=$?
+ if [ $ret == 0 ] ; then
+ echo "$out"
+ echo "failed - NT_STATUS_XXXX listing \\manglenames_share\\FF4GBY~Q"
+ false
+ fi
+}
+
+
LOGDIR_PREFIX=test_smbclient_s3
# possibly remove old logdirs:
@@ -942,6 +976,10 @@ testit "list a share with bad names (won't convert)" \
test_bad_names || \
failed=`expr $failed + 1`
+testit "list a share with a mangled name + acl_xattr object" \
+ test_mangled_names || \
+ failed=`expr $failed + 1`
+
testit "rm -rf $LOGDIR" \
rm -rf $LOGDIR || \
failed=`expr $failed + 1`
--
2.2.0.rc0.207.ga3a616c
More information about the samba-technical
mailing list