Re: [PATCH] - avoid permission checks on EXCLUSIVE_CREATE replay

Re: [PATCH] - avoid permission checks on EXCLUSIVE_CREATE replay

[J.Bruce Fields]

On Thu, Apr 22, 2010 at 10:10:42AM +1000, Neil Brown wrote:
> 
> With NFSv4, if we create a file then open it we explicit avoid checking the
> permissions on the file during the open because the fact that we created it
> ensures we should be allow to open it (the create and the open should appear
> to be a single operation).
> 
> However if the reply to an EXCLUSIVE create gets lots and the client resends
> the create, the current code will perform the permission check - because it
> doesn't realise that it did the open already..
> 
> This patch should fix this.

Thanks, but: hm, does this leave a loophole for a clever attacker?
They'll still have to get past the initial

	fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE)

but that just checks the parent directory; if the existing file is
actually owned by someone else, do we allow an open that we shouldn't?

Maybe when "created" is set we should keep the permission check but add
NFSD_ALLOW_OWNER_OVERRIDE?

--b.

> 
> Note that I haven't actually seen this cause a problem.  I was just looking
> at the code trying to figure out a different EXCLUSIVE open related issue,
> and this looked wrong.
> 
> NeilBrown
> 
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 6dd5f19..ec40b36 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1451,8 +1451,11 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		case NFS3_CREATE_EXCLUSIVE:
>  			if (   dchild->d_inode->i_mtime.tv_sec == v_mtime
>  			    && dchild->d_inode->i_atime.tv_sec == v_atime
> -			    && dchild->d_inode->i_size  == 0 )
> +			    && dchild->d_inode->i_size  == 0 ) {
> +				if (created)
> +					*created = 1;
>  				break;
> +			}
>  			 /* fallthru */
>  		case NFS3_CREATE_GUARDED:
>  			err = nfserr_exist;

Re: [PATCH] - avoid permission checks on EXCLUSIVE_CREATE replay

[Neil Brown]

On Thu, 22 Apr 2010 12:25:33 -0400
"J.Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Apr 22, 2010 at 10:10:42AM +1000, Neil Brown wrote:
> > 
> > With NFSv4, if we create a file then open it we explicit avoid checking the
> > permissions on the file during the open because the fact that we created it
> > ensures we should be allow to open it (the create and the open should appear
> > to be a single operation).
> > 
> > However if the reply to an EXCLUSIVE create gets lots and the client resends
> > the create, the current code will perform the permission check - because it
> > doesn't realise that it did the open already..
> > 
> > This patch should fix this.
> 
> Thanks, but: hm, does this leave a loophole for a clever attacker?
> They'll still have to get past the initial
> 
> 	fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE)
> 
> but that just checks the parent directory; if the existing file is
> actually owned by someone else, do we allow an open that we shouldn't?
> 
> Maybe when "created" is set we should keep the permission check but add
> NFSD_ALLOW_OWNER_OVERRIDE?
> 

I think that is possibly a good idea.  However......

commit 81ac95c5569d7a60ab5db6c1ccec56c12b3ebcb5
Author: J. Bruce Fields <bfields@fieldses.org>
Date:   Wed Nov 8 17:44:40 2006 -0800

    [PATCH] nfsd4: fix open-create permissions
    
    In the case where an open creates the file, we shouldn't be rechecking
    permissions to open the file; the open succeeds regardless of what the new
    file's mode bits say.
    
    This patch fixes the problem, but only by introducing yet another parameter
    to nfsd_create_v3.  This is ugly.  This will be fixed by later patches.
    

I wouldn't want to get in the way of these 'later patches' that might be
going to remove the 'created' flag from nfsd_create_v3 :-)

NeilBrown

Re: [PATCH] - avoid permission checks on EXCLUSIVE_CREATE replay

[J.Bruce Fields]

