diff mbox

nfsd: SECINFO* can use integrity protected flavors

Message ID 1378232772-4066-1-git-send-email-dros@netapp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Weston Andros Adamson Sept. 3, 2013, 6:26 p.m. UTC
SECINFO and SECINFO_NONAME should be able to use integrity protected auth
flavors even if the export wasn't explicitly configured to use them.

An example is a sec=sys export - upstream linux clients will attempt to use
krb5i for EXCHANGE_ID, CREATE_SESSION, DESTROY_SESSION, etc.  If this is
successful, the client will try to use the same auth flavor for SECINFO
as described in the Security Considerations sections of rfc3530 and rfc5661.

This patch adds a nfsd4_op_flag to describe operations that may use these
auth flavors to get around nfsd_access() checks. This should be useful in
future implementations of SP4_MACH_CRED (nfsd_permission still needs to be
handled).

This patch also stops SECINFO* from returning NFS4ERR_WRONGSEC which is
not allowed by either rfc3530 (not in list of allowed errors) or rfc6551
(section 2.6.3.1.1.5 says it MUST NOT).

Signed-off-by: Weston Andros Adamson <dros@netapp.com>
---
 fs/nfsd/export.c   | 10 ++++++++++
 fs/nfsd/nfs4proc.c | 27 ++++++++++++++++++++++++---
 fs/nfsd/xdr4.h     |  2 ++
 3 files changed, 36 insertions(+), 3 deletions(-)

Comments

Chuck Lever III Sept. 3, 2013, 6:48 p.m. UTC | #1
On Sep 3, 2013, at 2:26 PM, Weston Andros Adamson <dros@netapp.com> wrote:

> SECINFO and SECINFO_NONAME should be able to use integrity protected auth
> flavors even if the export wasn't explicitly configured to use them.
> 
> An example is a sec=sys export - upstream linux clients will attempt to use
> krb5i for EXCHANGE_ID, CREATE_SESSION, DESTROY_SESSION, etc.  If this is
> successful, the client will try to use the same auth flavor for SECINFO
> as described in the Security Considerations sections of rfc3530 and rfc5661.

The reason krb5i can work for lease management operations is because those operations are not connected to any particular export, so typically they are not controlled directly by export security settings.

But I'm not sure all server implementations will allow a SECINFO on an FSID using a flavor not allowed by the FSID's export options.  SECINFO is definitely connected to a particular FSID, unlike lease management operations.

Given FreeBSD's implementation choices wrt lease management security flavors, that would be the first server I'd check.

> This patch adds a nfsd4_op_flag to describe operations that may use these
> auth flavors to get around nfsd_access() checks. This should be useful in
> future implementations of SP4_MACH_CRED (nfsd_permission still needs to be
> handled).
> 
> This patch also stops SECINFO* from returning NFS4ERR_WRONGSEC which is
> not allowed by either rfc3530 (not in list of allowed errors) or rfc6551

5661

> (section 2.6.3.1.1.5 says it MUST NOT).

That sounds inconsistent with the Security Considerations recommendation.

