All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
@ 2016-05-27 17:43 Aurélien Aptel
  2016-06-09 16:50 ` Aurélien Aptel
  0 siblings, 1 reply; 20+ messages in thread
From: Aurélien Aptel @ 2016-05-27 17:43 UTC (permalink / raw)
  To: linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Steve French,
	Marcus Hoffmann


[-- Attachment #1.1: Type: text/plain, Size: 994 bytes --]

Hi all,

I've come up with a new solution for the problem (attached). Between
the long, old and irrelevant comments on the bug report and the various
mailing-list threads it was getting hard to understand. So I've written
a summary which explains the problem in details and the 2 proposed
solutions.

http://diobla.info/stuff/bugs/bsc799133/

The new solution is basically switching to the old prefixpath system
when the normal method of querying intermediary path fails. It's much
more simpler.

*Any* feedback would be nice. I haven't noticed any problems so far but
I haven't run the xfs test suite (I still have to figure out how
to make it play nice with cifs...). I'm sending to samba-tech too in
hopes of reviews/comments.

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-fs-cifs-make-share-unaccessible-at-root-level-mounta.patch --]
[-- Type: text/x-patch, Size: 4998 bytes --]

From 3e9af75bea055bf88a4700fced50a7ba38b39b6f Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
Date: Wed, 25 May 2016 19:59:09 +0200
Subject: [PATCH] fs/cifs: make share unaccessible at root level mountable

if, when mounting //HOST/share/sub/dir/foo we can query /sub/dir/foo but
not any of the path components above:

- store the /sub/dir/foo prefix in the cifs super_block info
- in the superblock, set root dentry to the subpath dentry (instead of
  the share root)
- set a flag in the superblock to remember it
- use prefixpath when building path from a dentry

fixes bso#8950

Signed-off-by: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |  2 ++
 fs/cifs/cifsfs.c     | 19 +++++++++++++++++++
 fs/cifs/connect.c    |  3 +++
 fs/cifs/dir.c        | 19 +++++++++++++++++--
 4 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index 3182273..02b9ac3 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -46,6 +46,7 @@
 #define CIFS_MOUNT_CIFS_BACKUPUID 0x200000 /* backup intent bit for a user */
 #define CIFS_MOUNT_CIFS_BACKUPGID 0x400000 /* backup intent bit for a group */
 #define CIFS_MOUNT_MAP_SFM_CHR	0x800000 /* SFM/MAC mapping for illegal chars */
+#define CIFS_MOUNT_USE_PREFIX_PATH 0x1000000 /* make subpath with unaccessible root mountable */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
@@ -67,5 +68,6 @@ struct cifs_sb_info {
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 	struct rcu_head rcu;
+	char *prepath;
 };
 #endif				/* _CIFS_FS_SB_H */
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5d8b7ed..d1fc593 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -649,6 +649,17 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 		dentry = child;
 	} while (!IS_ERR(dentry));
 	kfree(full_path);
+
+	if (IS_ERR(dentry) /* && path accessible */) {
+		/* we know the path is accessible (it is tested
+		 * earlier in cifs_do_mount()) so there must be a perm
+		 * problem */
+		cifs_dbg(VFS, "cannot query directories between root and final path, "
+			 "enabling CIFS_MOUNT_USE_PREFIX_PATH\n");
+		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_USE_PREFIX_PATH;
+		dentry = dget(sb->s_root);
+	}
+
 	return dentry;
 }
 
@@ -688,6 +699,14 @@ cifs_do_mount(struct file_system_type *fs_type,
 		goto out_cifs_sb;
 	}
 
+	if (volume_info->prepath) {
+		cifs_sb->prepath = kstrdup(volume_info->prepath, GFP_KERNEL);
+		if (cifs_sb->prepath == NULL) {
+			root = ERR_PTR(-ENOMEM);
+			goto out_cifs_sb;
+		}
+	}
+
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
 	rc = cifs_mount(cifs_sb, volume_info);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 66736f5..90e57ee 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3887,6 +3887,9 @@ cifs_umount(struct cifs_sb_info *cifs_sb)
 
 	bdi_destroy(&cifs_sb->bdi);
 	kfree(cifs_sb->mountdata);
+	if (cifs_sb->prepath) {
+		kfree(cifs_sb->prepath);
+	}
 	call_rcu(&cifs_sb->rcu, delayed_free);
 }
 
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index c3eb998..5374253 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -84,6 +84,7 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int dfsplen;
+	int pplen = 0;
 	char *full_path;
 	char dirsep;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
@@ -95,8 +96,12 @@ build_path_from_dentry(struct dentry *direntry)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		pplen = cifs_sb->prepath ? strlen(cifs_sb->prepath) + 1 : 0;
+
 cifs_bp_rename_retry:
-	namelen = dfsplen;
+	namelen = dfsplen + pplen;
 	seq = read_seqbegin(&rename_lock);
 	rcu_read_lock();
 	for (temp = direntry; !IS_ROOT(temp);) {
@@ -137,7 +142,7 @@ cifs_bp_rename_retry:
 		}
 	}
 	rcu_read_unlock();
-	if (namelen != dfsplen || read_seqretry(&rename_lock, seq)) {
+	if (namelen != dfsplen + pplen || read_seqretry(&rename_lock, seq)) {
 		cifs_dbg(FYI, "did not end path lookup where expected. namelen=%ddfsplen=%d\n",
 			 namelen, dfsplen);
 		/* presumably this is only possible if racing with a rename
@@ -153,6 +158,16 @@ cifs_bp_rename_retry:
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
 
+	if (pplen) {
+		int i;
+		cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath);
+		memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1);
+		full_path[dfsplen] = '\\';
+		for (i = 0; i < pplen-1; i++)
+			if (full_path[dfsplen+1+i] == '/')
+				full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb);
+	}
+
 	if (dfsplen) {
 		strncpy(full_path, tcon->treeName, dfsplen);
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {
-- 
2.1.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-05-27 17:43 [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again) Aurélien Aptel
@ 2016-06-09 16:50 ` Aurélien Aptel
  2016-06-09 19:27   ` Marcus Hoffmann
  0 siblings, 1 reply; 20+ messages in thread
From: Aurélien Aptel @ 2016-06-09 16:50 UTC (permalink / raw)
  To: linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Steve French,
	Marcus Hoffmann


[-- Attachment #1.1: Type: text/plain, Size: 957 bytes --]

Small update: I've written a powershell script to reproduce the problem
(attached). If you're wondering I'm not using samba see my notes
about it [1].

On the window server:
- Edit $Dir (script will create parent dirs)
- Edit $LimitedUser/$AdminUser to an existing one
- Run the script as admin

On the linux client:
- Mount the share sub dir with the limited user credentials:
  mount //lutze/bug8950/sub/dir' /mnt \
        -o 'domain=LURCH,ip=10.160.5.42,username=bill,password=*****,rw'

My second solution fails for the case when the dir *containing* the
shared dir restricts the limited user. See "HARD MODE" at the end
of the script.

1: http://diobla.info/stuff/bugs/bsc799133/#sec-4

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

[-- Attachment #1.2: repro-8950.ps1 --]
[-- Type: application/octet-stream, Size: 1497 bytes --]

#REQUIRES -Version 3.0

#
# powershell script to reproduce #8950
#

# On the server:
# - Edit $Dir (script will create parent dirs)
# - Edit $LimitedUser to an existing one
# - Run the script

# On the linux client:
# - Mount the share sub dir with the limited user credentials:
#   mount //lutze/bug8950/sub/dir' /mnt \
#         -o 'domain=LURCH,ip=10.160.5.42,username=bill,password=*****,rw'


$Dir = "C:\shares\bug8950\share"
$Dir1 = "sub"
$Dir2 = "dir"
$LimitedUser = "LURCH\bill"
$AdminUser = "LURCH\Administrator"
$Share = "bug8950"

$SubDir = $Dir + "\" + $Dir1 + "\" + $Dir2


if (Test-Path $Dir) {
    Remove-SMBShare -Name $Share -Force
    icacls.exe $Dir /grant:r   "$($AdminUser):(F)"
    icacls.exe $Dir /grant:r   "$($AdminUser):(F)" /T
    Get-ChildItem -Recurse -Path $Dir | Remove-Item -Recurse -Force
    Remove-Item -Recurse -Force $Dir
}

New-Item $SubDir -Type directory -Force
"blahblabh" > $SubDir\file.txt
New-SMBShare -Name $Share -Path $Dir



icacls.exe $Dir /deny    "$($LimitedUser):(F)"
icacls.exe $Dir /grant:r   "$($AdminUser):(F)"

icacls.exe $Dir\$Dir1 /deny    "$($LimitedUser):(F)"
icacls.exe $Dir\$Dir1 /grant:r   "$($AdminUser):(F)"

icacls.exe $SubDir /grant:r "$($LimitedUser):(F)"
icacls.exe $SubDir /grant:r   "$($AdminUser):(F)"
icacls.exe $Dir /inheritance:r /T

# HARD MODE make mounting work with this:
icacls.exe $Dir\.. /remove  $LimitedUser
icacls.exe $Dir\.. /deny    "$($LimitedUser):(F)"

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-06-09 16:50 ` Aurélien Aptel
@ 2016-06-09 19:27   ` Marcus Hoffmann
       [not found]     ` <5759C326.5040508-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org>
  0 siblings, 1 reply; 20+ messages in thread
