- 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.

NeilBrown

Signed-off-by NeilBrown <[email protected]>

fs/nfsd/vfs.c +4 -1

index 6dd5f19..ec40b36 100644
@@ -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;

Quoted 11 lines · Neil Brown

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?

Quoted 16 lines · On Thu, 22 Apr 2010 12:25:33 -0400 "J.Bruce Fields" <[email protected]>…

Quoted 11 lines · Neil Brown

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

Quoted 8 lines

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 <[email protected]>
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 :-)

Quoted 1 line · Neil Brown

On Thu, 22 Apr 2010 12:25:33 -0400

Quoted 43 lines

Quoted 16 lines · J.Bruce Fields

Quoted 11 lines · Neil Brown

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

Quoted 8 lines

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 <[email protected]>
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.

Quoted 2 lines

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.

Quoted 2 lines · J.Bruce Fields

Quoted 1 line · Neil Brown

On Thu, 22 Apr 2010 12:25:33 -0400

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

Quoted 47 lines

Quoted 46 lines

Quoted 16 lines · J.Bruce Fields

Quoted 11 lines · Neil Brown

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

Quoted 8 lines

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 <[email protected]>
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 :-)

Quoted 3 lines

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 <[email protected]>
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 [email protected]

Signed-off-by NeilBrown <[email protected]>

[bfields: use OWNER_OVERRIDE and update for 4.1]

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs4proc.c +5 -3

index 85a6915..beaa99f 100644
@@ -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:

fs/nfsd/vfs.c +8 -2

index b584205..0ef9b6b 100644
@@ -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;