Recommending that a client use integrity protection means servers MUST support it, or the client has to be prepared to retry it with some other flavor, and the server MUST have a way to ask for a retry (ie in that case, wouldn't WRONGSEC be the correct way to make such a request?).

Is there anything more in these RFCs that would require a server to support SECINFO via krb5i?

Thus I wonder if clients can depend on a krb5i SECINFO on a sec=sys FSID working everywhere.

> 
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
> fs/nfsd/export.c   | 10 ++++++++++
> fs/nfsd/nfs4proc.c | 27 ++++++++++++++++++++++++---
> fs/nfsd/xdr4.h     |  2 ++
> 3 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 5f38ea3..e2e5a13 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -22,6 +22,7 @@
> #include "nfsd.h"
> #include "nfsfh.h"
> #include "netns.h"
> +#include "xdr4.h"
> 
> #define NFSDDBG_FACILITY	NFSDDBG_EXPORT
> 
> @@ -909,6 +910,15 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
> 		    rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
> 			return 0;
> 	}
> +
> +	/* some operations allow use of integrity even though the mount
> +	 * may not be */
> +	if (nfsd4_allow_integrity(rqstp)) {
> +		if (rqstp->rq_cred.cr_flavor == RPC_AUTH_GSS_KRB5I ||
> +		    rqstp->rq_cred.cr_flavor == RPC_AUTH_GSS_KRB5P)
> +			return 0;
> +	}
> +
> 	return nfserr_wrongsec;
> }
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 0d4c410..ce79483 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1158,6 +1158,11 @@ enum nfsd4_op_flags {
> 	 * These are ops which clear current state id.
> 	 */
> 	OP_CLEAR_STATEID = 1 << 7,
> +	/*
> +	 * Procedures that can use auth flavors other than flavors
> +	 * specified in the export as long as they are integrity protected.
> +	 */
> +	OP_ALLOW_INTEGRITY = 1 << 8,
> };
> 
> struct nfsd4_operation {
> @@ -1249,7 +1254,23 @@ static bool need_wrongsec_check(struct svc_rqst *rqstp)
> 	 * errors themselves as necessary; others should check for them
> 	 * now:
> 	 */
> -	return !(nextd->op_flags & OP_HANDLES_WRONGSEC);
> +	return !(nextd->op_flags & (OP_HANDLES_WRONGSEC | OP_ALLOW_INTEGRITY));
> +}
> +
> +bool
> +nfsd4_allow_integrity(struct svc_rqst *rqstp)
> +{
> +	struct nfsd4_compoundres *resp = rqstp->rq_resp;
> +	struct nfsd4_compoundargs *argp = rqstp->rq_argp;
> +	struct nfsd4_op *this = &argp->ops[resp->opcnt - 1];
> +	struct nfsd4_operation *thisd;
> +
> +	thisd = OPDESC(this);
> +
> +	if (thisd->op_flags & OP_ALLOW_INTEGRITY)
> +		return true;
> +
> +	return false;
> }
> 
> /*
> @@ -1727,7 +1748,7 @@ static struct nfsd4_operation nfsd4_ops[] = {
> 	},
> 	[OP_SECINFO] = {
> 		.op_func = (nfsd4op_func)nfsd4_secinfo,
> -		.op_flags = OP_HANDLES_WRONGSEC,
> +		.op_flags = OP_ALLOW_INTEGRITY,
> 		.op_name = "OP_SECINFO",
> 	},
> 	[OP_SETATTR] = {
> @@ -1825,7 +1846,7 @@ static struct nfsd4_operation nfsd4_ops[] = {
> 	},
> 	[OP_SECINFO_NO_NAME] = {
> 		.op_func = (nfsd4op_func)nfsd4_secinfo_no_name,
> -		.op_flags = OP_HANDLES_WRONGSEC,
> +		.op_flags = OP_ALLOW_INTEGRITY,
> 		.op_name = "OP_SECINFO_NO_NAME",
> 	},
> 	[OP_TEST_STATEID] = {
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index b3ed644..ed79b6d 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -481,6 +481,8 @@ struct nfsd4_op {
> 
> bool nfsd4_cache_this_op(struct nfsd4_op *);
> 
> +bool nfsd4_allow_integrity(struct svc_rqst *);
> +
> struct nfsd4_compoundargs {
> 	/* scratch variables for XDR decode */
> 	__be32 *			p;
> -- 
> 1.7.12.4 (Apple Git-37)
> 
> --
> 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
Adamson, Dros Sept. 3, 2013, 7:11 p.m. UTC | #2
On Sep 3, 2013, at 2:48 PM, Chuck Lever <chuck.lever@oracle.com>
 wrote:

> 
> On Sep 3, 2013, at 2:26 PM, Weston Andros Adamson <dros@netapp.com> wrote:
> 
>> SECINFO and SECINFO_NONAME should be able to use integrity protected auth
>> flavors even if the export wasn't explicitly configured to use them.
>> 
>> An example is a sec=sys export - upstream linux clients will attempt to use
>> krb5i for EXCHANGE_ID, CREATE_SESSION, DESTROY_SESSION, etc.  If this is
>> successful, the client will try to use the same auth flavor for SECINFO
>> as described in the Security Considerations sections of rfc3530 and rfc5661.
> 
> The reason krb5i can work for lease management operations is because those operations are not connected to any particular export, so typically they are not controlled directly by export security settings.
> 
> But I'm not sure all server implementations will allow a SECINFO on an FSID using a flavor not allowed by the FSID's export options.  SECINFO is definitely connected to a particular FSID, unlike lease management operations.