From: Marcus Hoffmann @ 2016-06-09 19:27 UTC (permalink / raw)
  To: Aurélien Aptel, linux-cifs,
	samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Steve French

Hey Aurélien,
with your script I can reproduce the bug locally now.

I can mount the share (which is on a Windows 8.1 vm) with a Windows 7 PC
with the restricted user account. (Even in hard mode.)
I can mount the share from Linux-cifs using the admin user but not the
restricted user.

(I noticed though that no user has access to the file in the shared dir.
But this doesn't really matter for the test.)

Marcus

On 06/09/2016 06:50 PM, Aurélien Aptel wrote:
> Small update: I've written a powershell script to reproduce the problem
> (attached). If you're wondering I'm not using samba see my notes
> about it [1].
>
> On the window server:
> - Edit $Dir (script will create parent dirs)
> - Edit $LimitedUser/$AdminUser to an existing one
> - Run the script as admin
>
> On the linux client:
> - Mount the share sub dir with the limited user credentials:
>   mount //lutze/bug8950/sub/dir' /mnt \
>         -o 'domain=LURCH,ip=10.160.5.42,username=bill,password=*****,rw'
>
> My second solution fails for the case when the dir *containing* the
> shared dir restricts the limited user. See "HARD MODE" at the end
> of the script.
>
> 1: http://diobla.info/stuff/bugs/bsc799133/#sec-4
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]     ` <5759C326.5040508-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org>
@ 2016-06-10 15:16       ` Aurélien Aptel
  2016-06-12 18:01         ` Marcus Hoffmann
                           ` (3 more replies)
  0 siblings, 4 replies; 20+ messages in thread
From: Aurélien Aptel @ 2016-06-10 15:16 UTC (permalink / raw)
  To: Marcus Hoffmann
  Cc: linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Steve French


[-- Attachment #1.1: Type: text/plain, Size: 1195 bytes --]

On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann
<marcus.hoffmann-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org> wrote:
> Hey Aurélien,
> with your script I can reproduce the bug locally now.

Good.

> I can mount the share (which is on a Windows 8.1 vm) with a Windows 7
> PC with the restricted user account. (Even in hard mode.)
> I can mount the share from Linux-cifs using the admin user but not the
> restricted user.

I've moved some things around. All of the prefix path components are
now checked for accessibility in cifs_do_mount(). This is more
robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.

I've updated the cifs_root_iget() to use the prefix path when necessary
which should take care of the last case (hard mode).

Please test my latest patch (attached).

> (I noticed though that no user has access to the file in the shared
> dir. But this doesn't really matter for the test.)

Indeed.

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-fs-cifs-make-share-unaccessible-at-root-level-mounta.patch --]
[-- Type: text/x-patch, Size: 7849 bytes --]

From e858c28b7bf9b1c76c0a9703727c7bd02bf4a434 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
Date: Wed, 25 May 2016 19:59:09 +0200
Subject: [PATCH] fs/cifs: make share unaccessible at root level mountable

if, when mounting //HOST/share/sub/dir/foo we can query /sub/dir/foo but
not any of the path components above:

- store the /sub/dir/foo prefix in the cifs super_block info
- in the superblock, set root dentry to the subpath dentry (instead of
  the share root)
- set a flag in the superblock to remember it
- use prefixpath when building path from a dentry

fixes bso#8950

Signed-off-by: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |  2 ++
 fs/cifs/cifsfs.c     | 15 ++++++++++++++-
 fs/cifs/connect.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/dir.c        | 19 +++++++++++++++++--
 fs/cifs/inode.c      | 17 +++++++++++++++--
 5 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index 3182273..02b9ac3 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -46,6 +46,7 @@
 #define CIFS_MOUNT_CIFS_BACKUPUID 0x200000 /* backup intent bit for a user */
 #define CIFS_MOUNT_CIFS_BACKUPGID 0x400000 /* backup intent bit for a group */
 #define CIFS_MOUNT_MAP_SFM_CHR	0x800000 /* SFM/MAC mapping for illegal chars */
+#define CIFS_MOUNT_USE_PREFIX_PATH 0x1000000 /* make subpath with unaccessible root mountable */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
@@ -67,5 +68,6 @@ struct cifs_sb_info {
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 	struct rcu_head rcu;
+	char *prepath;
 };
 #endif				/* _CIFS_FS_SB_H */
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5d8b7ed..c75d80c 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -649,6 +649,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 		dentry = child;
 	} while (!IS_ERR(dentry));
 	kfree(full_path);
+
 	return dentry;
 }
 
@@ -688,6 +689,14 @@ cifs_do_mount(struct file_system_type *fs_type,
 		goto out_cifs_sb;
 	}
 
+	if (volume_info->prepath) {
+		cifs_sb->prepath = kstrdup(volume_info->prepath, GFP_KERNEL);
+		if (cifs_sb->prepath == NULL) {
+			root = ERR_PTR(-ENOMEM);
+			goto out_cifs_sb;
+		}
+	}
+
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
 	rc = cifs_mount(cifs_sb, volume_info);
@@ -726,7 +735,11 @@ cifs_do_mount(struct file_system_type *fs_type,
 		sb->s_flags |= MS_ACTIVE;
 	}
 
-	root = cifs_get_root(volume_info, sb);
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) {
+		root = dget(sb->s_root);
+	} else {
+		root = cifs_get_root(volume_info, sb);
+	}
 	if (IS_ERR(root))
 		goto out_super;
 
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 66736f5..b82d7b4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3481,6 +3481,42 @@ cifs_get_volume_info(char *mount_data, const char *devname)
 	return volume_info;
 }
 
+static int
+cifs_are_all_path_components_accessible(struct TCP_Server_Info *server,
+					unsigned int xid,
+					struct cifs_tcon *tcon,
+					struct cifs_sb_info *cifs_sb,
+					char *full_path)
+{
+	int rc;
+	char *s;
+	char sep, tmp;
+
+	sep = CIFS_DIR_SEP(cifs_sb);
+	s = full_path;
+
+	rc = server->ops->is_path_accessible(xid, tcon, cifs_sb, "");
+	while (rc == 0) {
+		/* skip separators */
+		while (*s == sep)
+			s++;
+		if (!*s)
+			break;
+
+		/* next separator */
+		while (*s && *s != sep)
+			s++;
+
+		/* temporarily null-terminate the path at the end of
+		 * the current component */
+		tmp = *s;
+		*s = 0;
+		rc = server->ops->is_path_accessible(xid, tcon, cifs_sb, full_path);
+		*s = tmp;
+	}
+	return rc;
+}
+
 int
 cifs_mount(struct cifs_sb_info *cifs_sb, struct smb_vol *volume_info)
 {
@@ -3618,6 +3654,14 @@ remote_path_check:
 			kfree(full_path);
 			goto mount_fail_check;
 		}