On Fri, Apr 23, 2010 at 07:16:31AM +1000, Neil Brown wrote:
> On Thu, 22 Apr 2010 12:25:33 -0400
> "J.Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Thu, Apr 22, 2010 at 10:10:42AM +1000, Neil Brown wrote:
> > > 
> > > With NFSv4, if we create a file then open it we explicit avoid checking the
> > > permissions on the file during the open because the fact that we created it
> > > ensures we should be allow to open it (the create and the open should appear
> > > to be a single operation).
> > > 
> > > However if the reply to an EXCLUSIVE create gets lots and the client resends
> > > the create, the current code will perform the permission check - because it
> > > doesn't realise that it did the open already..
> > > 
> > > This patch should fix this.
> > 
> > Thanks, but: hm, does this leave a loophole for a clever attacker?
> > They'll still have to get past the initial
> > 
> > 	fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE)
> > 
> > but that just checks the parent directory; if the existing file is
> > actually owned by someone else, do we allow an open that we shouldn't?
> > 
> > Maybe when "created" is set we should keep the permission check but add
> > NFSD_ALLOW_OWNER_OVERRIDE?
> > 
> 
> I think that is possibly a good idea.  However......
> 
> commit 81ac95c5569d7a60ab5db6c1ccec56c12b3ebcb5
> Author: J. Bruce Fields <bfields@fieldses.org>
> Date:   Wed Nov 8 17:44:40 2006 -0800
> 
>     [PATCH] nfsd4: fix open-create permissions
>     
>     In the case where an open creates the file, we shouldn't be rechecking
>     permissions to open the file; the open succeeds regardless of what the new
>     file's mode bits say.
>     
>     This patch fixes the problem, but only by introducing yet another parameter
>     to nfsd_create_v3.  This is ugly.  This will be fixed by later patches.
>     
> 
> I wouldn't want to get in the way of these 'later patches' that might be
> going to remove the 'created' flag from nfsd_create_v3 :-)

Har.  I was optimistic.

That code *is* really hairy.  I'll take another look.

--b.

Re: [PATCH] - avoid permission checks on EXCLUSIVE_CREATE replay

[J.Bruce Fields]

On Thu, Apr 22, 2010 at 05:18:01PM -0400, J.Bruce Fields wrote:
> On Fri, Apr 23, 2010 at 07:16:31AM +1000, Neil Brown wrote:
> > On Thu, 22 Apr 2010 12:25:33 -0400

Always fun to reply on 2-year-old threads:

> > "J.Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Thu, Apr 22, 2010 at 10:10:42AM +1000, Neil Brown wrote:
> > > > 
> > > > With NFSv4, if we create a file then open it we explicit avoid checking the
> > > > permissions on the file during the open because the fact that we created it
> > > > ensures we should be allow to open it (the create and the open should appear
> > > > to be a single operation).
> > > > 
> > > > However if the reply to an EXCLUSIVE create gets lots and the client resends
> > > > the create, the current code will perform the permission check - because it
> > > > doesn't realise that it did the open already..
> > > > 
> > > > This patch should fix this.
> > > 
> > > Thanks, but: hm, does this leave a loophole for a clever attacker?
> > > They'll still have to get past the initial
> > > 
> > > 	fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE)
> > > 
> > > but that just checks the parent directory; if the existing file is
> > > actually owned by someone else, do we allow an open that we shouldn't?
> > > 
> > > Maybe when "created" is set we should keep the permission check but add
> > > NFSD_ALLOW_OWNER_OVERRIDE?
> > > 
> > 
> > I think that is possibly a good idea.  However......
> > 
> > commit 81ac95c5569d7a60ab5db6c1ccec56c12b3ebcb5
> > Author: J. Bruce Fields <bfields@fieldses.org>
> > Date:   Wed Nov 8 17:44:40 2006 -0800
> > 
> >     [PATCH] nfsd4: fix open-create permissions
> >     
> >     In the case where an open creates the file, we shouldn't be rechecking
> >     permissions to open the file; the open succeeds regardless of what the new
> >     file's mode bits say.
> >     
> >     This patch fixes the problem, but only by introducing yet another parameter
> >     to nfsd_create_v3.  This is ugly.  This will be fixed by later patches.
> >     
> > 
> > I wouldn't want to get in the way of these 'later patches' that might be
> > going to remove the 'created' flag from nfsd_create_v3 :-)
> 
> Har.  I was optimistic.
> 
> That code *is* really hairy.  I'll take another look.

Well, it remains as ugly as ever, but pynfs just reminded me that I'd
forgotten that the original bug still needed fixing!

Version I plan to merge follows.

