krb5 commit [krb5-1.16]: Fix KDC null dereference on large TGS replies
Greg Hudson
ghudson at mit.edu
Wed May 2 01:25:42 EDT 2018
https://github.com/krb5/krb5/commit/086b505b7248ec78502857d6dac72a57c59b36e8
commit 086b505b7248ec78502857d6dac72a57c59b36e8
Author: Robbie Harwood <rharwood at redhat.com>
Date: Fri Apr 20 16:16:02 2018 -0400
Fix KDC null dereference on large TGS replies
For TGS requests, dispatch() doesn't set state->active_realm, which
leads to a NULL dereference in finish_dispatch() if the reply is too
big for UDP. Prior to commit 0a2f14f752c32a24200363cc6b6ae64a92f81379
the active realm was a global and was set when process_tgs_req()
called setup_server_realm().
Move TGS decoding out of process_tgs_req() so that we can set
state->active_realm before any errors requiring response. Add a test
case.
[ghudson at mit.edu: edited commit message; added test case; reduced code
duplication; removed server handle from process_tgs_req() parameters]
(cherry picked from commit 6afa8b4abf8f7c5774d03e6b15ee7288ad68d725)
ticket: 8666
version_fixed: 1.16.1
src/kdc/Makefile.in | 1 +
src/kdc/dispatch.c | 48 +++++++++++++++++++++++++++---------------------
src/kdc/do_tgs_req.c | 24 ++++++------------------
src/kdc/kdc_util.h | 5 ++---
src/kdc/t_bigreply.py | 19 +++++++++++++++++++
5 files changed, 55 insertions(+), 42 deletions(-)
diff --git a/src/kdc/Makefile.in b/src/kdc/Makefile.in
index 61a3dbc..117a8f5 100644
--- a/src/kdc/Makefile.in
+++ b/src/kdc/Makefile.in
@@ -85,6 +85,7 @@ check-cmocka: t_replay
check-pytests:
$(RUNPYTEST) $(srcdir)/t_workers.py $(PYTESTFLAGS)
$(RUNPYTEST) $(srcdir)/t_emptytgt.py $(PYTESTFLAGS)
+ $(RUNPYTEST) $(srcdir)/t_bigreply.py $(PYTESTFLAGS)
install:
$(INSTALL_PROGRAM) krb5kdc ${DESTDIR}$(SERVER_BINDIR)/krb5kdc
diff --git a/src/kdc/dispatch.c b/src/kdc/dispatch.c
index 3867ff9..3ed5176 100644
--- a/src/kdc/dispatch.c
+++ b/src/kdc/dispatch.c
@@ -124,7 +124,7 @@ dispatch(void *cb, const krb5_fulladdr *local_addr,
verto_ctx *vctx, loop_respond_fn respond, void *arg)
{
krb5_error_code retval;
- krb5_kdc_req *as_req;
+ krb5_kdc_req *req = NULL;
krb5_data *response = NULL;
struct dispatch_state *state;
struct server_handle *handle = cb;
@@ -176,29 +176,35 @@ dispatch(void *cb, const krb5_fulladdr *local_addr,
/* try TGS_REQ first; they are more common! */
+ if (krb5_is_tgs_req(pkt))
+ retval = decode_krb5_tgs_req(pkt, &req);
+ else if (krb5_is_as_req(pkt))
+ retval = decode_krb5_as_req(pkt, &req);
+ else
+ retval = KRB5KRB_AP_ERR_MSG_TYPE;
+ if (retval)
+ goto done;
+
+ state->active_realm = setup_server_realm(handle, req->server);
+ if (state->active_realm == NULL) {
+ retval = KRB5KDC_ERR_WRONG_REALM;
+ goto done;
+ }
+
if (krb5_is_tgs_req(pkt)) {
- retval = process_tgs_req(handle, pkt, remote_addr, &response);
+ /* process_tgs_req frees the request */
+ retval = process_tgs_req(req, pkt, remote_addr, state->active_realm,
+ &response);
+ req = NULL;
} else if (krb5_is_as_req(pkt)) {
- if (!(retval = decode_krb5_as_req(pkt, &as_req))) {
- /*
- * setup_server_realm() sets up the global realm-specific data
- * pointer.
- * process_as_req frees the request if it is called
- */
- state->active_realm = setup_server_realm(handle, as_req->server);
- if (state->active_realm != NULL) {
- process_as_req(as_req, pkt, local_addr, remote_addr,
- state->active_realm, vctx,
- finish_dispatch_cache, state);
- return;
- } else {
- retval = KRB5KDC_ERR_WRONG_REALM;
- krb5_free_kdc_req(kdc_err_context, as_req);
- }
- }
- } else
- retval = KRB5KRB_AP_ERR_MSG_TYPE;
+ /* process_as_req frees the request and calls finish_dispatch_cache. */
+ process_as_req(req, pkt, local_addr, remote_addr, state->active_realm,
+ vctx, finish_dispatch_cache, state);
+ return;
+ }
+done:
+ krb5_free_kdc_req(kdc_err_context, req);
finish_dispatch_cache(state, retval, response);
}
diff --git a/src/kdc/do_tgs_req.c b/src/kdc/do_tgs_req.c
index fca01db..2d2e930 100644
--- a/src/kdc/do_tgs_req.c
+++ b/src/kdc/do_tgs_req.c
@@ -98,12 +98,12 @@ search_sprinc(kdc_realm_t *, krb5_kdc_req *, krb5_flags,
/*ARGSUSED*/
krb5_error_code
-process_tgs_req(struct server_handle *handle, krb5_data *pkt,
- const krb5_fulladdr *from, krb5_data **response)
+process_tgs_req(krb5_kdc_req *request, krb5_data *pkt,
+ const krb5_fulladdr *from, kdc_realm_t *kdc_active_realm,
+ krb5_data **response)
{
krb5_keyblock * subkey = 0;
krb5_keyblock *header_key = NULL;
- krb5_kdc_req *request = 0;
krb5_db_entry *server = NULL;
krb5_db_entry *stkt_server = NULL;
krb5_kdc_rep reply;
@@ -136,7 +136,6 @@ process_tgs_req(struct server_handle *handle, krb5_data *pkt,
krb5_pa_data *pa_tgs_req; /*points into request*/
krb5_data scratch;
krb5_pa_data **e_data = NULL;
- kdc_realm_t *kdc_active_realm = NULL;
krb5_audit_state *au_state = NULL;
krb5_data **auth_indicators = NULL;
@@ -147,36 +146,25 @@ process_tgs_req(struct server_handle *handle, krb5_data *pkt,
memset(&encrypting_key, 0, sizeof(encrypting_key));
session_key.contents = NULL;
- retval = decode_krb5_tgs_req(pkt, &request);
- if (retval)
- return retval;
/* Save pointer to client-requested service principal, in case of
* errors before a successful call to search_sprinc(). */
sprinc = request->server;
if (request->msg_type != KRB5_TGS_REQ) {
- krb5_free_kdc_req(handle->kdc_err_context, request);
+ krb5_free_kdc_req(kdc_context, request);
return KRB5_BADMSGTYPE;
}
- /*
- * setup_server_realm() sets up the global realm-specific data pointer.
- */
- kdc_active_realm = setup_server_realm(handle, request->server);
- if (kdc_active_realm == NULL) {
- krb5_free_kdc_req(handle->kdc_err_context, request);
- return KRB5KDC_ERR_WRONG_REALM;
- }
errcode = kdc_make_rstate(kdc_active_realm, &state);
if (errcode !=0) {
- krb5_free_kdc_req(handle->kdc_err_context, request);
+ krb5_free_kdc_req(kdc_context, request);
return errcode;
}
/* Initialize audit state. */
errcode = kau_init_kdc_req(kdc_context, request, from, &au_state);
if (errcode) {
- krb5_free_kdc_req(handle->kdc_err_context, request);
+ krb5_free_kdc_req(kdc_context, request);
return errcode;
}
/* Seed the audit trail with the request ID and basic information. */
diff --git a/src/kdc/kdc_util.h b/src/kdc/kdc_util.h
index f99efcf..c57d48f 100644
--- a/src/kdc/kdc_util.h
+++ b/src/kdc/kdc_util.h
@@ -145,9 +145,8 @@ process_as_req (krb5_kdc_req *, krb5_data *,
/* do_tgs_req.c */
krb5_error_code
-process_tgs_req (struct server_handle *, krb5_data *,
- const krb5_fulladdr *,
- krb5_data ** );
+process_tgs_req (krb5_kdc_req *, krb5_data *, const krb5_fulladdr *,
+ kdc_realm_t *, krb5_data ** );
/* dispatch.c */
void
dispatch (void *,
diff --git a/src/kdc/t_bigreply.py b/src/kdc/t_bigreply.py
new file mode 100644
index 0000000..6bc9a8f
--- /dev/null
+++ b/src/kdc/t_bigreply.py
@@ -0,0 +1,19 @@
+#!/usr/bin/python
+from k5test import *
+
+# Set the maximum UDP reply size very low, so that all replies go
+# through the RESPONSE_TOO_BIG path.
+kdc_conf = {'kdcdefaults': {'kdc_max_dgram_reply_size': '10'}}
+realm = K5Realm(kdc_conf=kdc_conf, get_creds=False)
+
+msgs = ('Sending initial UDP request',
+ 'Received answer',
+ 'Request or response is too big for UDP; retrying with TCP',
+ ' to KRBTEST.COM (tcp only)',
+ 'Initiating TCP connection',
+ 'Sending TCP request',
+ 'Terminating TCP connection')
+realm.kinit(realm.user_princ, password('user'), expected_trace=msgs)
+realm.run([kvno, realm.host_princ], expected_trace=msgs)
+
+success('Large KDC replies')
More information about the cvs-krb5
mailing list