+
+		rc = cifs_are_all_path_components_accessible(server, xid, tcon, cifs_sb, full_path);
+		if (rc != 0) {
+			cifs_dbg(VFS, "cannot query directories between root and final path, "
+				 "enabling CIFS_MOUNT_USE_PREFIX_PATH\n");
+			cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_USE_PREFIX_PATH;
+			rc = 0;
+		}
 		kfree(full_path);
 	}
 
@@ -3887,6 +3931,9 @@ cifs_umount(struct cifs_sb_info *cifs_sb)
 
 	bdi_destroy(&cifs_sb->bdi);
 	kfree(cifs_sb->mountdata);
+	if (cifs_sb->prepath) {
+		kfree(cifs_sb->prepath);
+	}
 	call_rcu(&cifs_sb->rcu, delayed_free);
 }
 
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index c3eb998..5374253 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -84,6 +84,7 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int dfsplen;
+	int pplen = 0;
 	char *full_path;
 	char dirsep;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
@@ -95,8 +96,12 @@ build_path_from_dentry(struct dentry *direntry)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		pplen = cifs_sb->prepath ? strlen(cifs_sb->prepath) + 1 : 0;
+
 cifs_bp_rename_retry:
-	namelen = dfsplen;
+	namelen = dfsplen + pplen;
 	seq = read_seqbegin(&rename_lock);
 	rcu_read_lock();
 	for (temp = direntry; !IS_ROOT(temp);) {
@@ -137,7 +142,7 @@ cifs_bp_rename_retry:
 		}
 	}
 	rcu_read_unlock();
-	if (namelen != dfsplen || read_seqretry(&rename_lock, seq)) {
+	if (namelen != dfsplen + pplen || read_seqretry(&rename_lock, seq)) {
 		cifs_dbg(FYI, "did not end path lookup where expected. namelen=%ddfsplen=%d\n",
 			 namelen, dfsplen);
 		/* presumably this is only possible if racing with a rename
@@ -153,6 +158,16 @@ cifs_bp_rename_retry:
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
 
+	if (pplen) {
+		int i;
+		cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath);
+		memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1);
+		full_path[dfsplen] = '\\';
+		for (i = 0; i < pplen-1; i++)
+			if (full_path[dfsplen+1+i] == '/')
+				full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb);
+	}
+
 	if (dfsplen) {
 		strncpy(full_path, tcon->treeName, dfsplen);
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 514dadb..7c6edd7 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1002,10 +1002,21 @@ struct inode *cifs_root_iget(struct super_block *sb)
 	struct inode *inode = NULL;
 	long rc;
 	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	char *path = NULL;
+	int len;
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) && cifs_sb->prepath) {
+		len = strlen(cifs_sb->prepath);
+		path = kzalloc(len + 2 /* leading sep + null */, GFP_KERNEL);
+		path[0] = '/';
+		memcpy(path+1, cifs_sb->prepath, len);
+	} else {
+		path = kstrdup("", GFP_KERNEL);
+	}
 
 	xid = get_xid();
 	if (tcon->unix_ext) {
-		rc = cifs_get_inode_info_unix(&inode, "", sb, xid);
+		rc = cifs_get_inode_info_unix(&inode, path, sb, xid);
 		/* some servers mistakenly claim POSIX support */
 		if (rc != -EOPNOTSUPP)
 			goto iget_no_retry;
@@ -1013,7 +1024,8 @@ struct inode *cifs_root_iget(struct super_block *sb)
 		tcon->unix_ext = false;
 	}
 
-	rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL);
+	convert_delimiter(path, CIFS_DIR_SEP(cifs_sb));
+	rc = cifs_get_inode_info(&inode, path, NULL, sb, xid, NULL);
 
 iget_no_retry:
 	if (!inode) {
@@ -1042,6 +1054,7 @@ iget_no_retry:
 	}
 
 out:
+	kfree(path);
 	/* can not call macro free_xid here since in a void func
 	 * TODO: This is no longer true
 	 */
-- 
2.1.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-06-10 15:16       ` Aurélien Aptel
@ 2016-06-12 18:01         ` Marcus Hoffmann
  2016-07-01 15:44         ` Marcus Hoffmann
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 20+ messages in thread
From: Marcus Hoffmann @ 2016-06-12 18:01 UTC (permalink / raw)
  To: Aurélien Aptel; +Cc: linux-cifs, Steve French

On 06/10/2016 05:16 PM, Aurélien Aptel wrote:
> On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann
> <marcus.hoffmann-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org> wrote:
>> Hey Aurélien,
>> with your script I can reproduce the bug locally now.
> 
> Good.
> 
>> I can mount the share (which is on a Windows 8.1 vm) with a Windows 7
>> PC with the restricted user account. (Even in hard mode.)
>> I can mount the share from Linux-cifs using the admin user but not the
>> restricted user.
> 
> I've moved some things around. All of the prefix path components are
> now checked for accessibility in cifs_do_mount(). This is more
> robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
> 
> I've updated the cifs_root_iget() to use the prefix path when necessary
> which should take care of the last case (hard mode).
> 
> Please test my latest patch (attached).

I can confirm it works with our setup now.
Any chance in getting this merged?

> 
>> (I noticed though that no user has access to the file in the shared
>> dir. But this doesn't really matter for the test.)
> 
> Indeed.
> 

Thanks for your work on this,
Marcus

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-06-10 15:16       ` Aurélien Aptel
  2016-06-12 18:01         ` Marcus Hoffmann
@ 2016-07-01 15:44         ` Marcus Hoffmann
       [not found]           ` <57768FC3.7020102-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org>
  2016-07-02  7:02         ` Pavel Shilovsky
  2016-07-29 13:11         ` Sachin Prabhu
  3 siblings, 1 reply; 20+ messages in thread
From: Marcus Hoffmann @ 2016-07-01 15:44 UTC (permalink / raw)
  To: Aurélien Aptel; +Cc: linux-cifs, Steve French

On 06/10/2016 05:16 PM, Aurélien Aptel wrote:
> On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann
> <marcus.hoffmann-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org> wrote:
>> Hey Aurélien,
>> with your script I can reproduce the bug locally now.
> 
> Good.
> 
>> I can mount the share (which is on a Windows 8.1 vm) with a Windows 7
>> PC with the restricted user account. (Even in hard mode.)
>> I can mount the share from Linux-cifs using the admin user but not the
>> restricted user.
> 
> I've moved some things around. All of the prefix path components are
> now checked for accessibility in cifs_do_mount(). This is more
> robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
> 
> I've updated the cifs_root_iget() to use the prefix path when necessary
> which should take care of the last case (hard mode).
> 
> Please test my latest patch (attached).
> 
I just wanted to ask what can be done to get this merged.

>> (I noticed though that no user has access to the file in the shared
>> dir. But this doesn't really matter for the test.)
> 
> Indeed.
> 

Marcus

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]           ` <57768FC3.7020102-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org>
@ 2016-07-01 16:02             ` Steve French
  0 siblings, 0 replies; 20+ messages in thread
From: Steve French @ 2016-07-01 16:02 UTC (permalink / raw)
  To: Marcus Hoffmann; +Cc: Aurélien Aptel, linux-cifs

I will take a look later today.

On Fri, Jul 1, 2016 at 10:44 AM, Marcus Hoffmann
<marcus.hoffmann-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org> wrote:
> On 06/10/2016 05:16 PM, Aurélien Aptel wrote:
>> On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann
>> <marcus.hoffmann-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org> wrote:
>>> Hey Aurélien,
>>> with your script I can reproduce the bug locally now.
>>
>> Good.
>>
>>> I can mount the share (which is on a Windows 8.1 vm) with a Windows 7
>>> PC with the restricted user account. (Even in hard mode.)
>>> I can mount the share from Linux-cifs using the admin user but not the
>>> restricted user.
>>
>> I've moved some things around. All of the prefix path components are
>> now checked for accessibility in cifs_do_mount(). This is more
>> robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
>>
>> I've updated the cifs_root_iget() to use the prefix path when necessary
>> which should take care of the last case (hard mode).
>>
>> Please test my latest patch (attached).
>>
> I just wanted to ask what can be done to get this merged.
>
>>> (I noticed though that no user has access to the file in the shared
>>> dir. But this doesn't really matter for the test.)
>>
>> Indeed.
>>
>
> Marcus



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-06-10 15:16       ` Aurélien Aptel
  2016-06-12 18:01         ` Marcus Hoffmann
  2016-07-01 15:44         ` Marcus Hoffmann
