| Message ID | 1374511328-49579-2-git-send-email-andros@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2013-07-22 at 12:42 -0400, andros@netapp.com wrote: > From: Andy Adamson <andros@netapp.com> > > As per RFC 3530 and RFC 5661 Security Considerations. > > Commit 4edaa308 "NFS: Use "krb5i" to establish NFSv4 state whenever possible" > uses the nfs_client cl_rpcclient for all clientid management operations. Why? From a security perspective, how is this any different from doing a READLINK, for instance? -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
On Wed, 2013-08-07 at 18:01 +0000, Adamson, Andy wrote: > > On Aug 7, 2013, at 12:54 PM, "Myklebust, Trond" > <Trond.Myklebust@netapp.com> > wrote: > > > On Mon, 2013-07-22 at 12:42 -0400, andros@netapp.com wrote: > > > From: Andy Adamson <andros@netapp.com> > > > > > > As per RFC 3530 and RFC 5661 Security Considerations. > > > > > > Commit 4edaa308 "NFS: Use "krb5i" to establish NFSv4 state > > > whenever possible" > > > uses the nfs_client cl_rpcclient for all clientid management > > > operations. > > > > Why? > > > To protect the integrity of the fs_locations server list. > > > From a security perspective, how is this any different from doing a > > READLINK, for instance? > > > fs_locations differs from READLINK in that compromising the > fs_locations attribute server list can result in all of the client > traffic under a junction redirected by an attacker. I repeat: how is this in any way, shape or form different from READLINK? > > Here is the attack as described in 3530bis Security Considerations > section: > > > The second operation that should definitely use integrity protection > is any GETATTR for the fs_locations attribute. The attack has two > steps. First the attacker modifies the unprotected results of some > operation to return NFS4ERR_MOVED. Second, when the client follows > up with a GETATTR for the fs_locations attribute, the attacker > modifies the results to cause the client migrate its traffic to a > server controlled by the attacker. You can the exact same thing by changing the READLINK results. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
T24gV2VkLCAyMDEzLTA4LTA3IGF0IDE0OjA0IC0wNDAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6 DQo+IE9uIFdlZCwgMjAxMy0wOC0wNyBhdCAxODowMSArMDAwMCwgQWRhbXNvbiwgQW5keSB3cm90 ZToNCj4gPiANCj4gPiBIZXJlIGlzIHRoZSBhdHRhY2sgYXMgZGVzY3JpYmVkIGluIDM1MzBiaXMg U2VjdXJpdHkgQ29uc2lkZXJhdGlvbnMNCj4gPiBzZWN0aW9uOg0KPiA+IA0KPiA+IA0KPiA+ICAg IFRoZSBzZWNvbmQgb3BlcmF0aW9uIHRoYXQgc2hvdWxkIGRlZmluaXRlbHkgdXNlIGludGVncml0 eSBwcm90ZWN0aW9uDQo+ID4gICAgaXMgYW55IEdFVEFUVFIgZm9yIHRoZSBmc19sb2NhdGlvbnMg YXR0cmlidXRlLiAgVGhlIGF0dGFjayBoYXMgdHdvDQo+ID4gICAgc3RlcHMuICBGaXJzdCB0aGUg YXR0YWNrZXIgbW9kaWZpZXMgdGhlIHVucHJvdGVjdGVkIHJlc3VsdHMgb2Ygc29tZQ0KPiA+ICAg IG9wZXJhdGlvbiB0byByZXR1cm4gTkZTNEVSUl9NT1ZFRC4gIFNlY29uZCwgd2hlbiB0aGUgY2xp ZW50IGZvbGxvd3MNCj4gPiAgICB1cCB3aXRoIGEgR0VUQVRUUiBmb3IgdGhlIGZzX2xvY2F0aW9u cyBhdHRyaWJ1dGUsIHRoZSBhdHRhY2tlcg0KPiA+ICAgIG1vZGlmaWVzIHRoZSByZXN1bHRzIHRv IGNhdXNlIHRoZSBjbGllbnQgbWlncmF0ZSBpdHMgdHJhZmZpYyB0byBhDQo+ID4gICAgc2VydmVy IGNvbnRyb2xsZWQgYnkgdGhlIGF0dGFja2VyLg0KPiANCj4gWW91IGNhbiB0aGUgZXhhY3Qgc2Ft ZSB0aGluZyBieSBjaGFuZ2luZyB0aGUgUkVBRExJTksgcmVzdWx0cy4NCg0KVGhlIGF0dGFjayBp czogY2hhbmdlIHRoZSB1bnByb3RlY3RlZCBMT09LVVAgcmVzdWx0cyB0byBwb2ludCB0byBhDQpz eW1saW5rLCB0aGVuIGZlZWQgJy9uZXQvPGV2aWwtaXAtYWRkcmVzcz4vbXkvZXZpbC9wYXRobmFt ZScgaW50bw0KUkVBRExJTksuDQoNCk15IHBvaW50IGlzIHRoYXQgaWYgeW91J3JlIG9uIGEgbmV0 d29yayB3aGVyZSB0aGUgYWJvdmUgaXMgYSBwb3RlbnRpYWwNCnRocmVhdCwgdGhlbiB5b3Ugc2hv dWxkIGJlIHVzaW5nIGtyYjVpIG9yLCBiZXR0ZXIgeWV0LCBrcmI1cCBmb3IgX2FsbF8NCm9wZXJh dGlvbnMuIEl0J3Mgbm90IHN1ZmZpY2llbnQgdG8gc2luZ2xlIG91dCBmc19sb2NhdGlvbnMgZm9y IHNwZWNpYWwNCnRyZWF0bWVudC4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBj bGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3 d3cubmV0YXBwLmNvbQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 7, 2013, at 2:19 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > On Wed, 2013-08-07 at 14:04 -0400, Trond Myklebust wrote: >> On Wed, 2013-08-07 at 18:01 +0000, Adamson, Andy wrote: >>> >>> Here is the attack as described in 3530bis Security Considerations >>> section: >>> >>> >>> The second operation that should definitely use integrity protection >>> is any GETATTR for the fs_locations attribute. The attack has two >>> steps. First the attacker modifies the unprotected results of some >>> operation to return NFS4ERR_MOVED. Second, when the client follows >>> up with a GETATTR for the fs_locations attribute, the attacker >>> modifies the results to cause the client migrate its traffic to a >>> server controlled by the attacker. >> >> You can the exact same thing by changing the READLINK results. > > The attack is: change the unprotected LOOKUP results to point to a > symlink, then feed '/net/<evil-ip-address>/my/evil/pathname' into > READLINK. > > My point is that if you're on a network where the above is a potential > threat, then you should be using krb5i or, better yet, krb5p for _all_ > operations. It's not sufficient to single out fs_locations for special > treatment. In that case, why did you accept commit 4edaa308 "NFS: Use "krb5i" to establish NFSv4 state whenever possible" ? -->Andy > > -- > Trond Myklebust > Linux NFS client maintainer > > NetApp > Trond.Myklebust@netapp.com > www.netapp.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re-send due to my mailer adding html to the message, and thus being rejected by linux-nfs@vger.kernel.org -->Andy Begin forwarded message: > From: "Adamson, Andy" <William.Adamson@netapp.com> > Subject: Re: [PATCH 2/4] NFSv4.1 Use clientid management rpc_clnt for fs_locations > Date: August 7, 2013 2:24:31 PM EDT > To: "Myklebust, Trond" <Trond.Myklebust@netapp.com> > Cc: "Adamson, Andy" <William.Adamson@netapp.com>, "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org> > > > On Aug 7, 2013, at 2:19 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> > wrote: > >> On Wed, 2013-08-07 at 14:04 -0400, Trond Myklebust wrote: >>> On Wed, 2013-08-07 at 18:01 +0000, Adamson, Andy wrote: >>>> >>>> Here is the attack as described in 3530bis Security Considerations >>>> section: >>>> >>>> >>>> The second operation that should definitely use integrity protection >>>> is any GETATTR for the fs_locations attribute. The attack has two >>>> steps. First the attacker modifies the unprotected results of some >>>> operation to return NFS4ERR_MOVED. Second, when the client follows >>>> up with a GETATTR for the fs_locations attribute, the attacker >>>> modifies the results to cause the client migrate its traffic to a >>>> server controlled by the attacker. >>> >>> You can the exact same thing by changing the READLINK results. >> >> The attack is: change the unprotected LOOKUP results to point to a >> symlink, then feed '/net/<evil-ip-address>/my/evil/pathname' into >> READLINK. >> >> My point is that if you're on a network where the above is a potential >> threat, then you should be using krb5i or, better yet, krb5p for _all_ >> operations. It's not sufficient to single out fs_locations for special >> treatment. > > In that case, why did you accept commit 4edaa308 "NFS: Use "krb5i" to establish NFSv4 state whenever possible" ? > > -->Andy > >> >> -- >> Trond Myklebust >> Linux NFS client maintainer >> >> NetApp >> Trond.Myklebust@netapp.com >> www.netapp.com > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 7, 2013, at 2:19 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote: > On Wed, 2013-08-07 at 14:04 -0400, Trond Myklebust wrote: >> On Wed, 2013-08-07 at 18:01 +0000, Adamson, Andy wrote: >>> >>> Here is the attack as described in 3530bis Security Considerations >>> section: >>> >>> >>> The second operation that should definitely use integrity protection >>> is any GETATTR for the fs_locations attribute. The attack has two >>> steps. First the attacker modifies the unprotected results of some >>> operation to return NFS4ERR_MOVED. Second, when the client follows >>> up with a GETATTR for the fs_locations attribute, the attacker >>> modifies the results to cause the client migrate its traffic to a >>> server controlled by the attacker. >> >> You can the exact same thing by changing the READLINK results. > > The attack is: change the unprotected LOOKUP results to point to a > symlink, then feed '/net/<evil-ip-address>/my/evil/pathname' into > READLINK. Does the linux client actually follow links with embedded IP addresses? -->Andy > > My point is that if you're on a network where the above is a potential > threat, then you should be using krb5i or, better yet, krb5p for _all_ > operations. It's not sufficient to single out fs_locations for special > treatment. > > -- > Trond Myklebust > Linux NFS client maintainer > > NetApp > Trond.Myklebust@netapp.com > www.netapp.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-08-07 at 18:24 +0000, Adamson, Andy wrote: > On Aug 7, 2013, at 2:19 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> > wrote: > > > On Wed, 2013-08-07 at 14:04 -0400, Trond Myklebust wrote: > >> On Wed, 2013-08-07 at 18:01 +0000, Adamson, Andy wrote: > >>> > >>> Here is the attack as described in 3530bis Security Considerations > >>> section: > >>> > >>> > >>> The second operation that should definitely use integrity protection > >>> is any GETATTR for the fs_locations attribute. The attack has two > >>> steps. First the attacker modifies the unprotected results of some > >>> operation to return NFS4ERR_MOVED. Second, when the client follows > >>> up with a GETATTR for the fs_locations attribute, the attacker > >>> modifies the results to cause the client migrate its traffic to a > >>> server controlled by the attacker. > >> > >> You can the exact same thing by changing the READLINK results. > > > > The attack is: change the unprotected LOOKUP results to point to a > > symlink, then feed '/net/<evil-ip-address>/my/evil/pathname' into > > READLINK. > > > > My point is that if you're on a network where the above is a potential > > threat, then you should be using krb5i or, better yet, krb5p for _all_ > > operations. It's not sufficient to single out fs_locations for special > > treatment. > > In that case, why did you accept commit 4edaa308 "NFS: Use "krb5i" to establish NFSv4 state whenever possible" ? 1. To avoid the NFS4ERR_CLID_IN_USE problem when you change from one mount auth flavour to another. 2. In scenarios where mixed security is in use, the state manager should always use the strongest security. Previously, we just chose whatever security flavour that was used first. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
On Wed, 2013-08-07 at 18:32 +0000, Adamson, Andy wrote: > On Aug 7, 2013, at 2:19 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> > wrote: > > > On Wed, 2013-08-07 at 14:04 -0400, Trond Myklebust wrote: > >> On Wed, 2013-08-07 at 18:01 +0000, Adamson, Andy wrote: > >>> > >>> Here is the attack as described in 3530bis Security Considerations > >>> section: > >>> > >>> > >>> The second operation that should definitely use integrity protection > >>> is any GETATTR for the fs_locations attribute. The attack has two > >>> steps. First the attacker modifies the unprotected results of some > >>> operation to return NFS4ERR_MOVED. Second, when the client follows > >>> up with a GETATTR for the fs_locations attribute, the attacker > >>> modifies the results to cause the client migrate its traffic to a > >>> server controlled by the attacker. > >> > >> You can the exact same thing by changing the READLINK results. > > > > The attack is: change the unprotected LOOKUP results to point to a > > symlink, then feed '/net/<evil-ip-address>/my/evil/pathname' into > > READLINK. > > Does the linux client actually follow links with embedded IP addresses? If you have autofs or amd running on your client, then sure... -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h index ee81e35..97feff2 100644 --- a/fs/nfs/nfs4_fs.h +++ b/fs/nfs/nfs4_fs.h @@ -231,7 +231,7 @@ extern int nfs4_init_clientid(struct nfs_client *, struct rpc_cred *); extern int nfs41_init_clientid(struct nfs_client *, struct rpc_cred *); extern int nfs4_do_close(struct nfs4_state *state, gfp_t gfp_mask, int wait); extern int nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *fhandle); -extern int nfs4_proc_fs_locations(struct rpc_clnt *, struct inode *, const struct qstr *, +extern int nfs4_proc_fs_locations(struct inode *, const struct qstr *, struct nfs4_fs_locations *, struct page *); extern struct rpc_clnt *nfs4_proc_lookup_mountpoint(struct inode *, struct qstr *, struct nfs_fh *, struct nfs_fattr *); diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c index cdb0b41..dca2f3a 100644 --- a/fs/nfs/nfs4namespace.c +++ b/fs/nfs/nfs4namespace.c @@ -350,7 +350,7 @@ static struct vfsmount *nfs_do_refmount(struct rpc_clnt *client, struct dentry * dprintk("%s: getting locations for %s/%s\n", __func__, parent->d_name.name, dentry->d_name.name); - err = nfs4_proc_fs_locations(client, parent->d_inode, &dentry->d_name, fs_locations, page); + err = nfs4_proc_fs_locations(parent->d_inode, &dentry->d_name, fs_locations, page); dput(parent); if (err != 0 || fs_locations->nlocations <= 0 || diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 7a846b6..7761802 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -2831,7 +2831,7 @@ err_free_label: * Note that we'll actually follow the referral later when * we detect fsid mismatch in inode revalidation */ -static int nfs4_get_referral(struct rpc_clnt *client, struct inode *dir, +static int nfs4_get_referral(struct inode *dir, const struct qstr *name, struct nfs_fattr *fattr, struct nfs_fh *fhandle) { @@ -2846,7 +2846,7 @@ static int nfs4_get_referral(struct rpc_clnt *client, struct inode *dir, if (locations == NULL) goto out; - status = nfs4_proc_fs_locations(client, dir, name, locations, page); + status = nfs4_proc_fs_locations(dir, name, locations, page); if (status != 0) goto out; /* Make sure server returned a different fsid for the referral */ @@ -3025,7 +3025,7 @@ static int nfs4_proc_lookup_common(struct rpc_clnt **clnt, struct inode *dir, err = -ENOENT; goto out; case -NFS4ERR_MOVED: - err = nfs4_get_referral(client, dir, name, fattr, fhandle); + err = nfs4_get_referral(dir, name, fattr, fhandle); goto out; case -NFS4ERR_WRONGSEC: err = -EPERM; @@ -5733,7 +5733,7 @@ static void nfs_fixup_referral_attributes(struct nfs_fattr *fattr) fattr->nlink = 2; } -static int _nfs4_proc_fs_locations(struct rpc_clnt *client, struct inode *dir, +static int _nfs4_proc_fs_locations(struct inode *dir, const struct qstr *name, struct nfs4_fs_locations *fs_locations, struct page *page) @@ -5756,6 +5756,7 @@ static int _nfs4_proc_fs_locations(struct rpc_clnt *client, struct inode *dir, .rpc_argp = &args, .rpc_resp = &res, }; + struct rpc_clnt *client = NFS_SERVER(dir)->nfs_client->cl_rpcclient; int status; dprintk("%s: start\n", __func__); @@ -5775,7 +5776,7 @@ static int _nfs4_proc_fs_locations(struct rpc_clnt *client, struct inode *dir, return status; } -int nfs4_proc_fs_locations(struct rpc_clnt *client, struct inode *dir, +int nfs4_proc_fs_locations(struct inode *dir, const struct qstr *name, struct nfs4_fs_locations *fs_locations, struct page *page) @@ -5784,7 +5785,7 @@ int nfs4_proc_fs_locations(struct rpc_clnt *client, struct inode *dir, int err; do { err = nfs4_handle_exception(NFS_SERVER(dir), - _nfs4_proc_fs_locations(client, dir, name, fs_locations, page), + _nfs4_proc_fs_locations(dir, name, fs_locations, page), &exception); } while (exception.retry); return err;