I understand how these operations are different from SECINFO, but the spec is pretty clear that krb5i/p should be used if at all possible.

> 
> Given FreeBSD's implementation choices wrt lease management security flavors, that would be the first server I'd check.
> 
>> This patch adds a nfsd4_op_flag to describe operations that may use these
>> auth flavors to get around nfsd_access() checks. This should be useful in
>> future implementations of SP4_MACH_CRED (nfsd_permission still needs to be
>> handled).
>> 
>> This patch also stops SECINFO* from returning NFS4ERR_WRONGSEC which is
>> not allowed by either rfc3530 (not in list of allowed errors) or rfc6551
> 
> 5661

Heh, good catch!

> 
>> (section 2.6.3.1.1.5 says it MUST NOT).
> 
> That sounds inconsistent with the Security Considerations recommendation.

How so? Just because we don't know what error to return if the server doesn't support the recommendation to use integrity protection for SECINFO?

> 
> Recommending that a client use integrity protection means servers MUST support it, or the client has to be prepared to retry it with some other flavor, and the server MUST have a way to ask for a retry (ie in that case, wouldn't WRONGSEC be the correct way to make such a request?).

So, I'm preparing to post a client-side patch that retries (with filesystem's rpc_client) if the server returns NFS4ERR_WRONGSEC as Trond doesn't want the client to be broken against deployed linux servers, but this is a good question - what error should a server return in this case?

> 
> Is there anything more in these RFCs that would require a server to support SECINFO via krb5i?
> 
> Thus I wonder if clients can depend on a krb5i SECINFO on a sec=sys FSID working everywhere.

Sections 17 of rfc3530bis and 21 of rfc 5661 clearly state that it's RECOMMENDED to use integrity protection for SECINFO and SECINFO_NONAME, so I think we should try, but your point remains - what error should a server return if the server doesn't support this?

-dros