@ 2016-07-02  7:02         ` Pavel Shilovsky
       [not found]           ` <CAKywueRMvJ4B6ojqA1TduS4nGFTr5m4wLO2=0M_EVv=vw2T1pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2016-07-29 13:11         ` Sachin Prabhu
  3 siblings, 1 reply; 20+ messages in thread
From: Pavel Shilovsky @ 2016-07-02  7:02 UTC (permalink / raw)
  To: Aurélien Aptel
  Cc: Marcus Hoffmann, linux-cifs, samba-technical, Steve French

2016-06-10 18:16 GMT+03:00 Aurélien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>:
...
> I've moved some things around. All of the prefix path components are
> now checked for accessibility in cifs_do_mount(). This is more
> robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
>
> I've updated the cifs_root_iget() to use the prefix path when necessary
> which should take care of the last case (hard mode).
>
> Please test my latest patch (attached).

Hi Aurélien,

Thank you for the patch, it looks good like a right thing to do.

I put my comments below:

@@ -649,6 +649,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
  dentry = child;
  } while (!IS_ERR(dentry));
  kfree(full_path);
+
  return dentry;
 }

Please remove this unnecessary change - probably it will go to stable
some day and may cause extra conflicts.


@@ -1002,10 +1002,21 @@ struct inode *cifs_root_iget(struct super_block *sb)
  struct inode *inode = NULL;
  long rc;
  struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+ char *path = NULL;
+ int len;
+
+ if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) &&
cifs_sb->prepath) {
+ len = strlen(cifs_sb->prepath);
+ path = kzalloc(len + 2 /* leading sep + null */, GFP_KERNEL);
+ path[0] = '/';
+ memcpy(path+1, cifs_sb->prepath, len);
+ } else {
+ path = kstrdup("", GFP_KERNEL);
+ }

The above code should check for possible memory allocation failure.

-- 
Best regards,
Pavel Shilovsky

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]           ` <CAKywueRMvJ4B6ojqA1TduS4nGFTr5m4wLO2=0M_EVv=vw2T1pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-07-18 14:38             ` Aurélien Aptel
  2016-07-19 19:21               ` Pavel Shilovsky
  0 siblings, 1 reply; 20+ messages in thread
From: Aurélien Aptel @ 2016-07-18 14:38 UTC (permalink / raw)
  To: Pavel Shilovsky
  Cc: Marcus Hoffmann, linux-cifs, samba-technical, Steve French


[-- Attachment #1.1: Type: text/plain, Size: 634 bytes --]

On Sat, 2 Jul 2016 10:02:36 +0300 Pavel Shilovsky
<pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> Please remove this unnecessary change - probably it will go to stable
> some day and may cause extra conflicts.

> The above code should check for possible memory allocation failure.

I've added (new patch attached) NULL checks and removed the gratuitous
empty line.

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-fs-cifs-make-share-unaccessible-at-root-level-mounta.patch --]
[-- Type: text/x-patch, Size: 7976 bytes --]

From 46164fc278332f9dce945271048ec6f530d52635 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
Date: Wed, 25 May 2016 19:59:09 +0200
Subject: [PATCH] fs/cifs: make share unaccessible at root level mountable

if, when mounting //HOST/share/sub/dir/foo we can query /sub/dir/foo but
not any of the path components above:

- store the /sub/dir/foo prefix in the cifs super_block info
- in the superblock, set root dentry to the subpath dentry (instead of
  the share root)
- set a flag in the superblock to remember it
- use prefixpath when building path from a dentry

fixes bso#8950

Signed-off-by: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |  2 ++
 fs/cifs/cifsfs.c     | 15 ++++++++++++++-
 fs/cifs/connect.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/dir.c        | 19 +++++++++++++++++--
 fs/cifs/inode.c      | 23 +++++++++++++++++++++--
 5 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index 3182273..02b9ac3 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -46,6 +46,7 @@
 #define CIFS_MOUNT_CIFS_BACKUPUID 0x200000 /* backup intent bit for a user */
 #define CIFS_MOUNT_CIFS_BACKUPGID 0x400000 /* backup intent bit for a group */
 #define CIFS_MOUNT_MAP_SFM_CHR	0x800000 /* SFM/MAC mapping for illegal chars */
+#define CIFS_MOUNT_USE_PREFIX_PATH 0x1000000 /* make subpath with unaccessible root mountable */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
@@ -67,5 +68,6 @@ struct cifs_sb_info {
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 	struct rcu_head rcu;
+	char *prepath;
 };
 #endif				/* _CIFS_FS_SB_H */
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5d841f3..cce6eac 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -650,6 +650,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 		dentry = child;
 	} while (!IS_ERR(dentry));
 	kfree(full_path);
+
 	return dentry;
 }
 
@@ -689,6 +690,14 @@ cifs_do_mount(struct file_system_type *fs_type,
 		goto out_cifs_sb;
 	}
 
+	if (volume_info->prepath) {
+		cifs_sb->prepath = kstrdup(volume_info->prepath, GFP_KERNEL);
+		if (cifs_sb->prepath == NULL) {
+			root = ERR_PTR(-ENOMEM);
+			goto out_cifs_sb;
+		}
+	}
+
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
 	rc = cifs_mount(cifs_sb, volume_info);
@@ -727,7 +736,11 @@ cifs_do_mount(struct file_system_type *fs_type,
 		sb->s_flags |= MS_ACTIVE;
 	}
 
-	root = cifs_get_root(volume_info, sb);
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) {
+		root = dget(sb->s_root);
+	} else {
+		root = cifs_get_root(volume_info, sb);
+	}
 	if (IS_ERR(root))
 		goto out_super;
 
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 7d2b15c..751d628 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3483,6 +3483,42 @@ cifs_get_volume_info(char *mount_data, const char *devname)
 	return volume_info;
 }
 
+static int
+cifs_are_all_path_components_accessible(struct TCP_Server_Info *server,
+					unsigned int xid,
+					struct cifs_tcon *tcon,
+					struct cifs_sb_info *cifs_sb,
+					char *full_path)
+{
+	int rc;
+	char *s;
+	char sep, tmp;
+
+	sep = CIFS_DIR_SEP(cifs_sb);
+	s = full_path;
+
+	rc = server->ops->is_path_accessible(xid, tcon, cifs_sb, "");
+	while (rc == 0) {
+		/* skip separators */
+		while (*s == sep)
+			s++;
+		if (!*s)
+			break;
+
+		/* next separator */
+		while (*s && *s != sep)
+			s++;
+
+		/* temporarily null-terminate the path at the end of
+		 * the current component */
+		tmp = *s;
+		*s = 0;
+		rc = server->ops->is_path_accessible(xid, tcon, cifs_sb, full_path);
+		*s = tmp;
+	}
+	return rc;
+}
+
 int
 cifs_mount(struct cifs_sb_info *cifs_sb, struct smb_vol *volume_info)
 {
@@ -3620,6 +3656,14 @@ remote_path_check:
 			kfree(full_path);
 			goto mount_fail_check;
 		}
+
+		rc = cifs_are_all_path_components_accessible(server, xid, tcon, cifs_sb, full_path);
+		if (rc != 0) {
+			cifs_dbg(VFS, "cannot query directories between root and final path, "
+				 "enabling CIFS_MOUNT_USE_PREFIX_PATH\n");
+			cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_USE_PREFIX_PATH;
+			rc = 0;
+		}
 		kfree(full_path);
 	}
 
