From c66080ad0a9295eaf0246a46949619f92c75faaa Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Tue, 8 Oct 2013 15:33:53 -0400 Subject: svcrpc: fix gss-proxy NULL dereference in some error cases We depend on the xdr decoder to set this pointer, but if we error out before we decode this piece it could be left NULL. I think this is probably tough to hit without a buggy gss-proxy. Reported-by: Andi Kleen Cc: Simo Sorce Signed-off-by: J. Bruce Fields --- net/sunrpc/auth_gss/gss_rpc_upcall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'net/sunrpc/auth_gss') diff --git a/net/sunrpc/auth_gss/gss_rpc_upcall.c b/net/sunrpc/auth_gss/gss_rpc_upcall.c index f1eb0d16666c..458f85e9b0ba 100644 --- a/net/sunrpc/auth_gss/gss_rpc_upcall.c +++ b/net/sunrpc/auth_gss/gss_rpc_upcall.c @@ -298,7 +298,8 @@ int gssp_accept_sec_context_upcall(struct net *net, if (res.context_handle) { data->out_handle = rctxh.exported_context_token; data->mech_oid.len = rctxh.mech.len; - memcpy(data->mech_oid.data, rctxh.mech.data, + if (rctxh.mech.data) + memcpy(data->mech_oid.data, rctxh.mech.data, data->mech_oid.len); client_name = rctxh.src_name.display_name; } -- cgit v1.2.3 From 3be34555fa294483202483c4b171a8d335447187 Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Tue, 8 Oct 2013 15:53:07 -0400 Subject: svcrpc: fix error-handling on badd gssproxy downcall For every other problem here we bail out with an error, but here for some reason we're setting a negative cache entry (with, note, an undefined expiry). It seems simplest just to bail out in the same way as we do in other cases. Cc: Simo Sorce Reported-by: Andi Kleen Signed-off-by: J. Bruce Fields --- net/sunrpc/auth_gss/svcauth_gss.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'net/sunrpc/auth_gss') diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c index 09fb638bcaa4..008cdade5aae 100644 --- a/net/sunrpc/auth_gss/svcauth_gss.c +++ b/net/sunrpc/auth_gss/svcauth_gss.c @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd, if (!ud->found_creds) { /* userspace seem buggy, we should always get at least a * mapping to nobody */ - dprintk("RPC: No creds found, marking Negative!\n"); - set_bit(CACHE_NEGATIVE, &rsci.h.flags); + dprintk("RPC: No creds found!\n"); + goto out; } else { /* steal creds */ -- cgit v1.2.3 From b26ec9b11b309acd9f6bb15fcc9bb396091384e8 Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Tue, 8 Oct 2013 16:09:04 -0400 Subject: svcrpc: handle some gssproxy encoding errors Reported-by: Andi Kleen Signed-off-by: J. Bruce Fields --- net/sunrpc/auth_gss/gss_rpc_xdr.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) (limited to 'net/sunrpc/auth_gss') diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c index f0f78c5f1c7d..1ec19f6f0c2b 100644 --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c @@ -559,6 +559,8 @@ static int gssx_enc_cred(struct xdr_stream *xdr, /* cred->elements */ err = dummy_enc_credel_array(xdr, &cred->elements); + if (err) + return err; /* cred->cred_handle_reference */ err = gssx_enc_buffer(xdr, &cred->cred_handle_reference); @@ -740,22 +742,20 @@ void gssx_enc_accept_sec_context(struct rpc_rqst *req, goto done; /* arg->context_handle */ - if (arg->context_handle) { + if (arg->context_handle) err = gssx_enc_ctx(xdr, arg->context_handle); - if (err) - goto done; - } else { + else err = gssx_enc_bool(xdr, 0); - } + if (err) + goto done; /* arg->cred_handle */ - if (arg->cred_handle) { + if (arg->cred_handle) err = gssx_enc_cred(xdr, arg->cred_handle); - if (err) - goto done; - } else { + else err = gssx_enc_bool(xdr, 0); - } + if (err) + goto done; /* arg->input_token */ err = gssx_enc_in_token(xdr, &arg->input_token); @@ -763,13 +763,12 @@ void gssx_enc_accept_sec_context(struct rpc_rqst *req, goto done; /* arg->input_cb */ - if (arg->input_cb) { + if (arg->input_cb) err = gssx_enc_cb(xdr, arg->input_cb); - if (err) - goto done; - } else { + else err = gssx_enc_bool(xdr, 0); - } + if (err) + goto done; err = gssx_enc_bool(xdr, arg->ret_deleg_cred); if (err) -- cgit v1.2.3 From 5d6baef9ab52d0d02b3106d8ccd1b05ec628e027 Mon Sep 17 00:00:00 2001 From: "J. Bruce Fields" Date: Wed, 9 Oct 2013 15:59:29 -0400 Subject: gss_krb5: document that we ignore sequence number A couple times recently somebody has noticed that we're ignoring a sequence number here and wondered whether there's a bug. In fact, there's not. Thanks to Andy Adamson for pointing out a useful explanation in rfc 2203. Add comments citing that rfc, and remove "seqnum" to prevent static checkers complaining about unused variables. Reported-by: Andi Kleen Signed-off-by: J. Bruce Fields --- net/sunrpc/auth_gss/gss_krb5_unseal.c | 8 ++++---- net/sunrpc/auth_gss/gss_krb5_wrap.c | 6 ++++-- 2 files changed, 8 insertions(+), 6 deletions(-) (limited to 'net/sunrpc/auth_gss') diff --git a/net/sunrpc/auth_gss/gss_krb5_unseal.c b/net/sunrpc/auth_gss/gss_krb5_unseal.c index 6cd930f3678f..6c981ddc19f8 100644 --- a/net/sunrpc/auth_gss/gss_krb5_unseal.c +++ b/net/sunrpc/auth_gss/gss_krb5_unseal.c @@ -150,7 +150,6 @@ gss_verify_mic_v2(struct krb5_ctx *ctx, struct xdr_netobj cksumobj = {.len = sizeof(cksumdata), .data = cksumdata}; s32 now; - u64 seqnum; u8 *ptr = read_token->data; u8 *cksumkey; u8 flags; @@ -197,9 +196,10 @@ gss_verify_mic_v2(struct krb5_ctx *ctx, if (now > ctx->endtime) return GSS_S_CONTEXT_EXPIRED; - /* do sequencing checks */ - - seqnum = be64_to_cpup((__be64 *)ptr + 8); + /* + * NOTE: the sequence number at ptr + 8 is skipped, rpcsec_gss + * doesn't want it checked; see page 6 of rfc 2203. + */ return GSS_S_COMPLETE; } diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c index 1da52d1406fc..5040a460f1d9 100644 --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c @@ -489,7 +489,6 @@ static u32 gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf) { s32 now; - u64 seqnum; u8 *ptr; u8 flags = 0x00; u16 ec, rrc; @@ -525,7 +524,10 @@ gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf) ec = be16_to_cpup((__be16 *)(ptr + 4)); rrc = be16_to_cpup((__be16 *)(ptr + 6)); - seqnum = be64_to_cpup((__be64 *)(ptr + 8)); + /* + * NOTE: the sequence number at ptr + 8 is skipped, rpcsec_gss + * doesn't want it checked; see page 6 of rfc 2203. + */ if (rrc != 0) rotate_left(offset + 16, buf, rrc); -- cgit v1.2.3 From cf4c024b908353fcc48309374d39e3399d67dfd1 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Thu, 10 Oct 2013 06:55:35 -0400 Subject: sunrpc: trim off EC bytes in GSSAPI v2 unwrap As Bruce points out in RFC 4121, section 4.2.3: "In Wrap tokens that provide for confidentiality, the first 16 octets of the Wrap token (the "header", as defined in section 4.2.6), SHALL be appended to the plaintext data before encryption. Filler octets MAY be inserted between the plaintext data and the "header."" ...and... "In Wrap tokens with confidentiality, the EC field SHALL be used to encode the number of octets in the filler..." It's possible for the client to stuff different data in that area on a retransmission, which could make the checksum come out wrong in the DRC code. After decrypting the blob, we should trim off any extra count bytes in addition to the checksum blob. Reported-by: "J. Bruce Fields" Signed-off-by: Jeff Layton Signed-off-by: J. Bruce Fields --- net/sunrpc/auth_gss/gss_krb5_wrap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'net/sunrpc/auth_gss') diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c b/net/sunrpc/auth_gss/gss_krb5_wrap.c index 5040a460f1d9..42560e55d978 100644 --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c @@ -576,8 +576,8 @@ gss_unwrap_kerberos_v2(struct krb5_ctx *kctx, int offset, struct xdr_buf *buf) buf->head[0].iov_len -= GSS_KRB5_TOK_HDR_LEN + headskip; buf->len -= GSS_KRB5_TOK_HDR_LEN + headskip; - /* Trim off the checksum blob */ - xdr_buf_trim(buf, GSS_KRB5_TOK_HDR_LEN + tailskip); + /* Trim off the trailing "extra count" and checksum blob */ + xdr_buf_trim(buf, ec + GSS_KRB5_TOK_HDR_LEN + tailskip); return GSS_S_COMPLETE; } -- cgit v1.2.3