krb5 commit [krb5-1.12]: Fix gss_accept_sec_context error tokens
Tom Yu
tlyu at MIT.EDU
Thu Oct 17 18:07:22 EDT 2013
https://github.com/krb5/krb5/commit/5df4b25fe8e64ca1555662a11a4b8d8b042e048a
commit 5df4b25fe8e64ca1555662a11a4b8d8b042e048a
Author: Greg Hudson <ghudson at mit.edu>
Date: Tue Oct 8 17:07:34 2013 -0400
Fix gss_accept_sec_context error tokens
A GSS krb5 error response contains a KRB-ERROR message, which is
required to have a server principal name, although few recipients
actually use it. Starting in 1.3, accept_sec_context would fail to
encode the error in the GSS_C_NO_NAME/GSS_C_NO_CREDENTIAL case
(introduced by #1370) because cred->princ (which became
cred->name->princ in 1.8) is unset.
This problem got worse in 1.10 because we stopped setting the server
field in all cases due to the changes for #6855. In 1.11 the problem
got worse again when a misguided change to the mechglue started
discarding output tokens when the mechanism returns an error; the
mechglue should only do so when it itself causes the error.
Fix krb5 gss_accept_sec_context by unconditionally decoding the AP-REQ
and using krb5_rd_req_decoded, and then using the requested ticket
server in the KRB-ERROR message. Fix the mechglue
gss_accept_sec_context by reverting that part of commit
56feee187579905c9101b0cdbdd8c6a850adcfc9. Add a test program which
artificially induces a replay cache failure (the easiest failure we
can produce which has an associated RFC 4120 error code) and checks
that this can be communicated back to the initiator via an error
token.
(cherry picked from commit c547bc16f2ab6ee66c076ef944c3fbac8a66f5d4)
ticket: 1445
version_fixed: 1.12
status: resolved
src/lib/gssapi/krb5/accept_sec_context.c | 43 ++++-----
src/lib/gssapi/mechglue/g_accept_sec_context.c | 6 +-
src/lib/krb5_32.def | 1 +
src/tests/gssapi/Makefile.in | 19 ++--
src/tests/gssapi/t_err.c | 121 ++++++++++++++++++++++++
src/tests/gssapi/t_gssapi.py | 4 +
6 files changed, 157 insertions(+), 37 deletions(-)
diff --git a/src/lib/gssapi/krb5/accept_sec_context.c b/src/lib/gssapi/krb5/accept_sec_context.c
index 9f9b6c6..82f03b7 100644
--- a/src/lib/gssapi/krb5/accept_sec_context.c
+++ b/src/lib/gssapi/krb5/accept_sec_context.c
@@ -450,7 +450,6 @@ kg_accept_krb5(minor_status, context_handle,
krb5_checksum reqcksum;
krb5_gss_name_t name = NULL;
krb5_ui_4 gss_flags = 0;
- int decode_req_message = 0;
krb5_gss_ctx_id_rec *ctx = NULL;
krb5_timestamp now;
gss_buffer_desc token;
@@ -472,6 +471,7 @@ kg_accept_krb5(minor_status, context_handle,
krb5_enctype negotiated_etype;
krb5_authdata_context ad_context = NULL;
krb5_principal accprinc = NULL;
+ krb5_ap_req *request = NULL;
code = krb5int_accessor (&kaccess, KRB5INT_ACCESS_VERSION);
if (code) {
@@ -586,7 +586,6 @@ kg_accept_krb5(minor_status, context_handle,
sptr = (char *) ptr;
TREAD_STR(sptr, ap_req.data, ap_req.length);
- decode_req_message = 1;
/* construct the sender_addr */
@@ -603,6 +602,11 @@ kg_accept_krb5(minor_status, context_handle,
}
/* decode the AP_REQ message */
+ code = decode_krb5_ap_req(&ap_req, &request);
+ if (code) {
+ major_status = GSS_S_FAILURE;
+ goto done;
+ }
/* decode the message */
@@ -639,8 +643,9 @@ kg_accept_krb5(minor_status, context_handle,
}
}
- code = krb5_rd_req(context, &auth_context, &ap_req, accprinc,
- cred->keytab, &ap_req_options, &ticket);
+ code = krb5_rd_req_decoded(context, &auth_context, request, accprinc,
+ cred->keytab, &ap_req_options, &ticket);
+
krb5_free_principal(context, accprinc);
if (code) {
major_status = GSS_S_FAILURE;
@@ -697,7 +702,6 @@ kg_accept_krb5(minor_status, context_handle,
}
gss_flags = GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG | GSS_C_SEQUENCE_FLAG;
- decode_req_message = 0;
} else {
/* gss krb5 v1 */
@@ -777,7 +781,6 @@ kg_accept_krb5(minor_status, context_handle,
there's a delegation, we'll set
it below */
#endif
- decode_req_message = 0;
/* if the checksum length > 24, there are options to process */
@@ -1203,26 +1206,12 @@ fail:
*minor_status = code;
- /*
- * If decode_req_message is set, then we need to decode the ap_req
- * message to determine whether or not to send a response token.
- * We need to do this because for some errors we won't be able to
- * decode the authenticator to read out the gss_flags field.
- */
- if (decode_req_message) {
- krb5_ap_req * request;
-
- if (decode_krb5_ap_req(&ap_req, &request))
- goto done;
-
- if (request->ap_options & AP_OPTS_MUTUAL_REQUIRED)
- gss_flags |= GSS_C_MUTUAL_FLAG;
- krb5_free_ap_req(context, request);
- }
-
- if (cred
- && ((gss_flags & GSS_C_MUTUAL_FLAG)
- || (major_status == GSS_S_CONTINUE_NEEDED))) {
+ /* We may have failed before being able to read the GSS flags from the
+ * authenticator, so also check the request AP options. */
+ if (cred != NULL && request != NULL &&
+ ((gss_flags & GSS_C_MUTUAL_FLAG) ||
+ (request->ap_options & AP_OPTS_MUTUAL_REQUIRED) ||
+ major_status == GSS_S_CONTINUE_NEEDED)) {
unsigned int tmsglen;
int toktype;
@@ -1240,6 +1229,7 @@ fail:
(void) krb5_us_timeofday(context, &krb_error_data.stime,
&krb_error_data.susec);
+ krb_error_data.server = request->ticket->server;
code = krb5_mk_error(context, &krb_error_data, &scratch);
if (code)
goto done;
@@ -1262,6 +1252,7 @@ fail:
}
done:
+ krb5_free_ap_req(context, request);
if (cred)
k5_mutex_unlock(&cred->lock);
if (defcred)
diff --git a/src/lib/gssapi/mechglue/g_accept_sec_context.c b/src/lib/gssapi/mechglue/g_accept_sec_context.c
index dae83cc..b8f128b 100644
--- a/src/lib/gssapi/mechglue/g_accept_sec_context.c
+++ b/src/lib/gssapi/mechglue/g_accept_sec_context.c
@@ -269,6 +269,9 @@ gss_cred_id_t * d_cred;
status = temp_status;
*minor_status = temp_minor_status;
map_error(minor_status, mech);
+ if (output_token->length)
+ (void) gss_release_buffer(&temp_minor_status,
+ output_token);
goto error_out;
}
*src_name = tmp_src_name;
@@ -357,9 +360,6 @@ error_out:
(void) gss_release_buffer(&temp_minor_status,
(gss_buffer_t)tmp_src_name);
- if (output_token->length)
- (void) gss_release_buffer(&temp_minor_status, output_token);
-
return (status);
}
#endif /* LEAN_CLIENT */
diff --git a/src/lib/krb5_32.def b/src/lib/krb5_32.def
index b3ce21a..dd0a16c 100644
--- a/src/lib/krb5_32.def
+++ b/src/lib/krb5_32.def
@@ -451,3 +451,4 @@ EXPORTS
krb5_responder_pkinit_set_answer @422
krb5_responder_pkinit_challenge_free @423
krb5_auth_con_setpermetypes @424 ; PRIVATE GSSAPI
+ krb5_rd_req_decoded @425 ; PRIVATE GSSAPI
diff --git a/src/tests/gssapi/Makefile.in b/src/tests/gssapi/Makefile.in
index 5f554e1..0bf5bc8 100644
--- a/src/tests/gssapi/Makefile.in
+++ b/src/tests/gssapi/Makefile.in
@@ -4,7 +4,7 @@ DEFINES = -DUSE_AUTOCONF_H
SRCS= $(srcdir)/ccinit.c $(srcdir)/ccrefresh.c $(srcdir)/common.c \
$(srcdir)/t_accname.c $(srcdir)/t_ccselect.c $(srcdir)/t_credstore.c \
- $(srcdir)/t_enctypes.c $(srcdir)/t_export_cred.c \
+ $(srcdir)/t_enctypes.c $(srcdir)/t_err.c $(srcdir)/t_export_cred.c \
$(srcdir)/t_export_name.c $(srcdir)/t_gssexts.c \
$(srcdir)/t_imp_cred.c $(srcdir)/t_imp_name.c $(srcdir)/t_inq_cred.c \
$(srcdir)/t_inq_mechs_name.c $(srcdir)/t_iov.c \
@@ -13,14 +13,15 @@ SRCS= $(srcdir)/ccinit.c $(srcdir)/ccrefresh.c $(srcdir)/common.c \
$(srcdir)/t_spnego.c
OBJS= ccinit.o ccrefresh.o common.o t_accname.o t_ccselect.o t_credstore.o \
- t_enctypes.o t_export_cred.o t_export_name.o t_gssexts.o t_imp_cred.o \
- t_imp_name.o t_inq_cred.o t_inq_mechs_name.o t_iov.o t_namingexts.o \
- t_oid.o t_s4u.o t_s4u2proxy_krb5.o t_saslname.o t_spnego.o
+ t_enctypes.o t_err.o t_export_cred.o t_export_name.o t_gssexts.o \
+ t_imp_cred.o t_imp_name.o t_inq_cred.o t_inq_mechs_name.o t_iov.o \
+ t_namingexts.o t_oid.o t_s4u.o t_s4u2proxy_krb5.o t_saslname.o \
+ t_spnego.o
COMMON_DEPS= common.o $(GSS_DEPLIBS) $(KRB5_BASE_DEPLIBS)
COMMON_LIBS= common.o $(GSS_LIBS) $(KRB5_BASE_LIBS)
-all:: ccinit ccrefresh t_accname t_ccselect t_credstore t_enctypes \
+all:: ccinit ccrefresh t_accname t_ccselect t_credstore t_enctypes t_err \
t_export_cred t_export_name t_gssexts t_imp_cred t_imp_name \
t_inq_cred t_inq_mechs_name t_iov t_namingexts t_oid t_s4u \
t_s4u2proxy_krb5 t_saslname t_spnego
@@ -29,8 +30,8 @@ check-unix:: t_oid
$(RUN_SETUP) $(VALGRIND) ./t_oid
check-pytests:: ccinit ccrefresh t_accname t_ccselect t_credstore t_enctypes \
- t_export_cred t_export_name t_imp_cred t_inq_cred t_inq_mechs_name \
- t_iov t_s4u t_s4u2proxy_krb5 t_spnego
+ t_err t_export_cred t_export_name t_imp_cred t_inq_cred \
+ t_inq_mechs_name t_iov t_s4u t_s4u2proxy_krb5 t_spnego
$(RUNPYTEST) $(srcdir)/t_gssapi.py $(PYTESTFLAGS)
$(RUNPYTEST) $(srcdir)/t_ccselect.py $(PYTESTFLAGS)
$(RUNPYTEST) $(srcdir)/t_client_keytab.py $(PYTESTFLAGS)
@@ -50,6 +51,8 @@ t_credstore: t_credstore.o $(COMMON_DEPS)
$(CC_LINK) -o $@ t_credstore.o $(COMMON_LIBS)
t_enctypes: t_enctypes.o $(COMMON_DEPS)
$(CC_LINK) -o $@ t_enctypes.o $(COMMON_LIBS)
+t_err: t_err.o $(COMMON_DEPS)
+ $(CC_LINK) -o $@ t_err.o $(COMMON_LIBS)
t_export_cred: t_export_cred.o $(COMMON_DEPS)
$(CC_LINK) -o $@ t_export_cred.o $(COMMON_LIBS)
t_export_name: t_export_name.o $(COMMON_DEPS)
@@ -81,6 +84,6 @@ t_spnego: t_spnego.o $(COMMON_DEPS)
clean::
$(RM) ccinit ccrefresh t_accname t_ccselect t_credstore t_enctypes
- $(RM) t_export_cred t_export_name t_gssexts t_imp_cred t_imp_name
+ $(RM) t_err t_export_cred t_export_name t_gssexts t_imp_cred t_imp_name
$(RM) t_inq_cred t_inq_mechs_name t_iov t_namingexts t_oid t_s4u
$(RM) t_s4u2proxy_krb5 t_saslname t_spnego
diff --git a/src/tests/gssapi/t_err.c b/src/tests/gssapi/t_err.c
new file mode 100644
index 0000000..b7c32b4
--- /dev/null
+++ b/src/tests/gssapi/t_err.c
@@ -0,0 +1,121 @@
+/* -*- mode: c; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/* tests/gssapi/t_err.c - Test accept_sec_context error generation */
+/*
+ * Copyright (C) 2013 by the Massachusetts Institute of Technology.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in
+ * the documentation and/or other materials provided with the
+ * distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+/*
+ * This test program verifies that the krb5 gss_accept_sec_context can produce
+ * error tokens and that gss_init_sec_context can interpret them.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <assert.h>
+
+#include "common.h"
+
+static void
+check_replay_error(const char *msg, OM_uint32 major, OM_uint32 minor)
+{
+ OM_uint32 tmpmin, msg_ctx = 0;
+ const char *replay = "Request is a replay";
+ gss_buffer_desc m;
+
+ if (major != GSS_S_FAILURE) {
+ fprintf(stderr, "%s: expected major code GSS_S_FAILURE\n", msg);
+ check_gsserr(msg, major, minor);
+ exit(1);
+ }
+
+ (void)gss_display_status(&tmpmin, minor, GSS_C_MECH_CODE, GSS_C_NULL_OID,
+ &msg_ctx, &m);
+ if (m.length != strlen(replay) || memcmp(m.value, replay, m.length) != 0) {
+ fprintf(stderr, "%s: expected replay error; got %.*s\n", msg,
+ (int)m.length, (char *)m.value);
+ exit(1);
+ }
+ (void)gss_release_buffer(&tmpmin, &m);
+}
+
+int
+main(int argc, char *argv[])
+{
+ OM_uint32 minor, major, flags;
+ gss_OID mech = &mech_krb5;
+ gss_name_t tname;
+ gss_buffer_desc itok, atok;
+ gss_ctx_id_t ictx = GSS_C_NO_CONTEXT, actx = GSS_C_NO_CONTEXT;
+
+ if (argc != 2) {
+ fprintf(stderr, "Usage: %s targetname\n", argv[0]);
+ return 1;
+ }
+ tname = import_name(argv[1]);
+
+ /* Get the initial context token. */
+ flags = GSS_C_REPLAY_FLAG | GSS_C_SEQUENCE_FLAG | GSS_C_MUTUAL_FLAG;
+ major = gss_init_sec_context(&minor, GSS_C_NO_CREDENTIAL, &ictx, tname,
+ mech, flags, GSS_C_INDEFINITE,
+ GSS_C_NO_CHANNEL_BINDINGS, GSS_C_NO_BUFFER,
+ NULL, &itok, NULL, NULL);
+ check_gsserr("gss_init_sec_context(1)", major, minor);
+ assert(major == GSS_S_CONTINUE_NEEDED);
+
+ /* Process this token into an acceptor context, then discard it. */
+ major = gss_accept_sec_context(&minor, &actx, GSS_C_NO_CREDENTIAL, &itok,
+ GSS_C_NO_CHANNEL_BINDINGS, NULL,
+ NULL, &atok, NULL, NULL, NULL);
+ check_gsserr("gss_accept_sec_context(1)", major, minor);
+ (void)gss_release_buffer(&minor, &atok);
+ (void)gss_delete_sec_context(&minor, &actx, NULL);
+
+ /* Process the same token again, producing a replay error. */
+ major = gss_accept_sec_context(&minor, &actx, GSS_C_NO_CREDENTIAL, &itok,
+ GSS_C_NO_CHANNEL_BINDINGS, NULL,
+ NULL, &atok, NULL, NULL, NULL);
+ check_replay_error("gss_accept_sec_context(2)", major, minor);
+ assert(atok.length != 0);
+
+ /* Send the error token back the initiator. */
+ (void)gss_release_buffer(&minor, &itok);
+ major = gss_init_sec_context(&minor, GSS_C_NO_CREDENTIAL, &ictx, tname,
+ mech, flags, GSS_C_INDEFINITE,
+ GSS_C_NO_CHANNEL_BINDINGS, &atok,
+ NULL, &itok, NULL, NULL);
+ check_replay_error("gss_init_sec_context(2)", major, minor);
+
+ (void)gss_release_name(&minor, &tname);
+ (void)gss_release_buffer(&minor, &itok);
+ (void)gss_release_buffer(&minor, &atok);
+ (void)gss_delete_sec_context(&minor, &ictx, NULL);
+ (void)gss_delete_sec_context(&minor, &actx, NULL);
+ return 0;
+}
diff --git a/src/tests/gssapi/t_gssapi.py b/src/tests/gssapi/t_gssapi.py
index 2b86d3e..74139e4 100755
--- a/src/tests/gssapi/t_gssapi.py
+++ b/src/tests/gssapi/t_gssapi.py
@@ -194,4 +194,8 @@ out = realm.run(['./t_inq_mechs_name', 'h:host'])
if krb5_mech not in out or spnego_mech not in out:
fail('t_inq_mecs_name (hostbased)')
+# Test that accept_sec_context can produce an error token and
+# init_sec_context can interpret it.
+realm.run(['./t_err', 'p:' + realm.host_princ])
+
success('GSSAPI tests')
More information about the cvs-krb5
mailing list