@@ -3889,6 +3933,9 @@ cifs_umount(struct cifs_sb_info *cifs_sb)
 
 	bdi_destroy(&cifs_sb->bdi);
 	kfree(cifs_sb->mountdata);
+	if (cifs_sb->prepath) {
+		kfree(cifs_sb->prepath);
+	}
 	call_rcu(&cifs_sb->rcu, delayed_free);
 }
 
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index fb0903f..fb4de01 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -84,6 +84,7 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int dfsplen;
+	int pplen = 0;
 	char *full_path;
 	char dirsep;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
@@ -95,8 +96,12 @@ build_path_from_dentry(struct dentry *direntry)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		pplen = cifs_sb->prepath ? strlen(cifs_sb->prepath) + 1 : 0;
+
 cifs_bp_rename_retry:
-	namelen = dfsplen;
+	namelen = dfsplen + pplen;
 	seq = read_seqbegin(&rename_lock);
 	rcu_read_lock();
 	for (temp = direntry; !IS_ROOT(temp);) {
@@ -137,7 +142,7 @@ cifs_bp_rename_retry:
 		}
 	}
 	rcu_read_unlock();
-	if (namelen != dfsplen || read_seqretry(&rename_lock, seq)) {
+	if (namelen != dfsplen + pplen || read_seqretry(&rename_lock, seq)) {
 		cifs_dbg(FYI, "did not end path lookup where expected. namelen=%ddfsplen=%d\n",
 			 namelen, dfsplen);
 		/* presumably this is only possible if racing with a rename
@@ -153,6 +158,16 @@ cifs_bp_rename_retry:
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
 
+	if (pplen) {
+		int i;
+		cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath);
+		memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1);
+		full_path[dfsplen] = '\\';
+		for (i = 0; i < pplen-1; i++)
+			if (full_path[dfsplen+1+i] == '/')
+				full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb);
+	}
+
 	if (dfsplen) {
 		strncpy(full_path, tcon->treeName, dfsplen);
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 514dadb..f2f4af0 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1002,10 +1002,27 @@ struct inode *cifs_root_iget(struct super_block *sb)
 	struct inode *inode = NULL;
 	long rc;
 	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	char *path = NULL;
+	int len;
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) && cifs_sb->prepath) {
+		len = strlen(cifs_sb->prepath);
+		path = kzalloc(len + 2 /* leading sep + null */, GFP_KERNEL);
+		if (path == NULL) {
+			return ERR_PTR(-ENOMEM);
+		}
+		path[0] = '/';
+		memcpy(path+1, cifs_sb->prepath, len);
+	} else {
+		path = kstrdup("", GFP_KERNEL);
+		if (path == NULL) {
+			return ERR_PTR(-ENOMEM);
+		}
+	}
 
 	xid = get_xid();
 	if (tcon->unix_ext) {
-		rc = cifs_get_inode_info_unix(&inode, "", sb, xid);
+		rc = cifs_get_inode_info_unix(&inode, path, sb, xid);
 		/* some servers mistakenly claim POSIX support */
 		if (rc != -EOPNOTSUPP)
 			goto iget_no_retry;
@@ -1013,7 +1030,8 @@ struct inode *cifs_root_iget(struct super_block *sb)
 		tcon->unix_ext = false;
 	}
 
-	rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL);
+	convert_delimiter(path, CIFS_DIR_SEP(cifs_sb));
+	rc = cifs_get_inode_info(&inode, path, NULL, sb, xid, NULL);
 
 iget_no_retry:
 	if (!inode) {
@@ -1042,6 +1060,7 @@ iget_no_retry:
 	}
 
 out:
+	kfree(path);
 	/* can not call macro free_xid here since in a void func
 	 * TODO: This is no longer true
 	 */
-- 
2.1.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-07-18 14:38             ` Aurélien Aptel
@ 2016-07-19 19:21               ` Pavel Shilovsky
       [not found]                 ` <CAKywueRFMu9nvwi_01Yz0HpOqhrK2yZVaLT2JMqw4622irQzNw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 20+ messages in thread
From: Pavel Shilovsky @ 2016-07-19 19:21 UTC (permalink / raw)
  To: Aurélien Aptel
  Cc: Marcus Hoffmann, linux-cifs, samba-technical, Steve French

2016-07-18 17:38 GMT+03:00 Aurélien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>:
> On Sat, 2 Jul 2016 10:02:36 +0300 Pavel Shilovsky
> <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> Please remove this unnecessary change - probably it will go to stable
>> some day and may cause extra conflicts.
>
>> The above code should check for possible memory allocation failure.
>
> I've added (new patch attached) NULL checks and removed the gratuitous
> empty line.
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
> Nürnberg)

I suggest you to run scripts/checkpatch.pl against your patch - it has
several warnings like {} braces for single statement blocks. Also the
patch from the attachment still has an empty line in fs/cifs/cifsfs.c.

-- 
Best regards,
Pavel Shilovsky

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]                 ` <CAKywueRFMu9nvwi_01Yz0HpOqhrK2yZVaLT2JMqw4622irQzNw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-07-20 10:57                   ` Aurélien Aptel
  2016-07-20 12:16                     ` Aurélien Aptel
  0 siblings, 1 reply; 20+ messages in thread
From: Aurélien Aptel @ 2016-07-20 10:57 UTC (permalink / raw)
  To: Pavel Shilovsky
  Cc: Marcus Hoffmann, linux-cifs, samba-technical, Steve French


[-- Attachment #1.1: Type: text/plain, Size: 738 bytes --]

On Tue, 19 Jul 2016 22:21:26 +0300 Pavel Shilovsky
<piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> I suggest you to run scripts/checkpatch.pl against your patch - it has
> several warnings like {} braces for single statement blocks. Also the
> patch from the attachment still has an empty line in fs/cifs/cifsfs.c.

Oops. Okay this time checkpatch.pl only complains about a quoted string
split across lines wich I'm not really sure how to fix so I'm leaving
that as is.

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-fs-cifs-make-share-unaccessible-at-root-level-mounta.patch --]
[-- Type: text/x-patch, Size: 7824 bytes --]

From 8035f8f1b8771e93fb66b4fe8db954fb0c5d5f43 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
Date: Wed, 25 May 2016 19:59:09 +0200
Subject: [PATCH] fs/cifs: make share unaccessible at root level mountable

if, when mounting //HOST/share/sub/dir/foo we can query /sub/dir/foo but
not any of the path components above:

- store the /sub/dir/foo prefix in the cifs super_block info
- in the superblock, set root dentry to the subpath dentry (instead of
  the share root)
- set a flag in the superblock to remember it
- use prefixpath when building path from a dentry

fixes bso#8950

Signed-off-by: Aurelien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |  4 ++++
 fs/cifs/cifsfs.c     | 14 +++++++++++++-
 fs/cifs/connect.c    | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/dir.c        | 20 ++++++++++++++++++--
 fs/cifs/inode.c      | 22 ++++++++++++++++++++--
 5 files changed, 104 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index 3182273..1418daa 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -46,6 +46,9 @@
 #define CIFS_MOUNT_CIFS_BACKUPUID 0x200000 /* backup intent bit for a user */
 #define CIFS_MOUNT_CIFS_BACKUPGID 0x400000 /* backup intent bit for a group */
 #define CIFS_MOUNT_MAP_SFM_CHR	0x800000 /* SFM/MAC mapping for illegal chars */
+#define CIFS_MOUNT_USE_PREFIX_PATH 0x1000000 /* make subpath with unaccessible
+					      * root mountable
+					      */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;
@@ -67,5 +70,6 @@ struct cifs_sb_info {
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 	struct rcu_head rcu;
+	char *prepath;
 };
 #endif				/* _CIFS_FS_SB_H */
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5d841f3..6bbec5e 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -689,6 +689,14 @@ cifs_do_mount(struct file_system_type *fs_type,
 		goto out_cifs_sb;
 	}
 