> 
>> 
>> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
>> ---
>> fs/nfsd/export.c   | 10 ++++++++++
>> fs/nfsd/nfs4proc.c | 27 ++++++++++++++++++++++++---
>> fs/nfsd/xdr4.h     |  2 ++
>> 3 files changed, 36 insertions(+), 3 deletions(-)
>> 
>> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
>> index 5f38ea3..e2e5a13 100644
>> --- a/fs/nfsd/export.c
>> +++ b/fs/nfsd/export.c
>> @@ -22,6 +22,7 @@
>> #include "nfsd.h"
>> #include "nfsfh.h"
>> #include "netns.h"
>> +#include "xdr4.h"
>> 
>> #define NFSDDBG_FACILITY	NFSDDBG_EXPORT
>> 
>> @@ -909,6 +910,15 @@ __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
>> 		    rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
>> 			return 0;
>> 	}
>> +
>> +	/* some operations allow use of integrity even though the mount
>> +	 * may not be */
>> +	if (nfsd4_allow_integrity(rqstp)) {
>> +		if (rqstp->rq_cred.cr_flavor == RPC_AUTH_GSS_KRB5I ||
>> +		    rqstp->rq_cred.cr_flavor == RPC_AUTH_GSS_KRB5P)
>> +			return 0;
>> +	}
>> +
>> 	return nfserr_wrongsec;
>> }
>> 
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index 0d4c410..ce79483 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -1158,6 +1158,11 @@ enum nfsd4_op_flags {
>> 	 * These are ops which clear current state id.
>> 	 */
>> 	OP_CLEAR_STATEID = 1 << 7,
>> +	/*
>> +	 * Procedures that can use auth flavors other than flavors
>> +	 * specified in the export as long as they are integrity protected.
>> +	 */
>> +	OP_ALLOW_INTEGRITY = 1 << 8,
>> };
>> 
>> struct nfsd4_operation {
>> @@ -1249,7 +1254,23 @@ static bool need_wrongsec_check(struct svc_rqst *rqstp)
>> 	 * errors themselves as necessary; others should check for them
>> 	 * now:
>> 	 */
>> -	return !(nextd->op_flags & OP_HANDLES_WRONGSEC);
>> +	return !(nextd->op_flags & (OP_HANDLES_WRONGSEC | OP_ALLOW_INTEGRITY));
>> +}
>> +
>> +bool
>> +nfsd4_allow_integrity(struct svc_rqst *rqstp)
>> +{
>> +	struct nfsd4_compoundres *resp = rqstp->rq_resp;
>> +	struct nfsd4_compoundargs *argp = rqstp->rq_argp;
>> +	struct nfsd4_op *this = &argp->ops[resp->opcnt - 1];
>> +	struct nfsd4_operation *thisd;
>> +
>> +	thisd = OPDESC(this);
>> +
>> +	if (thisd->op_flags & OP_ALLOW_INTEGRITY)
>> +		return true;
>> +
>> +	return false;
>> }
>> 
>> /*
>> @@ -1727,7 +1748,7 @@ static struct nfsd4_operation nfsd4_ops[] = {
>> 	},
>> 	[OP_SECINFO] = {
>> 		.op_func = (nfsd4op_func)nfsd4_secinfo,
>> -		.op_flags = OP_HANDLES_WRONGSEC,
>> +		.op_flags = OP_ALLOW_INTEGRITY,
>> 		.op_name = "OP_SECINFO",
>> 	},
>> 	[OP_SETATTR] = {
>> @@ -1825,7 +1846,7 @@ static struct nfsd4_operation nfsd4_ops[] = {
>> 	},
>> 	[OP_SECINFO_NO_NAME] = {
>> 		.op_func = (nfsd4op_func)nfsd4_secinfo_no_name,
>> -		.op_flags = OP_HANDLES_WRONGSEC,
>> +		.op_flags = OP_ALLOW_INTEGRITY,
>> 		.op_name = "OP_SECINFO_NO_NAME",
>> 	},
>> 	[OP_TEST_STATEID] = {
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index b3ed644..ed79b6d 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -481,6 +481,8 @@ struct nfsd4_op {
>> 
>> bool nfsd4_cache_this_op(struct nfsd4_op *);
>> 
>> +bool nfsd4_allow_integrity(struct svc_rqst *);
>> +
>> struct nfsd4_compoundargs {
>> 	/* scratch variables for XDR decode */
>> 	__be32 *			p;
>> -- 
>> 1.7.12.4 (Apple Git-37)
>> 
>> --
>> 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
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]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