commit 1895f5075c59f6a7b5e4c3a59ffe108e881f69d4
Author: Neil Brown <neilb@suse.de>
Date:   Fri Dec 7 15:40:55 2012 -0500

    nfsd: avoid permission checks on EXCLUSIVE_CREATE replay
    
    With NFSv4, if we create a file then open it we explicit avoid checking
    the permissions on the file during the open because the fact that we
    created it ensures we should be allow to open it (the create and the
    open should appear to be a single operation).
    
    However if the reply to an EXCLUSIVE create gets lots and the client
    resends the create, the current code will perform the permission check -
    because it doesn't realise that it did the open already..
    
    This patch should fix this.
    
    Note that I haven't actually seen this cause a problem.  I was just
    looking at the code trying to figure out a different EXCLUSIVE open
    related issue, and this looked wrong.
    
    Cc: stable@kernel.org
    Signed-off-by: NeilBrown <neilb@suse.de>
    [bfields: use OWNER_OVERRIDE and update for 4.1]
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 85a6915..beaa99f 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -195,6 +195,7 @@ static __be32
 do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_open *open)
 {
 	struct svc_fh *resfh;
+	int accmode;
 	__be32 status;
 
 	resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
@@ -254,9 +255,10 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
 	/* set reply cache */
 	fh_copy_shallow(&open->op_openowner->oo_owner.so_replay.rp_openfh,
 			&resfh->fh_handle);
-	if (!open->op_created)
-		status = do_open_permission(rqstp, resfh, open,
-					    NFSD_MAY_NOP);
+	accmode = NFSD_MAY_NOP;
+	if (open->op_created)
+		accmode |= NFSD_MAY_OWNER_OVERRIDE;
+	status = do_open_permission(rqstp, resfh, open, accmode);
 	set_change_info(&open->op_cinfo, current_fh);
 	fh_dup2(current_fh, resfh);
 out:
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index b584205..0ef9b6b 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1471,13 +1471,19 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		case NFS3_CREATE_EXCLUSIVE:
 			if (   dchild->d_inode->i_mtime.tv_sec == v_mtime
 			    && dchild->d_inode->i_atime.tv_sec == v_atime
-			    && dchild->d_inode->i_size  == 0 )
+			    && dchild->d_inode->i_size  == 0 ) {
+				if (created)
+					*created = 1;
 				break;
+			}
 		case NFS4_CREATE_EXCLUSIVE4_1:
 			if (   dchild->d_inode->i_mtime.tv_sec == v_mtime
 			    && dchild->d_inode->i_atime.tv_sec == v_atime
-			    && dchild->d_inode->i_size  == 0 )
+			    && dchild->d_inode->i_size  == 0 ) {
+				if (created)
+					*created = 1;
 				goto set_attr;
+			}
 			 /* fallthru */
 		case NFS3_CREATE_GUARDED:
 			err = nfserr_exist;

Re: [PATCH] - avoid permission checks on EXCLUSIVE_CREATE replay

[NeilBrown]

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

On Fri, 7 Dec 2012 17:50:36 -0500 "J.Bruce Fields" <bfields@fieldses.org>
wrote:

> On Thu, Apr 22, 2010 at 05:18:01PM -0400, J.Bruce Fields wrote:
> > On Fri, Apr 23, 2010 at 07:16:31AM +1000, Neil Brown wrote:
> > > On Thu, 22 Apr 2010 12:25:33 -0400
>
> Always fun to reply on 2-year-old threads:

So *that* is why I don't delete old email.  It means that when I get
replies two years later, they get added to the right thread and I have all
the right context.  Now I can stop feeling guilty about it - thanks.