+	if (volume_info->prepath) {
+		cifs_sb->prepath = kstrdup(volume_info->prepath, GFP_KERNEL);
+		if (cifs_sb->prepath == NULL) {
+			root = ERR_PTR(-ENOMEM);
+			goto out_cifs_sb;
+		}
+	}
+
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
 	rc = cifs_mount(cifs_sb, volume_info);
@@ -727,7 +735,11 @@ cifs_do_mount(struct file_system_type *fs_type,
 		sb->s_flags |= MS_ACTIVE;
 	}
 
-	root = cifs_get_root(volume_info, sb);
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		root = dget(sb->s_root);
+	else
+		root = cifs_get_root(volume_info, sb);
+
 	if (IS_ERR(root))
 		goto out_super;
 
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 7d2b15c..7304aab 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3483,6 +3483,44 @@ cifs_get_volume_info(char *mount_data, const char *devname)
 	return volume_info;
 }
 
+static int
+cifs_are_all_path_components_accessible(struct TCP_Server_Info *server,
+					unsigned int xid,
+					struct cifs_tcon *tcon,
+					struct cifs_sb_info *cifs_sb,
+					char *full_path)
+{
+	int rc;
+	char *s;
+	char sep, tmp;
+
+	sep = CIFS_DIR_SEP(cifs_sb);
+	s = full_path;
+
+	rc = server->ops->is_path_accessible(xid, tcon, cifs_sb, "");
+	while (rc == 0) {
+		/* skip separators */
+		while (*s == sep)
+			s++;
+		if (!*s)
+			break;
+		/* next separator */
+		while (*s && *s != sep)
+			s++;
+
+		/*
+		 * temporarily null-terminate the path at the end of
+		 * the current component
+		 */
+		tmp = *s;
+		*s = 0;
+		rc = server->ops->is_path_accessible(xid, tcon, cifs_sb,
+						     full_path);
+		*s = tmp;
+	}
+	return rc;
+}
+
 int
 cifs_mount(struct cifs_sb_info *cifs_sb, struct smb_vol *volume_info)
 {
@@ -3620,6 +3658,16 @@ remote_path_check:
 			kfree(full_path);
 			goto mount_fail_check;
 		}
+
+		rc = cifs_are_all_path_components_accessible(server,
+							     xid, tcon, cifs_sb,
+							     full_path);
+		if (rc != 0) {
+			cifs_dbg(VFS, "cannot query dirs between root and final path, "
+				 "enabling CIFS_MOUNT_USE_PREFIX_PATH\n");
+			cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_USE_PREFIX_PATH;
+			rc = 0;
+		}
 		kfree(full_path);
 	}
 
@@ -3889,6 +3937,7 @@ cifs_umount(struct cifs_sb_info *cifs_sb)
 
 	bdi_destroy(&cifs_sb->bdi);
 	kfree(cifs_sb->mountdata);
+	kfree(cifs_sb->prepath);
 	call_rcu(&cifs_sb->rcu, delayed_free);
 }
 
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index fb0903f..b8dbbe0 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -84,6 +84,7 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int dfsplen;
+	int pplen = 0;
 	char *full_path;
 	char dirsep;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
@@ -95,8 +96,12 @@ build_path_from_dentry(struct dentry *direntry)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		pplen = cifs_sb->prepath ? strlen(cifs_sb->prepath) + 1 : 0;
+
 cifs_bp_rename_retry:
-	namelen = dfsplen;
+	namelen = dfsplen + pplen;
 	seq = read_seqbegin(&rename_lock);
 	rcu_read_lock();
 	for (temp = direntry; !IS_ROOT(temp);) {
@@ -137,7 +142,7 @@ cifs_bp_rename_retry:
 		}
 	}
 	rcu_read_unlock();
-	if (namelen != dfsplen || read_seqretry(&rename_lock, seq)) {
+	if (namelen != dfsplen + pplen || read_seqretry(&rename_lock, seq)) {
 		cifs_dbg(FYI, "did not end path lookup where expected. namelen=%ddfsplen=%d\n",
 			 namelen, dfsplen);
 		/* presumably this is only possible if racing with a rename
@@ -153,6 +158,17 @@ cifs_bp_rename_retry:
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
 
+	if (pplen) {
+		int i;
+
+		cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath);
+		memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1);
+		full_path[dfsplen] = '\\';
+		for (i = 0; i < pplen-1; i++)
+			if (full_path[dfsplen+1+i] == '/')
+				full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb);
+	}
+
 	if (dfsplen) {
 		strncpy(full_path, tcon->treeName, dfsplen);
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 514dadb..b87efd0 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1002,10 +1002,26 @@ struct inode *cifs_root_iget(struct super_block *sb)
 	struct inode *inode = NULL;
 	long rc;
 	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	char *path = NULL;
+	int len;
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+	    && cifs_sb->prepath) {
+		len = strlen(cifs_sb->prepath);
+		path = kzalloc(len + 2 /* leading sep + null */, GFP_KERNEL);
+		if (path == NULL)
+			return ERR_PTR(-ENOMEM);
+		path[0] = '/';
+		memcpy(path+1, cifs_sb->prepath, len);
+	} else {
+		path = kstrdup("", GFP_KERNEL);
+		if (path == NULL)
+			return ERR_PTR(-ENOMEM);
+	}
 
 	xid = get_xid();
 	if (tcon->unix_ext) {
-		rc = cifs_get_inode_info_unix(&inode, "", sb, xid);
+		rc = cifs_get_inode_info_unix(&inode, path, sb, xid);
 		/* some servers mistakenly claim POSIX support */
 		if (rc != -EOPNOTSUPP)
 			goto iget_no_retry;
@@ -1013,7 +1029,8 @@ struct inode *cifs_root_iget(struct super_block *sb)
 		tcon->unix_ext = false;
 	}
 
-	rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL);
+	convert_delimiter(path, CIFS_DIR_SEP(cifs_sb));
+	rc = cifs_get_inode_info(&inode, path, NULL, sb, xid, NULL);
 
 iget_no_retry:
 	if (!inode) {
@@ -1042,6 +1059,7 @@ iget_no_retry:
 	}
 
 out:
+	kfree(path);
 	/* can not call macro free_xid here since in a void func
 	 * TODO: This is no longer true
 	 */
-- 
2.1.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-07-20 10:57                   ` Aurélien Aptel
@ 2016-07-20 12:16                     ` Aurélien Aptel
  2016-07-20 18:28                       ` Pavel Shilovsky
  0 siblings, 1 reply; 20+ messages in thread
From: Aurélien Aptel @ 2016-07-20 12:16 UTC (permalink / raw)
  To: Pavel Shilovsky
  Cc: Marcus Hoffmann, linux-cifs, samba-technical, Steve French

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On Wed, 20 Jul 2016 12:57:02 +0200 Aurélien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>
wrote:
> Oops. Okay this time checkpatch.pl only complains about a quoted
> string split across lines wich I'm not really sure how to fix so I'm
> leaving that as is.

...because not splitting it would make the line >80 chars. The whole
point of doing this is AFAIU to be able to grep the message so using "\"
wouldn't fix it really.

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-07-20 12:16                     ` Aurélien Aptel
@ 2016-07-20 18:28                       ` Pavel Shilovsky
       [not found]                         ` <CAKywueTOSD0G1k+EU-Qo_9D7S5bBw6g6T=dbQpWYWdOhr5Lsrg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 20+ messages in thread
From: Pavel Shilovsky @ 2016-07-20 18:28 UTC (permalink / raw)
  To: Aurélien Aptel
  Cc: Marcus Hoffmann, linux-cifs, samba-technical, Steve French

2016-07-20 15:16 GMT+03:00 Aurélien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>:
> On Wed, 20 Jul 2016 12:57:02 +0200 Aurélien Aptel <aaptel@suse.com>
> wrote:
>> Oops. Okay this time checkpatch.pl only complains about a quoted
>> string split across lines wich I'm not really sure how to fix so I'm
>> leaving that as is.
>
> ...because not splitting it would make the line >80 chars. The whole
> point of doing this is AFAIU to be able to grep the message so using "\"
> wouldn't fix it really.
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
> Nürnberg)