--
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
bfields@fieldses.org Sept. 4, 2013, 10:18 p.m. UTC | #3
On Tue, Sep 03, 2013 at 07:11:57PM +0000, Adamson, Dros wrote:
> 
> On Sep 3, 2013, at 2:48 PM, Chuck Lever <chuck.lever@oracle.com>
>  wrote:
> 
> > 
> > On Sep 3, 2013, at 2:26 PM, Weston Andros Adamson <dros@netapp.com> wrote:
> > 
> >> SECINFO and SECINFO_NONAME should be able to use integrity protected auth
> >> flavors even if the export wasn't explicitly configured to use them.
> >> 
> >> An example is a sec=sys export - upstream linux clients will attempt to use
> >> krb5i for EXCHANGE_ID, CREATE_SESSION, DESTROY_SESSION, etc.  If this is
> >> successful, the client will try to use the same auth flavor for SECINFO
> >> as described in the Security Considerations sections of rfc3530 and rfc5661.
> > 
> > The reason krb5i can work for lease management operations is because those operations are not connected to any particular export, so typically they are not controlled directly by export security settings.
> > 
> > But I'm not sure all server implementations will allow a SECINFO on an FSID using a flavor not allowed by the FSID's export options.  SECINFO is definitely connected to a particular FSID, unlike lease management operations.
> 
> I understand how these operations are different from SECINFO, but the spec is pretty clear that krb5i/p should be used if at all possible.
> 
> > 
> > Given FreeBSD's implementation choices wrt lease management security flavors, that would be the first server I'd check.
> > 
> >> This patch adds a nfsd4_op_flag to describe operations that may use these
> >> auth flavors to get around nfsd_access() checks. This should be useful in
> >> future implementations of SP4_MACH_CRED (nfsd_permission still needs to be
> >> handled).
> >> 
> >> This patch also stops SECINFO* from returning NFS4ERR_WRONGSEC which is
> >> not allowed by either rfc3530 (not in list of allowed errors) or rfc6551
> > 
> > 5661
> 
> Heh, good catch!
> 
> > 
> >> (section 2.6.3.1.1.5 says it MUST NOT).
> > 
> > That sounds inconsistent with the Security Considerations recommendation.
> 
> How so? Just because we don't know what error to return if the server doesn't support the recommendation to use integrity protection for SECINFO?
> 
> > 
> > Recommending that a client use integrity protection means servers MUST support it, or the client has to be prepared to retry it with some other flavor, and the server MUST have a way to ask for a retry (ie in that case, wouldn't WRONGSEC be the correct way to make such a request?).
> 
> So, I'm preparing to post a client-side patch that retries (with filesystem's rpc_client) if the server returns NFS4ERR_WRONGSEC as Trond doesn't want the client to be broken against deployed linux servers, but this is a good question - what error should a server return in this case?
> 
> > 
> > Is there anything more in these RFCs that would require a server to support SECINFO via krb5i?
> > 
> > Thus I wonder if clients can depend on a krb5i SECINFO on a sec=sys FSID working everywhere.
> 
> Sections 17 of rfc3530bis and 21 of rfc 5661 clearly state that it's RECOMMENDED to use integrity protection for SECINFO and SECINFO_NONAME, so I think we should try, but your point remains - what error should a server return if the server doesn't support this?

I'm confused--we should just allow SECINFO/SECINFO_NO_NAME in these
cases and then we don't have to decide, right?

--b.
--
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
Chuck Lever III Sept. 5, 2013, 2:07 p.m. UTC | #4
On Sep 4, 2013, at 6:18 PM, bfields@fieldses.org wrote:

> On Tue, Sep 03, 2013 at 07:11:57PM +0000, Adamson, Dros wrote:
>> 
>> On Sep 3, 2013, at 2:48 PM, Chuck Lever <chuck.lever@oracle.com>
>> wrote:
>> 
>>> 
>>> On Sep 3, 2013, at 2:26 PM, Weston Andros Adamson <dros@netapp.com> wrote:
>>> 
>>>> SECINFO and SECINFO_NONAME should be able to use integrity protected auth
>>>> flavors even if the export wasn't explicitly configured to use them.
>>>> 
>>>> An example is a sec=sys export - upstream linux clients will attempt to use
>>>> krb5i for EXCHANGE_ID, CREATE_SESSION, DESTROY_SESSION, etc.  If this is
>>>> successful, the client will try to use the same auth flavor for SECINFO
>>>> as described in the Security Considerations sections of rfc3530 and rfc5661.
>>> 
>>> The reason krb5i can work for lease management operations is because those operations are not connected to any particular export, so typically they are not controlled directly by export security settings.
>>> 
>>> But I'm not sure all server implementations will allow a SECINFO on an FSID using a flavor not allowed by the FSID's export options.  SECINFO is definitely connected to a particular FSID, unlike lease management operations.
>> 
>> I understand how these operations are different from SECINFO, but the spec is pretty clear that krb5i/p should be used if at all possible.
>> 
>>> 
>>> Given FreeBSD's implementation choices wrt lease management security flavors, that would be the first server I'd check.
>>> 
>>>> This patch adds a nfsd4_op_flag to describe operations that may use these
>>>> auth flavors to get around nfsd_access() checks. This should be useful in
>>>> future implementations of SP4_MACH_CRED (nfsd_permission still needs to be
>>>> handled).
>>>> 
>>>> This patch also stops SECINFO* from returning NFS4ERR_WRONGSEC which is
>>>> not allowed by either rfc3530 (not in list of allowed errors) or rfc6551
>>> 
>>> 5661
>> 
>> Heh, good catch!
>> 
>>> 
>>>> (section 2.6.3.1.1.5 says it MUST NOT).
>>> 
>>> That sounds inconsistent with the Security Considerations recommendation.
>> 
>> How so? Just because we don't know what error to return if the server doesn't support the recommendation to use integrity protection for SECINFO?
>> 
>>> 
>>> Recommending that a client use integrity protection means servers MUST support it, or the client has to be prepared to retry it with some other flavor, and the server MUST have a way to ask for a retry (ie in that case, wouldn't WRONGSEC be the correct way to make such a request?).
>> 
>> So, I'm preparing to post a client-side patch that retries (with filesystem's rpc_client) if the server returns NFS4ERR_WRONGSEC as Trond doesn't want the client to be broken against deployed linux servers, but this is a good question - what error should a server return in this case?
>> 
>>> 
>>> Is there anything more in these RFCs that would require a server to support SECINFO via krb5i?
>>> 
>>> Thus I wonder if clients can depend on a krb5i SECINFO on a sec=sys FSID working everywhere.
>> 
>> Sections 17 of rfc3530bis and 21 of rfc 5661 clearly state that it's RECOMMENDED to use integrity protection for SECINFO and SECINFO_NONAME, so I think we should try, but your point remains - what error should a server return if the server doesn't support this?
> 
> I'm confused--we should just allow SECINFO/SECINFO_NO_NAME in these
> cases and then we don't have to decide, right?

