krb5/Clear-forwardable-flag-instead-of-denying-request.patch

From ab1435ed0654df9991bddb29971c913ef1f957be Mon Sep 17 00:00:00 2001
From: Greg Hudson <ghudson@mit.edu>
Date: Thu, 15 Nov 2018 13:40:43 -0500
Subject: [PATCH] Clear forwardable flag instead of denying request

If the client requests a forwardable or proxiable ticket and the
option cannot be honored by policy, issue a non-forwardable or
non-proxiable ticket rather than denying the request.

Add a test script for testing KDC request options and populate it with
tests for the forwardable and proxiable flags.

ticket: 7871
(cherry picked from commit 08e948cce2c79a3604066fcf7a64fc527456f83d)
---
 src/kdc/do_as_req.c       |  19 ++------
 src/kdc/do_tgs_req.c      |  56 ++++-----------------
 src/kdc/kdc_util.c        |  82 ++++++++++++++++++-------------
 src/kdc/kdc_util.h        |   9 ++--
 src/kdc/tgs_policy.c      |   8 +--
 src/tests/Makefile.in     |   1 +
 src/tests/gcred.c         |  28 ++++++++---
 src/tests/t_kdcoptions.py | 100 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 189 insertions(+), 114 deletions(-)
 create mode 100644 src/tests/t_kdcoptions.py

diff --git a/src/kdc/do_as_req.c b/src/kdc/do_as_req.c
index 588c1375a..8a96c12a9 100644
--- a/src/kdc/do_as_req.c
+++ b/src/kdc/do_as_req.c
@@ -192,13 +192,6 @@ finish_process_as_req(struct as_req_state *state, krb5_error_code errcode)
 
     au_state->stage = ENCR_REP;
 
