nfs_open_permission_mask() shouldn't check MAY_READ for suid/sgid files that
are opened with __FMODE_EXEC.
Also fix NFSv4 access-in-open path in a similar way -- openflags must be
used because fmode will not always have FMODE_EXEC set.
Signed-off-by: Weston Andros Adamson <[email protected]>
---
This patch fixes https://bugzilla.kernel.org/show_bug.cgi?id=49101
The fix is not as obvious or clean as I'd like, so here is a little background:
Not checking MAY_READ when a S_ISUID or S_ISGID file has MAY_EXEC causes
suid/sgid executables to behave the same as on a local FS.
The second part is fixing open-in-access for NFSv4 - we want to do the same
thing as in nfs_open_permission_mask, but oddly enough fmode doesn't always
indicate that the file is being opened for execution. Instead we have to
use the openflags for this.
Adding some debug prints show that the first exec of a suid/sgid
file will open with fmode containing FMODE_EXEC and FMODE_READ, but subsequent
execs will open with fmode containing FMODE_READ, but not FMODE_EXEC.
openflags always has __FMODE_EXEC set in these examples.
The first exec has these values in nfs4_opendata_access():
fmode = 0x21: contains read exec
openflags = 0x8020: contains exec
The second (and subsequent) execs have these values in nfs4_opendata_access():
fmode = 0x1: contains read
openflags = 0x8020: contains exec
-dros
fs/nfs/dir.c | 12 +++++++++---
fs/nfs/nfs4proc.c | 13 +++++++++++--
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 32e6c53..5141243 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2149,7 +2149,7 @@ out:
return -EACCES;
}
-static int nfs_open_permission_mask(int openflags)
+static int nfs_open_permission_mask(struct inode *inode, int openflags)
{
int mask = 0;
@@ -2157,14 +2157,20 @@ static int nfs_open_permission_mask(int openflags)
mask |= MAY_READ;
if ((openflags & O_ACCMODE) != O_RDONLY)
mask |= MAY_WRITE;
- if (openflags & __FMODE_EXEC)
+ if (openflags & __FMODE_EXEC) {
mask |= MAY_EXEC;
+ /* don't check MAY_READ for exec of suid/sgid */
+ if (inode->i_mode & (S_ISUID | S_ISGID))
+ mask &= ~MAY_READ;
+ }
+
return mask;
}
int nfs_may_open(struct inode *inode, struct rpc_cred *cred, int openflags)
{
- return nfs_do_access(inode, cred, nfs_open_permission_mask(openflags));
+ return nfs_do_access(inode, cred,
+ nfs_open_permission_mask(inode, openflags));
}
EXPORT_SYMBOL_GPL(nfs_may_open);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5d864fb..23cf1a8 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1626,7 +1626,8 @@ static int _nfs4_recover_proc_open(struct nfs4_opendata *data)
static int nfs4_opendata_access(struct rpc_cred *cred,
struct nfs4_opendata *opendata,
- struct nfs4_state *state, fmode_t fmode)
+ struct nfs4_state *state, fmode_t fmode,
+ int openflags)
{
struct nfs_access_entry cache;
u32 mask;
@@ -1644,6 +1645,14 @@ static int nfs4_opendata_access(struct rpc_cred *cred,
if (fmode & FMODE_EXEC)
mask |= MAY_EXEC;
+ /* use openflags to check for exec of suid/sgid, because fmode won't
+ * always have FMODE_EXEC set by VFS.
+ * Also, don't check MAY_READ on a suid/sgid file being executed */
+ if ((openflags & __FMODE_EXEC) &&
+ (state->inode->i_mode & (S_ISUID | S_ISGID))) {
+ mask = MAY_EXEC;
+ }
+
cache.cred = cred;
cache.jiffies = jiffies;
nfs_access_set_mask(&cache, opendata->o_res.access_result);
@@ -1896,7 +1905,7 @@ static int _nfs4_do_open(struct inode *dir,
if (server->caps & NFS_CAP_POSIX_LOCK)
set_bit(NFS_STATE_POSIX_LOCKS, &state->flags);
- status = nfs4_opendata_access(cred, opendata, state, fmode);
+ status = nfs4_opendata_access(cred, opendata, state, fmode, flags);
if (status != 0)
goto err_opendata_put;
--
1.7.9.6 (Apple Git-31.1)
On Thu, 2013-01-03 at 15:33 -0500, Weston Andros Adamson wrote:
+AD4- nfs+AF8-open+AF8-permission+AF8-mask() shouldn't check MAY+AF8-READ for suid/sgid files that
+AD4- are opened with +AF8AXw-FMODE+AF8-EXEC.
+AD4-
+AD4- Also fix NFSv4 access-in-open path in a similar way -- openflags must be
+AD4- used because fmode will not always have FMODE+AF8-EXEC set.
+AD4-
+AD4- Signed-off-by: Weston Andros Adamson +ADw-dros+AEA-netapp.com+AD4-
+AD4- ---
+AD4-
+AD4- This patch fixes https://bugzilla.kernel.org/show+AF8-bug.cgi?id+AD0-49101
+AD4-
+AD4- The fix is not as obvious or clean as I'd like, so here is a little background:
+AD4-
+AD4- Not checking MAY+AF8-READ when a S+AF8-ISUID or S+AF8-ISGID file has MAY+AF8-EXEC causes
+AD4- suid/sgid executables to behave the same as on a local FS.
+AD4-
+AD4- The second part is fixing open-in-access for NFSv4 - we want to do the same
+AD4- thing as in nfs+AF8-open+AF8-permission+AF8-mask, but oddly enough fmode doesn't always
+AD4- indicate that the file is being opened for execution. Instead we have to
+AD4- use the openflags for this.
+AD4-
+AD4- Adding some debug prints show that the first exec of a suid/sgid
+AD4- file will open with fmode containing FMODE+AF8-EXEC and FMODE+AF8-READ, but subsequent
+AD4- execs will open with fmode containing FMODE+AF8-READ, but not FMODE+AF8-EXEC.
+AD4- openflags always has +AF8AXw-FMODE+AF8-EXEC set in these examples.
+AD4-
+AD4- The first exec has these values in nfs4+AF8-opendata+AF8-access():
+AD4-
+AD4- fmode +AD0- 0x21: contains read exec
+AD4- openflags +AD0- 0x8020: contains exec
+AD4-
+AD4- The second (and subsequent) execs have these values in nfs4+AF8-opendata+AF8-access():
+AD4-
+AD4- fmode +AD0- 0x1: contains read
+AD4- openflags +AD0- 0x8020: contains exec
+AD4-
+AD4- -dros
+AD4-
+AD4- fs/nfs/dir.c +AHw- 12 +-+-+-+-+-+-+-+-+----
+AD4- fs/nfs/nfs4proc.c +AHw- 13 +-+-+-+-+-+-+-+-+-+-+---
+AD4- 2 files changed, 20 insertions(+-), 5 deletions(-)
+AD4-
+AD4- diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
+AD4- index 32e6c53..5141243 100644
+AD4- --- a/fs/nfs/dir.c
+AD4- +-+-+- b/fs/nfs/dir.c
+AD4- +AEAAQA- -2149,7 +-2149,7 +AEAAQA- out:
+AD4- return -EACCES+ADs-
+AD4- +AH0-
+AD4-
+AD4- -static int nfs+AF8-open+AF8-permission+AF8-mask(int openflags)
+AD4- +-static int nfs+AF8-open+AF8-permission+AF8-mask(struct inode +ACo-inode, int openflags)
+AD4- +AHs-
+AD4- int mask +AD0- 0+ADs-
+AD4-
+AD4- +AEAAQA- -2157,14 +-2157,20 +AEAAQA- static int nfs+AF8-open+AF8-permission+AF8-mask(int openflags)
+AD4- mask +AHwAPQ- MAY+AF8-READ+ADs-
+AD4- if ((openflags +ACY- O+AF8-ACCMODE) +ACEAPQ- O+AF8-RDONLY)
+AD4- mask +AHwAPQ- MAY+AF8-WRITE+ADs-
+AD4- - if (openflags +ACY- +AF8AXw-FMODE+AF8-EXEC)
+AD4- +- if (openflags +ACY- +AF8AXw-FMODE+AF8-EXEC) +AHs-
+AD4- mask +AHwAPQ- MAY+AF8-EXEC+ADs-
+AD4- +- /+ACo- don't check MAY+AF8-READ for exec of suid/sgid +ACo-/
+AD4- +- if (inode-+AD4-i+AF8-mode +ACY- (S+AF8-ISUID +AHw- S+AF8-ISGID))
+AD4- +- mask +ACYAPQ- +AH4-MAY+AF8-READ+ADs-
Wait, this can't be right. Why does the presence of a suid/sgid flag
make a difference here? Either way, if +AF8AXw-FMODE+AF8-EXEC access is requested,
then we should only need MAY+AF8-EXEC rights.
+AD4- +- +AH0-
+AD4- +-
+AD4- return mask+ADs-
+AD4- +AH0-
+AD4-
+AD4- int nfs+AF8-may+AF8-open(struct inode +ACo-inode, struct rpc+AF8-cred +ACo-cred, int openflags)
+AD4- +AHs-
+AD4- - return nfs+AF8-do+AF8-access(inode, cred, nfs+AF8-open+AF8-permission+AF8-mask(openflags))+ADs-
+AD4- +- return nfs+AF8-do+AF8-access(inode, cred,
+AD4- +- nfs+AF8-open+AF8-permission+AF8-mask(inode, openflags))+ADs-
+AD4- +AH0-
+AD4- EXPORT+AF8-SYMBOL+AF8-GPL(nfs+AF8-may+AF8-open)+ADs-
+AD4-
+AD4- diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
+AD4- index 5d864fb..23cf1a8 100644
+AD4- --- a/fs/nfs/nfs4proc.c
+AD4- +-+-+- b/fs/nfs/nfs4proc.c
+AD4- +AEAAQA- -1626,7 +-1626,8 +AEAAQA- static int +AF8-nfs4+AF8-recover+AF8-proc+AF8-open(struct nfs4+AF8-opendata +ACo-data)
+AD4-
+AD4- static int nfs4+AF8-opendata+AF8-access(struct rpc+AF8-cred +ACo-cred,
+AD4- struct nfs4+AF8-opendata +ACo-opendata,
+AD4- - struct nfs4+AF8-state +ACo-state, fmode+AF8-t fmode)
+AD4- +- struct nfs4+AF8-state +ACo-state, fmode+AF8-t fmode,
+AD4- +- int openflags)
+AD4- +AHs-
+AD4- struct nfs+AF8-access+AF8-entry cache+ADs-
+AD4- u32 mask+ADs-
+AD4- +AEAAQA- -1644,6 +-1645,14 +AEAAQA- static int nfs4+AF8-opendata+AF8-access(struct rpc+AF8-cred +ACo-cred,
+AD4- if (fmode +ACY- FMODE+AF8-EXEC)
+AD4- mask +AHwAPQ- MAY+AF8-EXEC+ADs-
+AD4-
+AD4- +- /+ACo- use openflags to check for exec of suid/sgid, because fmode won't
+AD4- +- +ACo- always have FMODE+AF8-EXEC set by VFS.
+AD4- +- +ACo- Also, don't check MAY+AF8-READ on a suid/sgid file being executed +ACo-/
+AD4- +- if ((openflags +ACY- +AF8AXw-FMODE+AF8-EXEC) +ACYAJg-
+AD4- +- (state-+AD4-inode-+AD4-i+AF8-mode +ACY- (S+AF8-ISUID +AHw- S+AF8-ISGID))) +AHs-
+AD4- +- mask +AD0- MAY+AF8-EXEC+ADs-
+AD4- +- +AH0-
Ditto...
+AD4- +-
+AD4- cache.cred +AD0- cred+ADs-
+AD4- cache.jiffies +AD0- jiffies+ADs-
+AD4- nfs+AF8-access+AF8-set+AF8-mask(+ACY-cache, opendata-+AD4-o+AF8-res.access+AF8-result)+ADs-
+AD4- +AEAAQA- -1896,7 +-1905,7 +AEAAQA- static int +AF8-nfs4+AF8-do+AF8-open(struct inode +ACo-dir,
+AD4- if (server-+AD4-caps +ACY- NFS+AF8-CAP+AF8-POSIX+AF8-LOCK)
+AD4- set+AF8-bit(NFS+AF8-STATE+AF8-POSIX+AF8-LOCKS, +ACY-state-+AD4-flags)+ADs-
+AD4-
+AD4- - status +AD0- nfs4+AF8-opendata+AF8-access(cred, opendata, state, fmode)+ADs-
+AD4- +- status +AD0- nfs4+AF8-opendata+AF8-access(cred, opendata, state, fmode, flags)+ADs-
+AD4- if (status +ACEAPQ- 0)
+AD4- goto err+AF8-opendata+AF8-put+ADs-
+AD4-
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Jan 3, 2013, at 3:47 PM, "Myklebust, Trond" <[email protected]> wrote:
> On Thu, 2013-01-03 at 15:33 -0500, Weston Andros Adamson wrote:
>> nfs_open_permission_mask() shouldn't check MAY_READ for suid/sgid files that
>> are opened with __FMODE_EXEC.
>>
>> Also fix NFSv4 access-in-open path in a similar way -- openflags must be
>> used because fmode will not always have FMODE_EXEC set.
>>
>> Signed-off-by: Weston Andros Adamson <[email protected]>
>> ---
>>
>> This patch fixes https://bugzilla.kernel.org/show_bug.cgi?id=49101
>>
>> The fix is not as obvious or clean as I'd like, so here is a little background:
>>
>> Not checking MAY_READ when a S_ISUID or S_ISGID file has MAY_EXEC causes
>> suid/sgid executables to behave the same as on a local FS.
>>
>> The second part is fixing open-in-access for NFSv4 - we want to do the same
>> thing as in nfs_open_permission_mask, but oddly enough fmode doesn't always
>> indicate that the file is being opened for execution. Instead we have to
>> use the openflags for this.
>>
>> Adding some debug prints show that the first exec of a suid/sgid
>> file will open with fmode containing FMODE_EXEC and FMODE_READ, but subsequent
>> execs will open with fmode containing FMODE_READ, but not FMODE_EXEC.
>> openflags always has __FMODE_EXEC set in these examples.
>>
>> The first exec has these values in nfs4_opendata_access():
>>
>> fmode = 0x21: contains read exec
>> openflags = 0x8020: contains exec
>>
>> The second (and subsequent) execs have these values in nfs4_opendata_access():
>>
>> fmode = 0x1: contains read
>> openflags = 0x8020: contains exec
>>
>> -dros
>>
>> fs/nfs/dir.c | 12 +++++++++---
>> fs/nfs/nfs4proc.c | 13 +++++++++++--
>> 2 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>> index 32e6c53..5141243 100644
>> --- a/fs/nfs/dir.c
>> +++ b/fs/nfs/dir.c
>> @@ -2149,7 +2149,7 @@ out:
>> return -EACCES;
>> }
>>
>> -static int nfs_open_permission_mask(int openflags)
>> +static int nfs_open_permission_mask(struct inode *inode, int openflags)
>> {
>> int mask = 0;
>>
>> @@ -2157,14 +2157,20 @@ static int nfs_open_permission_mask(int openflags)
>> mask |= MAY_READ;
>> if ((openflags & O_ACCMODE) != O_RDONLY)
>> mask |= MAY_WRITE;
>> - if (openflags & __FMODE_EXEC)
>> + if (openflags & __FMODE_EXEC) {
>> mask |= MAY_EXEC;
>> + /* don't check MAY_READ for exec of suid/sgid */
>> + if (inode->i_mode & (S_ISUID | S_ISGID))
>> + mask &= ~MAY_READ;
>
> Wait, this can't be right. Why does the presence of a suid/sgid flag
> make a difference here? Either way, if __FMODE_EXEC access is requested,
> then we should only need MAY_EXEC rights.
Great point. Reposting after tests finish.
-dros
>
>> + }
>> +
>> return mask;
>> }
>>
>> int nfs_may_open(struct inode *inode, struct rpc_cred *cred, int openflags)
>> {
>> - return nfs_do_access(inode, cred, nfs_open_permission_mask(openflags));
>> + return nfs_do_access(inode, cred,
>> + nfs_open_permission_mask(inode, openflags));
>> }
>> EXPORT_SYMBOL_GPL(nfs_may_open);
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 5d864fb..23cf1a8 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1626,7 +1626,8 @@ static int _nfs4_recover_proc_open(struct nfs4_opendata *data)
>>
>> static int nfs4_opendata_access(struct rpc_cred *cred,
>> struct nfs4_opendata *opendata,
>> - struct nfs4_state *state, fmode_t fmode)
>> + struct nfs4_state *state, fmode_t fmode,
>> + int openflags)
>> {
>> struct nfs_access_entry cache;
>> u32 mask;
>> @@ -1644,6 +1645,14 @@ static int nfs4_opendata_access(struct rpc_cred *cred,
>> if (fmode & FMODE_EXEC)
>> mask |= MAY_EXEC;
>>
>> + /* use openflags to check for exec of suid/sgid, because fmode won't
>> + * always have FMODE_EXEC set by VFS.
>> + * Also, don't check MAY_READ on a suid/sgid file being executed */
>> + if ((openflags & __FMODE_EXEC) &&
>> + (state->inode->i_mode & (S_ISUID | S_ISGID))) {
>> + mask = MAY_EXEC;
>> + }
>
> Ditto...
>
>> +
>> cache.cred = cred;
>> cache.jiffies = jiffies;
>> nfs_access_set_mask(&cache, opendata->o_res.access_result);
>> @@ -1896,7 +1905,7 @@ static int _nfs4_do_open(struct inode *dir,
>> if (server->caps & NFS_CAP_POSIX_LOCK)
>> set_bit(NFS_STATE_POSIX_LOCKS, &state->flags);
>>
>> - status = nfs4_opendata_access(cred, opendata, state, fmode);
>> + status = nfs4_opendata_access(cred, opendata, state, fmode, flags);
>> if (status != 0)
>> goto err_opendata_put;
>>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com