The quoted string should not be split even if it is >80 chars.

Anyway, the patch looks good. Thanks for fixing this long-term issue.

Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]                         ` <CAKywueTOSD0G1k+EU-Qo_9D7S5bBw6g6T=dbQpWYWdOhr5Lsrg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-07-26 18:04                           ` Steve French
       [not found]                             ` <CAH2r5mviretFGDaHOre8BiZLmKhqwnfv9sdaiqoAG1xahbVjKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 20+ messages in thread
From: Steve French @ 2016-07-26 18:04 UTC (permalink / raw)
  To: Pavel Shilovsky
  Cc: Aurélien Aptel, Marcus Hoffmann, linux-cifs, samba-technical

thoughts on stable?

On Wed, Jul 20, 2016 at 1:28 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2016-07-20 15:16 GMT+03:00 Aurélien Aptel <aaptel-IBi9RG/b67k@public.gmane.org>:
>> On Wed, 20 Jul 2016 12:57:02 +0200 Aurélien Aptel <aaptel@suse.com>
>> wrote:
>>> Oops. Okay this time checkpatch.pl only complains about a quoted
>>> string split across lines wich I'm not really sure how to fix so I'm
>>> leaving that as is.
>>
>> ...because not splitting it would make the line >80 chars. The whole
>> point of doing this is AFAIU to be able to grep the message so using "\"
>> wouldn't fix it really.
>>
>> --
>> Aurélien Aptel / SUSE Labs Samba Team
>> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
>> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
>> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
>> Nürnberg)
>
> The quoted string should not be split even if it is >80 chars.
>
> Anyway, the patch looks good. Thanks for fixing this long-term issue.
>
> Reviewed-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>
> --
> Best regards,
> Pavel Shilovsky



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]                             ` <CAH2r5mviretFGDaHOre8BiZLmKhqwnfv9sdaiqoAG1xahbVjKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-07-26 19:10                               ` Pavel Shilovsky
       [not found]                                 ` <CAKywueR7K5OR7+NnzEtqpWGR0gApoR3X0Y6C6ACzTf1y7JOcsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 20+ messages in thread
From: Pavel Shilovsky @ 2016-07-26 19:10 UTC (permalink / raw)
  To: Steve French
  Cc: Aurélien Aptel, Marcus Hoffmann, linux-cifs, samba-technical

2016-07-26 21:04 GMT+03:00 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> thoughts on stable?
>

Yes, I think it should go to stable as well.

-- 
Best regards,
Pavel Shilovsky

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]                                 ` <CAKywueR7K5OR7+NnzEtqpWGR0gApoR3X0Y6C6ACzTf1y7JOcsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-07-28  5:02                                   ` Steve French
       [not found]                                     ` <CAH2r5mtiZNDyeRe_rYy4Pcg1WhbGaZtdweM=p8fG1uc0xZcAeg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 20+ messages in thread
From: Steve French @ 2016-07-28  5:02 UTC (permalink / raw)
  To: Pavel Shilovsky
  Cc: Aurélien Aptel, Marcus Hoffmann, linux-cifs, samba-technical

Pushed to cifs-2.6.git

On Tue, Jul 26, 2016 at 2:10 PM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2016-07-26 21:04 GMT+03:00 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> thoughts on stable?
>>
>
> Yes, I think it should go to stable as well.
>
> --
> Best regards,
> Pavel Shilovsky



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]                                     ` <CAH2r5mtiZNDyeRe_rYy4Pcg1WhbGaZtdweM=p8fG1uc0xZcAeg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-07-28  8:28                                       ` Aurélien Aptel
  0 siblings, 0 replies; 20+ messages in thread
From: Aurélien Aptel @ 2016-07-28  8:28 UTC (permalink / raw)
  To: Steve French
  Cc: Pavel Shilovsky, Marcus Hoffmann, linux-cifs, samba-technical

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

On Thu, 28 Jul 2016 00:02:47 -0500 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
> Pushed to cifs-2.6.git

Yay, thanks Steve and Pavel!

-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
  2016-06-10 15:16       ` Aurélien Aptel
                           ` (2 preceding siblings ...)
  2016-07-02  7:02         ` Pavel Shilovsky
@ 2016-07-29 13:11         ` Sachin Prabhu
       [not found]           ` <1469797864.14723.15.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  3 siblings, 1 reply; 20+ messages in thread
From: Sachin Prabhu @ 2016-07-29 13:11 UTC (permalink / raw)
  To: Aurélien Aptel, Marcus Hoffmann
  Cc: linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Steve French