-    if ((errcode = validate_forwardable(state->request, *state->client,
-                                        *state->server, state->kdc_time,
-                                        &state->status))) {
-        errcode += ERROR_TABLE_BASE_krb5;
-        goto egress;
-    }
-
     errcode = check_indicators(kdc_context, state->server,
                                state->auth_indicators);
     if (errcode) {
@@ -708,12 +701,11 @@ process_as_req(krb5_kdc_req *request, krb5_data *req_pkt,
     }
 
     /* Copy options that request the corresponding ticket flags. */
-    state->enc_tkt_reply.flags = OPTS2FLAGS(state->request->kdc_options);
+    state->enc_tkt_reply.flags = get_ticket_flags(state->request->kdc_options,
+                                                  state->client, state->server,
+                                                  NULL);
     state->enc_tkt_reply.times.authtime = state->authtime;
 
-    setflag(state->enc_tkt_reply.flags, TKT_FLG_INITIAL);
-    setflag(state->enc_tkt_reply.flags, TKT_FLG_ENC_PA_REP);
-
     /*
      * It should be noted that local policy may affect the
      * processing of any of these flags.  For example, some
@@ -732,10 +724,9 @@ process_as_req(krb5_kdc_req *request, krb5_data *req_pkt,
     state->enc_tkt_reply.transited.tr_type = KRB5_DOMAIN_X500_COMPRESS;
     state->enc_tkt_reply.transited.tr_contents = empty_string;
 
-    if (isflagset(state->request->kdc_options, KDC_OPT_POSTDATED)) {
-        setflag(state->enc_tkt_reply.flags, TKT_FLG_INVALID);
+    if (isflagset(state->request->kdc_options, KDC_OPT_POSTDATED))
         state->enc_tkt_reply.times.starttime = state->request->from;
-    } else
+    else
         state->enc_tkt_reply.times.starttime = state->kdc_time;
 
     kdc_get_ticket_endtime(kdc_active_realm,
diff --git a/src/kdc/do_tgs_req.c b/src/kdc/do_tgs_req.c
index 587342a6c..1da099318 100644
--- a/src/kdc/do_tgs_req.c
+++ b/src/kdc/do_tgs_req.c
@@ -378,15 +378,16 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt,
     else
         ticket_reply.server = request->server; /* XXX careful for realm... */
 
-    enc_tkt_reply.flags = OPTS2FLAGS(request->kdc_options);
-    enc_tkt_reply.flags |= COPY_TKT_FLAGS(header_enc_tkt->flags);
+    enc_tkt_reply.flags = get_ticket_flags(request->kdc_options, client,
+                                           server, header_enc_tkt);
     enc_tkt_reply.times.starttime = 0;
 
-    if (isflagset(server->attributes, KRB5_KDB_OK_AS_DELEGATE))
-        setflag(enc_tkt_reply.flags, TKT_FLG_OK_AS_DELEGATE);
-
-    /* Indicate support for encrypted padata (RFC 6806). */
-    setflag(enc_tkt_reply.flags, TKT_FLG_ENC_PA_REP);
+    /* OK_TO_AUTH_AS_DELEGATE must be set on the service requesting S4U2Self
+     * for forwardable tickets to be issued. */
+    if (isflagset(c_flags, KRB5_KDB_FLAG_PROTOCOL_TRANSITION) &&
+        !is_referral &&
+        !isflagset(server->attributes, KRB5_KDB_OK_TO_AUTH_AS_DELEGATE))
+        clear(enc_tkt_reply.flags, TKT_FLG_FORWARDABLE);
 
     /* don't use new addresses unless forwarded, see below */
 
@@ -401,37 +402,6 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt,
      * realms may refuse to issue renewable tickets
      */
 
-    if (isflagset(request->kdc_options, KDC_OPT_FORWARDABLE)) {
-
-        if (isflagset(c_flags, KRB5_KDB_FLAG_PROTOCOL_TRANSITION)) {
-            /*
-             * If S4U2Self principal is not forwardable, then mark ticket as
-             * unforwardable. This behaviour matches Windows, but it is
-             * different to the MIT AS-REQ path, which returns an error
-             * (KDC_ERR_POLICY) if forwardable tickets cannot be issued.
-             *
-             * Consider this block the S4U2Self equivalent to
-             * validate_forwardable().
-             */
-            if (client != NULL &&
-                isflagset(client->attributes, KRB5_KDB_DISALLOW_FORWARDABLE))
-                clear(enc_tkt_reply.flags, TKT_FLG_FORWARDABLE);
-            /*
-             * Forwardable flag is propagated along referral path.
-             */
-            else if (!isflagset(header_enc_tkt->flags, TKT_FLG_FORWARDABLE))
-                clear(enc_tkt_reply.flags, TKT_FLG_FORWARDABLE);
-            /*
-             * OK_TO_AUTH_AS_DELEGATE must be set on the service requesting
-             * S4U2Self in order for forwardable tickets to be returned.
-             */
-            else if (!is_referral &&
-                     !isflagset(server->attributes,
-                                KRB5_KDB_OK_TO_AUTH_AS_DELEGATE))
-                clear(enc_tkt_reply.flags, TKT_FLG_FORWARDABLE);
-        }
-    }
-
     if (isflagset(request->kdc_options, KDC_OPT_FORWARDED) ||
         isflagset(request->kdc_options, KDC_OPT_PROXY)) {
 
@@ -440,16 +410,10 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt,
         enc_tkt_reply.caddrs = request->addresses;
         reply_encpart.caddrs = request->addresses;
     }
-    /* We don't currently handle issuing anonymous tickets based on
-     * non-anonymous ones, so just ignore the option. */
-    if (isflagset(request->kdc_options, KDC_OPT_REQUEST_ANONYMOUS) &&
-        !isflagset(header_enc_tkt->flags, TKT_FLG_ANONYMOUS))
-        clear(enc_tkt_reply.flags, TKT_FLG_ANONYMOUS);
 
-    if (isflagset(request->kdc_options, KDC_OPT_POSTDATED)) {
-        setflag(enc_tkt_reply.flags, TKT_FLG_INVALID);
+    if (isflagset(request->kdc_options, KDC_OPT_POSTDATED))
         enc_tkt_reply.times.starttime = request->from;
-    } else
+    else
         enc_tkt_reply.times.starttime = kdc_time;
 
     if (isflagset(request->kdc_options, KDC_OPT_VALIDATE)) {
diff --git a/src/kdc/kdc_util.c b/src/kdc/kdc_util.c
index 96c88edc1..f2741090e 100644
--- a/src/kdc/kdc_util.c
+++ b/src/kdc/kdc_util.c
@@ -697,29 +697,6 @@ validate_as_request(kdc_realm_t *kdc_active_realm,
         return(KDC_ERR_CANNOT_POSTDATE);
     }
 
-    /*
-     * A Windows KDC will return KDC_ERR_PREAUTH_REQUIRED instead of
-     * KDC_ERR_POLICY in the following case:
-     *
-     *   - KDC_OPT_FORWARDABLE is set in KDCOptions but local
-     *     policy has KRB5_KDB_DISALLOW_FORWARDABLE set for the
-     *     client, and;
-     *   - KRB5_KDB_REQUIRES_PRE_AUTH is set for the client but
-     *     preauthentication data is absent in the request.
-     *
-     * Hence, this check most be done after the check for preauth
-     * data, and is now performed by validate_forwardable() (the
-     * contents of which were previously below).
-     */
-
-    /* Client and server must allow proxiable tickets */
-    if (isflagset(request->kdc_options, KDC_OPT_PROXIABLE) &&
-        (isflagset(client.attributes, KRB5_KDB_DISALLOW_PROXIABLE) ||
-         isflagset(server.attributes, KRB5_KDB_DISALLOW_PROXIABLE))) {
-        *status = "PROXIABLE NOT ALLOWED";
-        return(KDC_ERR_POLICY);
-    }
-
     /* Check to see if client is locked out */
     if (isflagset(client.attributes, KRB5_KDB_DISALLOW_ALL_TIX)) {
         *status = "CLIENT LOCKED OUT";
@@ -752,19 +729,54 @@ validate_as_request(kdc_realm_t *kdc_active_realm,
     return 0;
 }
 
-int
-validate_forwardable(krb5_kdc_req *request, krb5_db_entry client,
-                     krb5_db_entry server, krb5_timestamp kdc_time,
-                     const char **status)
+/*
+ * Compute ticket flags based on the request, the client and server DB entry
+ * (which may prohibit forwardable or proxiable tickets), and the header
+ * ticket.  client may be NULL for a TGS request (although it may be set, such
+ * as for an S4U2Self request).  header_enc may be NULL for an AS request.
+ */
+krb5_flags
+get_ticket_flags(krb5_flags reqflags, krb5_db_entry *client,
+                 krb5_db_entry *server, krb5_enc_tkt_part *header_enc)
 {
-    *status = NULL;
-    if (isflagset(request->kdc_options, KDC_OPT_FORWARDABLE) &&
-        (isflagset(client.attributes, KRB5_KDB_DISALLOW_FORWARDABLE) ||
-         isflagset(server.attributes, KRB5_KDB_DISALLOW_FORWARDABLE))) {
-        *status = "FORWARDABLE NOT ALLOWED";
-        return(KDC_ERR_POLICY);
-    } else
-        return 0;
+    krb5_flags flags;
+
+    /* Indicate support for encrypted padata (RFC 6806), and set flags based on
+     * request options and the header ticket. */
+    flags = OPTS2FLAGS(reqflags) | TKT_FLG_ENC_PA_REP;
+    if (reqflags & KDC_OPT_POSTDATED)
+        flags |= TKT_FLG_INVALID;
+    if (header_enc != NULL)
+        flags |= COPY_TKT_FLAGS(header_enc->flags);
+    if (header_enc == NULL)
+        flags |= TKT_FLG_INITIAL;
+
+    /* For TGS requests, indicate if the service is marked ok-as-delegate. */
+    if (header_enc != NULL && (server->attributes & KRB5_KDB_OK_AS_DELEGATE))
+        flags |= TKT_FLG_OK_AS_DELEGATE;
+
+    /* Unset PROXIABLE if it is disallowed. */
+    if (client != NULL && (client->attributes & KRB5_KDB_DISALLOW_PROXIABLE))
+        flags &= ~TKT_FLG_PROXIABLE;
+    if (server->attributes & KRB5_KDB_DISALLOW_PROXIABLE)
+        flags &= ~TKT_FLG_PROXIABLE;
+    if (header_enc != NULL && !(header_enc->flags & TKT_FLG_PROXIABLE))
+        flags &= ~TKT_FLG_PROXIABLE;
+
+    /* Unset FORWARDABLE if it is disallowed. */
+    if (client != NULL && (client->attributes & KRB5_KDB_DISALLOW_FORWARDABLE))
+        flags &= ~TKT_FLG_FORWARDABLE;
+    if (server->attributes & KRB5_KDB_DISALLOW_FORWARDABLE)
+        flags &= ~TKT_FLG_FORWARDABLE;
+    if (header_enc != NULL && !(header_enc->flags & TKT_FLG_FORWARDABLE))
+        flags &= ~TKT_FLG_FORWARDABLE;
+
+    /* We don't currently handle issuing anonymous tickets based on
+     * non-anonymous ones. */
+    if (header_enc != NULL && !(header_enc->flags & TKT_FLG_ANONYMOUS))
+        flags &= ~TKT_FLG_ANONYMOUS;
+
+    return flags;
 }
 
 /* Return KRB5KDC_ERR_POLICY if indicators does not contain the required auth
diff --git a/src/kdc/kdc_util.h b/src/kdc/kdc_util.h
index 25077cbf5..1314bdd58 100644
--- a/src/kdc/kdc_util.h
+++ b/src/kdc/kdc_util.h
@@ -85,16 +85,15 @@ validate_as_request (kdc_realm_t *, krb5_kdc_req *, krb5_db_entry,
                      krb5_db_entry, krb5_timestamp,
                      const char **, krb5_pa_data ***);
 
-int
-validate_forwardable(krb5_kdc_req *, krb5_db_entry,
-                     krb5_db_entry, krb5_timestamp,
-                     const char **);
-
 int
 validate_tgs_request (kdc_realm_t *, krb5_kdc_req *, krb5_db_entry,
                       krb5_ticket *, krb5_timestamp,
                       const char **, krb5_pa_data ***);
 
+krb5_flags
+get_ticket_flags(krb5_flags reqflags, krb5_db_entry *client,
+                 krb5_db_entry *server, krb5_enc_tkt_part *header_enc);
+
 krb5_error_code
 check_indicators(krb5_context context, krb5_db_entry *server,
                  krb5_data *const *indicators);
diff --git a/src/kdc/tgs_policy.c b/src/kdc/tgs_policy.c
index 907fcd330..554345ba5 100644
--- a/src/kdc/tgs_policy.c
+++ b/src/kdc/tgs_policy.c
@@ -63,9 +63,9 @@ static check_tgs_svc_pol_fn * const svc_pol_fns[] = {
 };
 
 static const struct tgsflagrule tgsflagrules[] = {
-    { (KDC_OPT_FORWARDED | KDC_OPT_FORWARDABLE), TKT_FLG_FORWARDABLE,
+    { KDC_OPT_FORWARDED, TKT_FLG_FORWARDABLE,
       "TGT NOT FORWARDABLE", KDC_ERR_BADOPTION },
-    { (KDC_OPT_PROXY | KDC_OPT_PROXIABLE), TKT_FLG_PROXIABLE,
+    { KDC_OPT_PROXY, TKT_FLG_PROXIABLE,
       "TGT NOT PROXIABLE", KDC_ERR_BADOPTION },
     { (KDC_OPT_ALLOW_POSTDATE | KDC_OPT_POSTDATED), TKT_FLG_MAY_POSTDATE,
       "TGT NOT POSTDATABLE", KDC_ERR_BADOPTION },
@@ -98,12 +98,8 @@ check_tgs_opts(krb5_kdc_req *req, krb5_ticket *tkt, const char **status)
 }
 
 static const struct tgsflagrule svcdenyrules[] = {
-    { KDC_OPT_FORWARDABLE, KRB5_KDB_DISALLOW_FORWARDABLE,
-      "NON-FORWARDABLE TICKET", KDC_ERR_POLICY },
     { KDC_OPT_RENEWABLE, KRB5_KDB_DISALLOW_RENEWABLE,
       "NON-RENEWABLE TICKET", KDC_ERR_POLICY },
-    { KDC_OPT_PROXIABLE, KRB5_KDB_DISALLOW_PROXIABLE,
-      "NON-PROXIABLE TICKET", KDC_ERR_POLICY },
     { KDC_OPT_ALLOW_POSTDATE, KRB5_KDB_DISALLOW_POSTDATED,
       "NON-POSTDATABLE TICKET", KDC_ERR_CANNOT_POSTDATE },
     { KDC_OPT_ENC_TKT_IN_SKEY, KRB5_KDB_DISALLOW_DUP_SKEY,
diff --git a/src/tests/Makefile.in b/src/tests/Makefile.in
index c96c5d6b7..d2a37c616 100644
--- a/src/tests/Makefile.in
+++ b/src/tests/Makefile.in
@@ -171,6 +171,7 @@ check-pytests: unlockiter
 	$(RUNPYTEST) $(srcdir)/t_y2038.py $(PYTESTFLAGS)
 	$(RUNPYTEST) $(srcdir)/t_kdcpolicy.py $(PYTESTFLAGS)
 	$(RUNPYTEST) $(srcdir)/t_u2u.py $(PYTESTFLAGS)
+	$(RUNPYTEST) $(srcdir)/t_kdcoptions.py $(PYTESTFLAGS)
 
 clean:
 	$(RM) adata etinfo forward gcred hist hooks hrealm icinterleave icred
diff --git a/src/tests/gcred.c b/src/tests/gcred.c
index cb0ae6af5..b14e4fc9a 100644
--- a/src/tests/gcred.c
+++ b/src/tests/gcred.c
@@ -66,20 +66,32 @@ main(int argc, char **argv)
     krb5_principal client, server;
     krb5_ccache ccache;
     krb5_creds in_creds, *creds;
+    krb5_flags options = 0;
     char *name;
+    int c;
 
     check(krb5_init_context(&ctx));
 
-    /* Parse arguments. */
-    assert(argc == 3);
-    check(krb5_parse_name(ctx, argv[2], &server));
-    if (strcmp(argv[1], "unknown") == 0)
+    while ((c = getopt(argc, argv, "f")) != -1) {
+        switch (c) {
+        case 'f':
+            options |= KRB5_GC_FORWARDABLE;
+            break;
+        default:
+            abort();
+        }
+    }
+    argc -= optind;
+    argv += optind;
+    assert(argc == 2);
+    check(krb5_parse_name(ctx, argv[1], &server));
+    if (strcmp(argv[0], "unknown") == 0)
         server->type = KRB5_NT_UNKNOWN;
-    else if (strcmp(argv[1], "principal") == 0)
+    else if (strcmp(argv[0], "principal") == 0)
         server->type = KRB5_NT_PRINCIPAL;
-    else if (strcmp(argv[1], "srv-inst") == 0)
+    else if (strcmp(argv[0], "srv-inst") == 0)
         server->type = KRB5_NT_SRV_INST;
-    else if (strcmp(argv[1], "srv-hst") == 0)
+    else if (strcmp(argv[0], "srv-hst") == 0)
         server->type = KRB5_NT_SRV_HST;
     else
         abort();
@@ -89,7 +101,7 @@ main(int argc, char **argv)
     memset(&in_creds, 0, sizeof(in_creds));
     in_creds.client = client;
     in_creds.server = server;
-    check(krb5_get_credentials(ctx, 0, ccache, &in_creds, &creds));
+    check(krb5_get_credentials(ctx, options, ccache, &in_creds, &creds));
     check(krb5_unparse_name(ctx, creds->server, &name));
     printf("%s\n", name);
 
diff --git a/src/tests/t_kdcoptions.py b/src/tests/t_kdcoptions.py
new file mode 100644
index 000000000..7ec57508c
--- /dev/null
+++ b/src/tests/t_kdcoptions.py
@@ -0,0 +1,100 @@
+from k5test import *
+import re
+
+# KDC option test coverage notes:
+#
+# FORWARDABLE              here
+# FORWARDED                no test
+# PROXIABLE                here
+# PROXY                    no test
+# ALLOW_POSTDATE           no test
+# POSTDATED                no test
+# RENEWABLE                t_renew.py
+# CNAME_IN_ADDL_TKT        gssapi/t_s4u.py
+# CANONICALIZE             t_kdb.py and various other tests
+# REQUEST_ANONYMOUS        t_pkinit.py
+# DISABLE_TRANSITED_CHECK  no test
+# RENEWABLE_OK             t_renew.py
+# ENC_TKT_IN_SKEY          t_u2u.py
+# RENEW                    t_renew.py
+# VALIDATE                 no test
+
+# Run klist -f and return the flags on the ticket for svcprinc.
+def get_flags(realm, svcprinc):
+    grab_flags = False
+    for line in realm.run([klist, '-f']).splitlines():
+        if grab_flags:
+            return re.findall(r'Flags: ([a-zA-Z]*)', line)[0]
+        grab_flags = line.endswith(svcprinc)
+
+
+# Get the flags on the ticket for svcprinc, and check for an expected
+# element and an expected-absent element, either of which can be None.
+def check_flags(realm, svcprinc, expected_flag, expected_noflag):
+    flags = get_flags(realm, svcprinc)
+    if expected_flag is not None and not expected_flag in flags:
+        fail('expected flag ' + expected_flag)
+    if expected_noflag is not None and expected_noflag in flags:
+        fail('did not expect flag ' + expected_noflag)
+
+
+# Run kinit with the given flags, and check the flags on the resulting
+# TGT.
+def kinit_check_flags(realm, flags, expected_flag, expected_noflag):
+    realm.kinit(realm.user_princ, password('user'), flags)
+    check_flags(realm, realm.krbtgt_princ, expected_flag, expected_noflag)
+
+
+# Run kinit with kflags.  Then get credentials for the host principal
+# with gflags, and check the flags on the resulting ticket.
+def gcred_check_flags(realm, kflags, gflags, expected_flag, expected_noflag):
+    realm.kinit(realm.user_princ, password('user'), kflags)
+    realm.run(['./gcred'] + gflags + ['unknown', realm.host_princ])
+    check_flags(realm, realm.host_princ, expected_flag, expected_noflag)
+
+
+realm = K5Realm()
+
+mark('proxiable (AS)')
+kinit_check_flags(realm, [], None, 'P')
+kinit_check_flags(realm, ['-p'], 'P', None)
+realm.run([kadminl, 'modprinc', '-allow_proxiable', realm.user_princ])
+kinit_check_flags(realm, ['-p'], None, 'P')
+realm.run([kadminl, 'modprinc', '+allow_proxiable', realm.user_princ])
+realm.run([kadminl, 'modprinc', '-allow_proxiable', realm.krbtgt_princ])
+kinit_check_flags(realm, ['-p'], None, 'P')
+realm.run([kadminl, 'modprinc', '+allow_proxiable', realm.krbtgt_princ])
+
+mark('proxiable (TGS)')
+gcred_check_flags(realm, [], [], None, 'P')
+gcred_check_flags(realm, ['-p'], [], 'P', None)
+
+# Not tested: PROXIABLE option set with a non-proxiable TGT (because
+# there is no krb5_get_credentials() flag to request this; would
+# expect a non-proxiable ticket).
+
+# Not tested: proxiable TGT but PROXIABLE flag not set (because we
+# internally set the PROXIABLE option when using a proxiable TGT;
+# would expect a non-proxiable ticket).
+
+mark('forwardable (AS)')
+kinit_check_flags(realm, [], None, 'F')
+kinit_check_flags(realm, ['-f'], 'F', None)
+realm.run([kadminl, 'modprinc', '-allow_forwardable', realm.user_princ])
+kinit_check_flags(realm, ['-f'], None, 'F')
+realm.run([kadminl, 'modprinc', '+allow_forwardable', realm.user_princ])
+realm.run([kadminl, 'modprinc', '-allow_forwardable', realm.krbtgt_princ])
+kinit_check_flags(realm, ['-f'], None, 'F')
+realm.run([kadminl, 'modprinc', '+allow_forwardable', realm.krbtgt_princ])
+
+mark('forwardable (TGS)')
+realm.kinit(realm.user_princ, password('user'))
+gcred_check_flags(realm, [], [], None, 'F')
+gcred_check_flags(realm, [], ['-f'], None, 'F')
+gcred_check_flags(realm, ['-f'], [], 'F', None)
+
+# Not tested: forwardable TGT but FORWARDABLE flag not set (because we
+# internally set the FORWARDABLE option when using a forwardable TGT;
+# would expect a non-proxiable ticket).
+
+success('KDC option tests')