I've finally read the relevant sections of the RFCs (doi!).  RFC 5661 section 2.6.3.1.1.4 and following discuss SECINFO/SECINFO_NO_NAME and NFS4ERR_WRONGSEC in some detail.

The way I read this, a server MUST NOT return NFS4ERR_WRONGSEC on any SECINFO or SECINFO_NO_NAME request, and MUST NOT return NFS4ERR_WRONGSEC on a PUTFH if the subsequent operation is SECINFO or SECINFO_NO_NAME.

Section 2.6.3.1.1.5 explicitly says "In theory, there is no connection between the security flavor used by SECINFO or SECINFO_NO_NAME and those supported by the security policy."  Despite the fact that both of these operations take a FH, and thus would be otherwise subject to the security policy in place for the containing FSID, they should be permitted under all security flavors a server supports.

If the server truly does not support the specified security flavor, an RPC-level error occurs, as Trond points out, and as the first paragraph of section 2.6.3.1.1.5 says.  This is an indication to the client that it should retry the SECINFO with another flavor.

RFC 3530bis does not state a similar strong requirement.  However, NFS4ERR_WRONGSEC is not listed in the valid status codes for the SECINFO operation, so we might assume that a client should expect similar behavior for NFSv4.0.

For the server, "we should just allow SECINFO/SECINFO_NO_NAME in these cases".  Perhaps there should be no security check at all -- the RPC layer should already cover the totally unsupported flavor case.  The only detail is ignoring security policy conflicts in a preceding PUTFH in the same compound.