On Fri, 2016-06-10 at 17:16 +0200, Aurélien Aptel wrote:
> On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann
> <marcus.hoffmann-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org> wrote:
> > 
> > Hey Aurélien,
> > with your script I can reproduce the bug locally now.
> Good.
> 
> > 
> > I can mount the share (which is on a Windows 8.1 vm) with a Windows
> > 7
> > PC with the restricted user account. (Even in hard mode.)
> > I can mount the share from Linux-cifs using the admin user but not
> > the
> > restricted user.
> I've moved some things around. All of the prefix path components are
> now checked for accessibility in cifs_do_mount(). This is more
> robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag
> earlier.
> 
> I've updated the cifs_root_iget() to use the prefix path when
> necessary
> which should take care of the last case (hard mode).
> 
> Please test my latest patch (attached).
> 
> > 
> > (I noticed though that no user has access to the file in the shared
> > dir. But this doesn't really matter for the test.)
> Indeed.
> 


Hello,

Sorry for the late reply but this has to be a NACK from me.

We need to check for CIFS_MOUNT_USE_PREFIX_PATH
and if set, check cifs_sb->prepath for both old and new
in cifs_match_super().

Else we have the following bug:

Consider 2 different mounts on a server where root access is limited. I
used the reproducer for this case but simply created a separate folder
in the root directory to which the user has access. I then attempt to
mount the 2 separate folders in 2 different locations.

# mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
52/test2/sub/dir /mnt
# mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
52/test2/sub2/ /mnt2

# grep mnt /proc/mounts
//vm140-52/test2/sub/dir /mnt cifs
rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0,n
oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mode=
0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=60
,actimeo=1 0 0
//vm140-52/test2/sub2/ /mnt2 cifs
rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0,n
oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mode=
0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=60
,actimeo=1 0 0

but since we do not compare the prepath, we end up with the same share
mounted at both mount points. This is the share mounted first.

To confirm.

# date >/mnt/test
# cat /mnt/test /mnt2/test
Fri 29 Jul 14:05:19 BST 2016
Fri 29 Jul 14:05:19 BST 2016

Steve, 

Can you recall the earlier patch or should I write a fix for this?

Sachin Prabhu

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]           ` <1469797864.14723.15.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-07-29 13:31             ` Sachin Prabhu
       [not found]               ` <1469799107.14723.18.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 20+ messages in thread
From: Sachin Prabhu @ 2016-07-29 13:31 UTC (permalink / raw)
  To: Aurélien Aptel, Marcus Hoffmann
  Cc: linux-cifs, samba-technical-w/Ol4Ecudpl8XjKLYN78aQ, Steve French

On Fri, 2016-07-29 at 14:11 +0100, Sachin Prabhu wrote:
> On Fri, 2016-06-10 at 17:16 +0200, Aurélien Aptel wrote:
> > 
> > On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann
> > <marcus.hoffmann-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org> wrote:
> > > 
> > > 
> > > Hey Aurélien,
> > > with your script I can reproduce the bug locally now.
> > Good.
> > 
> > > 
> > > 
> > > I can mount the share (which is on a Windows 8.1 vm) with a
> > > Windows
> > > 7
> > > PC with the restricted user account. (Even in hard mode.)
> > > I can mount the share from Linux-cifs using the admin user but
> > > not
> > > the
> > > restricted user.
> > I've moved some things around. All of the prefix path components
> > are
> > now checked for accessibility in cifs_do_mount(). This is more
> > robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag
> > earlier.
> > 
> > I've updated the cifs_root_iget() to use the prefix path when
> > necessary
> > which should take care of the last case (hard mode).
> > 
> > Please test my latest patch (attached).
> > 
> > > 
> > > 
> > > (I noticed though that no user has access to the file in the
> > > shared
> > > dir. But this doesn't really matter for the test.)
> > Indeed.
> > 
> 
> Hello,
> 
> Sorry for the late reply but this has to be a NACK from me.
> 
> We need to check for CIFS_MOUNT_USE_PREFIX_PATH
> and if set, check cifs_sb->prepath for both old and new
> in cifs_match_super().
> 
> Else we have the following bug:
> 
> Consider 2 different mounts on a server where root access is limited.
> I
> used the reproducer for this case but simply created a separate
> folder
> in the root directory to which the user has access. I then attempt to
> mount the 2 separate folders in 2 different locations.
> 
> # mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
> 52/test2/sub/dir /mnt
> # mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
> 52/test2/sub2/ /mnt2
> 
> # grep mnt /proc/mounts
> //vm140-52/test2/sub/dir /mnt cifs
> rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0
> ,n
> oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mod
> e=
> 0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=
> 60
> ,actimeo=1 0 0
> //vm140-52/test2/sub2/ /mnt2 cifs
> rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0
> ,n
> oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mod
> e=
> 0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=
> 60
> ,actimeo=1 0 0
> 
> but since we do not compare the prepath, we end up with the same
> share
> mounted at both mount points. This is the share mounted first.
> 
> To confirm.
> 
> # date >/mnt/test
> # cat /mnt/test /mnt2/test
> Fri 29 Jul 14:05:19 BST 2016
> Fri 29 Jul 14:05:19 BST 2016
> 
> Steve, 
> 
> Can you recall the earlier patch or should I write a fix for this?
> 
> Sachin Prabhu

This bug in the patch was masked by another issue which was fixed by
the patch

cifs: unbreak TCP session reuse
by Rabin Vincent which has been posted to go into upstream at the same
time as this patch.

Sachin Prabhu

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)
       [not found]               ` <1469799107.14723.18.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2016-07-29 20:20                 ` Steve French
  0 siblings, 0 replies; 20+ messages in thread
From: Steve French @ 2016-07-29 20:20 UTC (permalink / raw)
  To: Sachin Prabhu
  Cc: Aurélien Aptel, Marcus Hoffmann, linux-cifs, samba-technical

Let's add your fix as a followon patch

On Fri, Jul 29, 2016 at 8:31 AM, Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 2016-07-29 at 14:11 +0100, Sachin Prabhu wrote:
>> On Fri, 2016-06-10 at 17:16 +0200, Aurélien Aptel wrote:
>> >
>> > On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann
>> > <marcus.hoffmann-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org> wrote:
>> > >
>> > >
>> > > Hey Aurélien,
>> > > with your script I can reproduce the bug locally now.
>> > Good.
>> >
>> > >
>> > >
>> > > I can mount the share (which is on a Windows 8.1 vm) with a
>> > > Windows
>> > > 7
>> > > PC with the restricted user account. (Even in hard mode.)
>> > > I can mount the share from Linux-cifs using the admin user but
>> > > not
>> > > the
>> > > restricted user.
>> > I've moved some things around. All of the prefix path components
>> > are
>> > now checked for accessibility in cifs_do_mount(). This is more
>> > robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag
>> > earlier.
>> >
>> > I've updated the cifs_root_iget() to use the prefix path when
>> > necessary
>> > which should take care of the last case (hard mode).
>> >
>> > Please test my latest patch (attached).
>> >
>> > >
>> > >
>> > > (I noticed though that no user has access to the file in the
>> > > shared
>> > > dir. But this doesn't really matter for the test.)
>> > Indeed.
>> >
>>
>> Hello,
>>
>> Sorry for the late reply but this has to be a NACK from me.
>>
>> We need to check for CIFS_MOUNT_USE_PREFIX_PATH
>> and if set, check cifs_sb->prepath for both old and new
>> in cifs_match_super().
>>
>> Else we have the following bug:
>>
>> Consider 2 different mounts on a server where root access is limited.
>> I
>> used the reproducer for this case but simply created a separate
>> folder
>> in the root directory to which the user has access. I then attempt to
>> mount the 2 separate folders in 2 different locations.
>>
>> # mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
>> 52/test2/sub/dir /mnt
>> # mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
>> 52/test2/sub2/ /mnt2
>>
>> # grep mnt /proc/mounts
>> //vm140-52/test2/sub/dir /mnt cifs
>> rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0
>> ,n
>> oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mod
>> e=
>> 0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=
>> 60
>> ,actimeo=1 0 0
>> //vm140-52/test2/sub2/ /mnt2 cifs
>> rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0
>> ,n
>> oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mod
>> e=
>> 0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=
>> 60
>> ,actimeo=1 0 0
>>
>> but since we do not compare the prepath, we end up with the same
>> share
>> mounted at both mount points. This is the share mounted first.
>>
>> To confirm.
>>
>> # date >/mnt/test
>> # cat /mnt/test /mnt2/test
>> Fri 29 Jul 14:05:19 BST 2016
>> Fri 29 Jul 14:05:19 BST 2016
>>
>> Steve,
>>
>> Can you recall the earlier patch or should I write a fix for this?
>>
>> Sachin Prabhu
>
> This bug in the patch was masked by another issue which was fixed by
> the patch
>
> cifs: unbreak TCP session reuse
> by Rabin Vincent which has been posted to go into upstream at the same
> time as this patch.
>
> Sachin Prabhu



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2016-07-29 20:20 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-27 17:43 [PATCH] Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again) Aurélien Aptel
2016-06-09 16:50 ` Aurélien Aptel
2016-06-09 19:27   ` Marcus Hoffmann
     [not found]     ` <5759C326.5040508-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org>
2016-06-10 15:16       ` Aurélien Aptel
2016-06-12 18:01         ` Marcus Hoffmann
2016-07-01 15:44         ` Marcus Hoffmann
     [not found]           ` <57768FC3.7020102-j/7cz5qe3tpn68oJJulU0Q@public.gmane.org>
2016-07-01 16:02             ` Steve French
2016-07-02  7:02         ` Pavel Shilovsky
     [not found]           ` <CAKywueRMvJ4B6ojqA1TduS4nGFTr5m4wLO2=0M_EVv=vw2T1pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-07-18 14:38             ` Aurélien Aptel
2016-07-19 19:21               ` Pavel Shilovsky
     [not found]                 ` <CAKywueRFMu9nvwi_01Yz0HpOqhrK2yZVaLT2JMqw4622irQzNw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-07-20 10:57                   ` Aurélien Aptel
2016-07-20 12:16                     ` Aurélien Aptel
2016-07-20 18:28                       ` Pavel Shilovsky
     [not found]                         ` <CAKywueTOSD0G1k+EU-Qo_9D7S5bBw6g6T=dbQpWYWdOhr5Lsrg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-07-26 18:04                           ` Steve French
     [not found]                             ` <CAH2r5mviretFGDaHOre8BiZLmKhqwnfv9sdaiqoAG1xahbVjKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-07-26 19:10                               ` Pavel Shilovsky
     [not found]                                 ` <CAKywueR7K5OR7+NnzEtqpWGR0gApoR3X0Y6C6ACzTf1y7JOcsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-07-28  5:02                                   ` Steve French
     [not found]                                     ` <CAH2r5mtiZNDyeRe_rYy4Pcg1WhbGaZtdweM=p8fG1uc0xZcAeg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-07-28  8:28                                       ` Aurélien Aptel
2016-07-29 13:11         ` Sachin Prabhu
     [not found]           ` <1469797864.14723.15.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-07-29 13:31             ` Sachin Prabhu
     [not found]               ` <1469799107.14723.18.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2016-07-29 20:20                 ` Steve French

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.