> 
> > > "J.Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Thu, Apr 22, 2010 at 10:10:42AM +1000, Neil Brown wrote:
> > > > > 
> > > > > With NFSv4, if we create a file then open it we explicit avoid checking the
> > > > > permissions on the file during the open because the fact that we created it
> > > > > ensures we should be allow to open it (the create and the open should appear
> > > > > to be a single operation).
> > > > > 
> > > > > However if the reply to an EXCLUSIVE create gets lots and the client resends
> > > > > the create, the current code will perform the permission check - because it
> > > > > doesn't realise that it did the open already..
> > > > > 
> > > > > This patch should fix this.
> > > > 
> > > > Thanks, but: hm, does this leave a loophole for a clever attacker?
> > > > They'll still have to get past the initial
> > > > 
> > > > 	fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE)
> > > > 
> > > > but that just checks the parent directory; if the existing file is
> > > > actually owned by someone else, do we allow an open that we shouldn't?
> > > > 
> > > > Maybe when "created" is set we should keep the permission check but add
> > > > NFSD_ALLOW_OWNER_OVERRIDE?
> > > > 
> > > 
> > > I think that is possibly a good idea.  However......
> > > 
> > > commit 81ac95c5569d7a60ab5db6c1ccec56c12b3ebcb5
> > > Author: J. Bruce Fields <bfields@fieldses.org>
> > > Date:   Wed Nov 8 17:44:40 2006 -0800
> > > 
> > >     [PATCH] nfsd4: fix open-create permissions
> > >     
> > >     In the case where an open creates the file, we shouldn't be rechecking
> > >     permissions to open the file; the open succeeds regardless of what the new
> > >     file's mode bits say.
> > >     
> > >     This patch fixes the problem, but only by introducing yet another parameter
> > >     to nfsd_create_v3.  This is ugly.  This will be fixed by later patches.
> > >     
> > > 
> > > I wouldn't want to get in the way of these 'later patches' that might be
> > > going to remove the 'created' flag from nfsd_create_v3 :-)
> > 
> > Har.  I was optimistic.
> > 
> > That code *is* really hairy.  I'll take another look.
> 
> Well, it remains as ugly as ever, but pynfs just reminded me that I'd
> forgotten that the original bug still needed fixing!
> 
> Version I plan to merge follows.


Patch looks good to me.
Thanks!

NeilBrown





> 
> commit 1895f5075c59f6a7b5e4c3a59ffe108e881f69d4
> Author: Neil Brown <neilb@suse.de>
> Date:   Fri Dec 7 15:40:55 2012 -0500
> 
>     nfsd: avoid permission checks on EXCLUSIVE_CREATE replay
>     
>     With NFSv4, if we create a file then open it we explicit avoid checking
>     the permissions on the file during the open because the fact that we
>     created it ensures we should be allow to open it (the create and the
>     open should appear to be a single operation).
>     
>     However if the reply to an EXCLUSIVE create gets lots and the client
>     resends the create, the current code will perform the permission check -
>     because it doesn't realise that it did the open already..
>     
>     This patch should fix this.
>     
>     Note that I haven't actually seen this cause a problem.  I was just
>     looking at the code trying to figure out a different EXCLUSIVE open
>     related issue, and this looked wrong.
>     
>     Cc: stable@kernel.org
>     Signed-off-by: NeilBrown <neilb@suse.de>
>     [bfields: use OWNER_OVERRIDE and update for 4.1]
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 85a6915..beaa99f 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -195,6 +195,7 @@ static __be32
>  do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_open *open)
>  {
>  	struct svc_fh *resfh;
> +	int accmode;
>  	__be32 status;
>  
>  	resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
> @@ -254,9 +255,10 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
>  	/* set reply cache */
>  	fh_copy_shallow(&open->op_openowner->oo_owner.so_replay.rp_openfh,
>  			&resfh->fh_handle);
> -	if (!open->op_created)
> -		status = do_open_permission(rqstp, resfh, open,
> -					    NFSD_MAY_NOP);
> +	accmode = NFSD_MAY_NOP;
> +	if (open->op_created)
> +		accmode |= NFSD_MAY_OWNER_OVERRIDE;
> +	status = do_open_permission(rqstp, resfh, open, accmode);
>  	set_change_info(&open->op_cinfo, current_fh);
>  	fh_dup2(current_fh, resfh);
>  out:
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index b584205..0ef9b6b 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1471,13 +1471,19 @@ do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		case NFS3_CREATE_EXCLUSIVE:
>  			if (   dchild->d_inode->i_mtime.tv_sec == v_mtime
>  			    && dchild->d_inode->i_atime.tv_sec == v_atime
> -			    && dchild->d_inode->i_size  == 0 )
> +			    && dchild->d_inode->i_size  == 0 ) {
> +				if (created)
> +					*created = 1;
>  				break;
> +			}
>  		case NFS4_CREATE_EXCLUSIVE4_1:
>  			if (   dchild->d_inode->i_mtime.tv_sec == v_mtime
>  			    && dchild->d_inode->i_atime.tv_sec == v_atime
> -			    && dchild->d_inode->i_size  == 0 )
> +			    && dchild->d_inode->i_size  == 0 ) {
> +				if (created)
> +					*created = 1;
>  				goto set_attr;
> +			}
>  			 /* fallthru */
>  		case NFS3_CREATE_GUARDED:
>  			err = nfserr_exist;


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]