I think the main difficulty now is that some versions of the Linux server do not behave this way.
Adamson, Dros Sept. 5, 2013, 3:03 p.m. UTC | #5
On Sep 5, 2013, at 10:07 AM, Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Sep 4, 2013, at 6:18 PM, bfields@fieldses.org wrote:
> 
>> On Tue, Sep 03, 2013 at 07:11:57PM +0000, Adamson, Dros wrote:
>>> 
>>> On Sep 3, 2013, at 2:48 PM, Chuck Lever <chuck.lever@oracle.com>
>>> wrote:
>>> 
>>>> 
>>>> On Sep 3, 2013, at 2:26 PM, Weston Andros Adamson <dros@netapp.com> wrote:
>>>> 
>>>>> SECINFO and SECINFO_NONAME should be able to use integrity protected auth
>>>>> flavors even if the export wasn't explicitly configured to use them.
>>>>> 
>>>>> An example is a sec=sys export - upstream linux clients will attempt to use
>>>>> krb5i for EXCHANGE_ID, CREATE_SESSION, DESTROY_SESSION, etc.  If this is
>>>>> successful, the client will try to use the same auth flavor for SECINFO
>>>>> as described in the Security Considerations sections of rfc3530 and rfc5661.
>>>> 
>>>> The reason krb5i can work for lease management operations is because those operations are not connected to any particular export, so typically they are not controlled directly by export security settings.
>>>> 
>>>> But I'm not sure all server implementations will allow a SECINFO on an FSID using a flavor not allowed by the FSID's export options.  SECINFO is definitely connected to a particular FSID, unlike lease management operations.
>>> 
>>> I understand how these operations are different from SECINFO, but the spec is pretty clear that krb5i/p should be used if at all possible.
>>> 
>>>> 
>>>> Given FreeBSD's implementation choices wrt lease management security flavors, that would be the first server I'd check.
>>>> 
>>>>> This patch adds a nfsd4_op_flag to describe operations that may use these
>>>>> auth flavors to get around nfsd_access() checks. This should be useful in
>>>>> future implementations of SP4_MACH_CRED (nfsd_permission still needs to be
>>>>> handled).
>>>>> 
>>>>> This patch also stops SECINFO* from returning NFS4ERR_WRONGSEC which is
>>>>> not allowed by either rfc3530 (not in list of allowed errors) or rfc6551
>>>> 
>>>> 5661
>>> 
>>> Heh, good catch!
>>> 
>>>> 
>>>>> (section 2.6.3.1.1.5 says it MUST NOT).
>>>> 
>>>> That sounds inconsistent with the Security Considerations recommendation.
>>> 
>>> How so? Just because we don't know what error to return if the server doesn't support the recommendation to use integrity protection for SECINFO?
>>> 
>>>> 
>>>> Recommending that a client use integrity protection means servers MUST support it, or the client has to be prepared to retry it with some other flavor, and the server MUST have a way to ask for a retry (ie in that case, wouldn't WRONGSEC be the correct way to make such a request?).
>>> 
>>> So, I'm preparing to post a client-side patch that retries (with filesystem's rpc_client) if the server returns NFS4ERR_WRONGSEC as Trond doesn't want the client to be broken against deployed linux servers, but this is a good question - what error should a server return in this case?
>>> 
>>>> 
>>>> Is there anything more in these RFCs that would require a server to support SECINFO via krb5i?
>>>> 
>>>> Thus I wonder if clients can depend on a krb5i SECINFO on a sec=sys FSID working everywhere.
>>> 
>>> Sections 17 of rfc3530bis and 21 of rfc 5661 clearly state that it's RECOMMENDED to use integrity protection for SECINFO and SECINFO_NONAME, so I think we should try, but your point remains - what error should a server return if the server doesn't support this?
>> 
>> I'm confused--we should just allow SECINFO/SECINFO_NO_NAME in these
>> cases and then we don't have to decide, right?
> 
> I've finally read the relevant sections of the RFCs (doi!).  RFC 5661 section 2.6.3.1.1.4 and following discuss SECINFO/SECINFO_NO_NAME and NFS4ERR_WRONGSEC in some detail.
> 
> The way I read this, a server MUST NOT return NFS4ERR_WRONGSEC on any SECINFO or SECINFO_NO_NAME request, and MUST NOT return NFS4ERR_WRONGSEC on a PUTFH if the subsequent operation is SECINFO or SECINFO_NO_NAME.
> 
> Section 2.6.3.1.1.5 explicitly says "In theory, there is no connection between the security flavor used by SECINFO or SECINFO_NO_NAME and those supported by the security policy."  Despite the fact that both of these operations take a FH, and thus would be otherwise subject to the security policy in place for the containing FSID, they should be permitted under all security flavors a server supports.
> 
> If the server truly does not support the specified security flavor, an RPC-level error occurs, as Trond points out, and as the first paragraph of section 2.6.3.1.1.5 says.  This is an indication to the client that it should retry the SECINFO with another flavor.
> 
> RFC 3530bis does not state a similar strong requirement.  However, NFS4ERR_WRONGSEC is not listed in the valid status codes for the SECINFO operation, so we might assume that a client should expect similar behavior for NFSv4.0.

Well put - thanks Chuck!

