From: Jeff Layton <jlayton@redhat.com> Date: Fri, 18 Sep 2009 07:17:48 -0400 Subject: [nfs] knfsd: fix NFSv4 O_EXCL creates Message-id: 1253272668-18713-1-git-send-email-jlayton@redhat.com O-Subject: [RHEL5.5 PATCH] BZ#522163: knfsd: fix NFSv4 O_EXCL creates Bugzilla: 524521 RH-Acked-by: Steve Dickson <SteveD@redhat.com> CVE: CVE-2009-3286 (Posting this as a 5.5 patch, but it may also be a z-stream candidate) This patch fixes a regression that appeared in the 5.4 kernel with O_EXCL creates over NFSv4. In 5.4 we added a patch that made it so that when the owner portion of the permission bits are cleared, then the server will deny access to the file. This exposed some existing bugs in the NFSv4 server code. When a file is created with O_EXCL permissions, the kernel first tries to create the file with all of the mode bits cleared, and then has the client set the attribute fields that hold the verifier. The existing server code does a redundant open permission check after creating the file and that usually fails now. That causes the operation to return an error and the file is left sitting on the server in the state it was in just after the vfs_create returned. It would always fail for an unprivileged user but for another bug...the verfier for exclusive creates is stored as a union with the file attributes for non-exclusive creates. When the server does the actual vfs_create of the file it fetches the ia_mode field from the attributes portion of the union. Unfortunately, it does this even for exclusive creates. This means that it "casts" a portion of the verifier as the mode used for the create. These modes are often non-sensical, but due to the first bug the files will end up staying that way after the server returns an error on the create back to the client. In addition to being a bug, this may also be a marginal security issue. Given enough attempts, it's not hard to imagine a lingering file from a failed create that is world-writeable but only setuid execute as the user who is attempting these creates. Fortunately, root is not susceptible to this bug so a setuid root file shouldn't be possible. It might be possible to exploit this to gain access as another user though. The following patch is a straight backport of two patches from upstream to remove the redundant open permissions check when a file is created. It also adds a delta to separate the verifer and attribute union into separate fields in order to ensure to make sure that files are created with sane modes. I cherry picked that delta from a recent NFSv4.1 patch that made this change upstream. I believe that inadvertantly fixed this bug there. Upstream commit ID's: af85852de0b32d92b14295aa6f5ba3a9ad044cf6 81ac95c5569d7a60ab5db6c1ccec56c12b3ebcb5 79fb54abd285b442e1f30f851902f3ddf58e7704 Signed-off-by: Jeff Layton <jlayton@redhat.com> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c index 7d47c16..ebb0464 100644 --- a/fs/nfsd/nfs3proc.c +++ b/fs/nfsd/nfs3proc.c @@ -256,7 +256,7 @@ nfsd3_proc_create(struct svc_rqst *rqstp, struct nfsd3_createargs *argp, /* Now create the file and set attributes */ nfserr = nfsd_create_v3(rqstp, dirfhp, argp->name, argp->len, attr, newfhp, - argp->createmode, argp->verf, NULL); + argp->createmode, argp->verf, NULL, NULL); RETURN_STATUS(nfserr); } diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 3740220..0d4e647 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -95,6 +95,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o { struct svc_fh resfh; int status; + int created = 0; fh_init(&resfh, NFS4_FHSIZE); open->op_truncate = 0; @@ -108,7 +109,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o open->op_fname.len, &open->op_iattr, &resfh, open->op_createmode, (u32 *)open->op_verf.data, - &open->op_truncate); + &open->op_truncate, &created); /* If we ever decide to use different attrs to store the * verifier in nfsd_create_v3, then we'll need to change this @@ -121,21 +122,21 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o open->op_fname.data, open->op_fname.len, &resfh); fh_unlock(current_fh); } + if (status) + goto out; - if (!status) { - set_change_info(&open->op_cinfo, current_fh); + set_change_info(&open->op_cinfo, current_fh); - /* set reply cache */ - fh_dup2(current_fh, &resfh); - open->op_stateowner->so_replay.rp_openfh_len = - resfh.fh_handle.fh_size; - memcpy(open->op_stateowner->so_replay.rp_openfh, - &resfh.fh_handle.fh_base, - resfh.fh_handle.fh_size); + /* set reply cache */ + fh_dup2(current_fh, &resfh); + open->op_stateowner->so_replay.rp_openfh_len = resfh.fh_handle.fh_size; + memcpy(open->op_stateowner->so_replay.rp_openfh, + &resfh.fh_handle.fh_base, resfh.fh_handle.fh_size); + if (!created) status = do_open_permission(rqstp, current_fh, open, MAY_NOP); - } +out: fh_put(&resfh); return status; } diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 5290248..773b280 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1241,7 +1241,7 @@ int nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp, char *fname, int flen, struct iattr *iap, struct svc_fh *resfhp, int createmode, u32 *verifier, - int *truncp) + int *truncp, int *created) { struct dentry *dentry, *dchild = NULL; struct inode *dirp; @@ -1330,6 +1330,8 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp, err = vfs_create(dirp, dchild, iap->ia_mode, NULL); if (err < 0) goto out_nfserr; + if (created) + *created = 1; if (EX_ISSYNC(fhp->fh_export)) { err = nfserrno(nfsd_sync_dir(dentry)); diff --git a/include/linux/nfsd/nfsd.h b/include/linux/nfsd/nfsd.h index fb65b20..c04fb25 100644 --- a/include/linux/nfsd/nfsd.h +++ b/include/linux/nfsd/nfsd.h @@ -92,7 +92,7 @@ int nfsd_access(struct svc_rqst *, struct svc_fh *, u32 *, u32 *); int nfsd_create_v3(struct svc_rqst *, struct svc_fh *, char *name, int len, struct iattr *attrs, struct svc_fh *res, int createmode, - u32 *verifier, int *truncp); + u32 *verifier, int *truncp, int *created); int nfsd_commit(struct svc_rqst *, struct svc_fh *, loff_t, unsigned long); #endif /* CONFIG_NFSD_V3 */ diff --git a/include/linux/nfsd/xdr4.h b/include/linux/nfsd/xdr4.h index f894f04..0426106 100644 --- a/include/linux/nfsd/xdr4.h +++ b/include/linux/nfsd/xdr4.h @@ -201,10 +201,8 @@ struct nfsd4_open { u32 op_create; /* request */ u32 op_createmode; /* request */ u32 op_bmval[2]; /* request */ - union { /* request */ - struct iattr iattr; /* UNCHECKED4,GUARDED4 */ - nfs4_verifier verf; /* EXCLUSIVE4 */ - } u; + struct iattr iattr; /* UNCHECKED4,GUARDED4 */ + nfs4_verifier verf; /* EXCLUSIVE4 */ clientid_t op_clientid; /* request */ struct xdr_netobj op_owner; /* request */ u32 op_seqid; /* request */ @@ -218,8 +216,8 @@ struct nfsd4_open { struct nfs4_stateowner *op_stateowner; /* used during processing */ struct nfs4_acl *op_acl; }; -#define op_iattr u.iattr -#define op_verf u.verf +#define op_iattr iattr +#define op_verf verf struct nfsd4_open_confirm { stateid_t oc_req_stateid /* request */;