> 
> For the server, "we should just allow SECINFO/SECINFO_NO_NAME in these cases".  Perhaps there should be no security check at all -- the RPC layer should already cover the totally unsupported flavor case.  The only detail is ignoring security policy conflicts in a preceding PUTFH in the same compound.
> 

So should I refactor this patch so that it doesn't "allow integrity" for flagged operations and instead simply skips access checks?

> I think the main difficulty now is that some versions of the Linux server do not behave this way.

Indeed - thats why my recent client patches will handle WRONGSEC on SECINFO* - the SECINFO patch has been accepted by Trond, but the SECINFO_NO_NAME patch is still under discussion.

-dros

> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]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
diff mbox

Patch

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 5f38ea3..e2e5a13 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -22,6 +22,7 @@ 
 #include "nfsd.h"
 #include "nfsfh.h"
 #include "netns.h"
+#include "xdr4.h"
 
 #define NFSDDBG_FACILITY	NFSDDBG_EXPORT
 
@@ -909,6 +910,15 @@  __be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp)
 		    rqstp->rq_cred.cr_flavor == RPC_AUTH_UNIX)
 			return 0;
 	}
+
+	/* some operations allow use of integrity even though the mount
+	 * may not be */
+	if (nfsd4_allow_integrity(rqstp)) {
+		if (rqstp->rq_cred.cr_flavor == RPC_AUTH_GSS_KRB5I ||
+		    rqstp->rq_cred.cr_flavor == RPC_AUTH_GSS_KRB5P)
+			return 0;
+	}
+
 	return nfserr_wrongsec;
 }
 
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0d4c410..ce79483 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1158,6 +1158,11 @@  enum nfsd4_op_flags {
 	 * These are ops which clear current state id.
 	 */
 	OP_CLEAR_STATEID = 1 << 7,
+	/*
+	 * Procedures that can use auth flavors other than flavors
+	 * specified in the export as long as they are integrity protected.
+	 */
+	OP_ALLOW_INTEGRITY = 1 << 8,
 };
 
 struct nfsd4_operation {
@@ -1249,7 +1254,23 @@  static bool need_wrongsec_check(struct svc_rqst *rqstp)
 	 * errors themselves as necessary; others should check for them
 	 * now:
 	 */
-	return !(nextd->op_flags & OP_HANDLES_WRONGSEC);
+	return !(nextd->op_flags & (OP_HANDLES_WRONGSEC | OP_ALLOW_INTEGRITY));
+}
+
+bool
+nfsd4_allow_integrity(struct svc_rqst *rqstp)
+{
+	struct nfsd4_compoundres *resp = rqstp->rq_resp;
+	struct nfsd4_compoundargs *argp = rqstp->rq_argp;
+	struct nfsd4_op *this = &argp->ops[resp->opcnt - 1];
+	struct nfsd4_operation *thisd;
+
+	thisd = OPDESC(this);
+
+	if (thisd->op_flags & OP_ALLOW_INTEGRITY)
+		return true;
+
+	return false;
 }
 
 /*
@@ -1727,7 +1748,7 @@  static struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_SECINFO] = {
 		.op_func = (nfsd4op_func)nfsd4_secinfo,
-		.op_flags = OP_HANDLES_WRONGSEC,
+		.op_flags = OP_ALLOW_INTEGRITY,
 		.op_name = "OP_SECINFO",
 	},
 	[OP_SETATTR] = {
@@ -1825,7 +1846,7 @@  static struct nfsd4_operation nfsd4_ops[] = {
 	},
 	[OP_SECINFO_NO_NAME] = {
 		.op_func = (nfsd4op_func)nfsd4_secinfo_no_name,
-		.op_flags = OP_HANDLES_WRONGSEC,
+		.op_flags = OP_ALLOW_INTEGRITY,
 		.op_name = "OP_SECINFO_NO_NAME",
 	},
 	[OP_TEST_STATEID] = {
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index b3ed644..ed79b6d 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -481,6 +481,8 @@  struct nfsd4_op {
 
 bool nfsd4_cache_this_op(struct nfsd4_op *);
 
+bool nfsd4_allow_integrity(struct svc_rqst *);
+
 struct nfsd4_compoundargs {
 	/* scratch variables for XDR decode */
 	__be32 *			p;