Fix GSS-API context leak in TKEY negotiation

Reject multi-round GSS-API negotiation (GSS_S_CONTINUE_NEEDED) in
dst_gssapi_acceptctx().  Each call to gss_accept_sec_context()
allocates a context inside the GSS library; without this fix, the
context handle was passed back to process_gsstkey() which did not
store it persistently, leaking it on every incomplete negotiation.

An unauthenticated attacker could exhaust server memory by sending
repeated TKEY queries with GSSAPI tokens, each leaking one GSS
context.  The leaked memory is allocated by the GSS library via
malloc(), bypassing BIND's memory accounting.

In practice, Kerberos/SPNEGO (the only mechanism used with BIND)
completes in a single round, so rejecting continuation does not
affect real-world deployments.  See RFC 3645 Section 4.1.3.

(cherry picked from commit 3883058bf284de2889e4e3676767e58ad91a0ae3)

diff --git a/lib/dns/gssapictx.c b/lib/dns/gssapictx.c
index 482c25e1cccb9aeeb92afcbebc341bbc26f9b18f..fabdef5ea637532b6db4316e405f6bef832da69f 100644 (file)
--- a/lib/dns/gssapictx.c
+++ b/lib/dns/gssapictx.c
@@ -594,7 +594,14 @@ dst_gssapi_initctx(dns_name_t *name, isc_buffer_t *intoken,
                                    0, NULL, gintokenp,
                                    NULL, &gouttoken, &ret_flags, NULL);
 
-       if (gret != GSS_S_COMPLETE && gret != GSS_S_CONTINUE_NEEDED) {
+       switch (gret) {
+       case GSS_S_COMPLETE:
+               result = ISC_R_SUCCESS;
+               break;
+       case GSS_S_CONTINUE_NEEDED:
+               result = DNS_R_CONTINUE;
+               break;
+       default:
                gss_err_message(mctx, gret, minor, err_message);
                if (err_message != NULL && *err_message != NULL)
                        gss_log(3, "Failure initiating security context: %s",
@@ -619,14 +626,10 @@ dst_gssapi_initctx(dns_name_t *name, isc_buffer_t *intoken,
                RETERR(isc_buffer_copyregion(outtoken, &r));
        }
 
-       if (gret == GSS_S_COMPLETE)
-               result = ISC_R_SUCCESS;
-       else
-               result = DNS_R_CONTINUE;
-
- out:
-       if (gouttoken.length != 0U)
+out:
+       if (gouttoken.length != 0U) {
                (void)gss_release_buffer(&minor, &gouttoken);
+       }
        (void)gss_release_name(&minor, &gname);
        return (result);
 }
@@ -648,14 +651,10 @@ dst_gssapi_acceptctx(gss_cred_id_t cred,
        char buf[1024];
 
        REQUIRE(outtoken != NULL && *outtoken == NULL);
+       REQUIRE(*ctxout == NULL);
 
        REGION_TO_GBUFFER(*intoken, gintoken);
 
-       if (*ctxout == NULL)
-               context = GSS_C_NO_CONTEXT;
-       else
-               context = *ctxout;
-
        if (gssapi_keytab != NULL) {
 #if defined(ISC_PLATFORM_GSSAPI_KRB5_HEADER) || defined(WIN32)
                gret = gsskrb5_register_acceptor_identity(gssapi_keytab);
@@ -697,8 +696,15 @@ dst_gssapi_acceptctx(gss_cred_id_t cred,
 
        switch (gret) {
        case GSS_S_COMPLETE:
-       case GSS_S_CONTINUE_NEEDED:
                break;
+       /*
+        * RFC 3645 4.1.3: we don't handle GSS_S_CONTINUE_NEEDED
+        * Multi-round GSS-API negotiation is not supported.
+        */
+       case GSS_S_CONTINUE_NEEDED:
+               gss_log(3, "multi-round GSS-API negotiation not supported");
+               (void)gss_delete_sec_context(&minor, &context, NULL);
+               /* FALLTHROUGH */
        case GSS_S_DEFECTIVE_TOKEN:
        case GSS_S_DEFECTIVE_CREDENTIAL:
        case GSS_S_BAD_SIG:
@@ -711,7 +717,7 @@ dst_gssapi_acceptctx(gss_cred_id_t cred,
        case GSS_S_BAD_MECH:
        case GSS_S_FAILURE:
                result = DNS_R_INVALIDTKEY;
-               /* fall through */
+               /* FALLTHROUGH */
        default:
                gss_log(3, "failed gss_accept_sec_context: %s",
                        gss_error_tostring(gret, minor, buf, sizeof(buf)));
@@ -729,48 +735,56 @@ dst_gssapi_acceptctx(gss_cred_id_t cred,
                (void)gss_release_buffer(&minor, &gouttoken);
        }
 
-       if (gret == GSS_S_COMPLETE) {
-               gret = gss_display_name(&minor, gname, &gnamebuf, NULL);
-               if (gret != GSS_S_COMPLETE) {
-                       gss_log(3, "failed gss_display_name: %s",
-                               gss_error_tostring(gret, minor,
-                                                  buf, sizeof(buf)));
-                       RETERR(ISC_R_FAILURE);
-               }
+       INSIST(gret == GSS_S_COMPLETE);
 
-               /*
-                * Compensate for a bug in Solaris8's implementation
-                * of gss_display_name().  Should be harmless in any
-                * case, since principal names really should not
-                * contain null characters.
-                */
-               if (gnamebuf.length > 0U &&
-                   ((char *)gnamebuf.value)[gnamebuf.length - 1] == '\0')
-                       gnamebuf.length--;
+       gret = gss_display_name(&minor, gname, &gnamebuf, NULL);
+       if (gret != GSS_S_COMPLETE) {
+               gss_log(3, "failed gss_display_name: %s",
+                       gss_error_tostring(gret, minor, buf, sizeof(buf)));
+               result = ISC_R_FAILURE;
+               goto out;
+       }
 
-               gss_log(3, "gss-api source name (accept) is %.*s",
-                       (int)gnamebuf.length, (char *)gnamebuf.value);
+       /*
+        * Compensate for a bug in Solaris8's implementation
+        * of gss_display_name().  Should be harmless in any
+        * case, since principal names really should not
+        * contain null characters.
+        */
+       if (gnamebuf.length > 0U &&
+           ((char *)gnamebuf.value)[gnamebuf.length - 1] == '\0')
+       {
+               gnamebuf.length--;
+       }
 
-               GBUFFER_TO_REGION(gnamebuf, r);
-               isc_buffer_init(&namebuf, r.base, r.length);
-               isc_buffer_add(&namebuf, r.length);
+       gss_log(3, "gss-api source name (accept) is %.*s", (int)gnamebuf.length,
+               (char *)gnamebuf.value);
 
-               RETERR(dns_name_fromtext(principal, &namebuf, dns_rootname,
-                                        0, NULL));
+       GBUFFER_TO_REGION(gnamebuf, r);
+       isc_buffer_init(&namebuf, r.base, r.length);
+       isc_buffer_add(&namebuf, r.length);
 
-               if (gnamebuf.length != 0U) {
-                       gret = gss_release_buffer(&minor, &gnamebuf);
-                       if (gret != GSS_S_COMPLETE)
-                               gss_log(3, "failed gss_release_buffer: %s",
-                                       gss_error_tostring(gret, minor, buf,
-                                                          sizeof(buf)));
-               }
-       } else
-               result = DNS_R_CONTINUE;
+       result = dns_name_fromtext(principal, &namebuf, dns_rootname, 0, NULL);
+       if (result != ISC_R_SUCCESS) {
+               goto out;
+       }
 
        *ctxout = context;
 
- out:
+out:
+       if (result != ISC_R_SUCCESS && context != GSS_C_NO_CONTEXT) {
+               (void)gss_delete_sec_context(&minor, &context, NULL);
+       }
+
+       if (gnamebuf.length != 0U) {
+               gret = gss_release_buffer(&minor, &gnamebuf);
+               if (gret != GSS_S_COMPLETE) {
+                       gss_log(3, "failed gss_release_buffer: %s",
+                               gss_error_tostring(gret, minor, buf,
+                                                  sizeof(buf)));
+               }
+       }
+
        if (gname != NULL) {
                gret = gss_release_name(&minor, &gname);
                if (gret != GSS_S_COMPLETE)

diff --git a/lib/dns/include/dst/gssapi.h b/lib/dns/include/dst/gssapi.h
index 8e31587871e801266fa5e92f34ec8b584188630a..bf1c4c959aadb0233bce22593f67f9cf6c227e54 100644 (file)
--- a/lib/dns/include/dst/gssapi.h
+++ b/lib/dns/include/dst/gssapi.h
@@ -126,20 +126,17 @@ dst_gssapi_acceptctx(gss_cred_id_t cred,
  *                generated by gss_accept_sec_context() to be sent to the
  *                initiator
  *      'context'  is a valid pointer to receive the generated context handle.
- *                 On the initial call, it should be a pointer to NULL, which
- *                will be allocated as a gss_ctx_id_t.  Subsequent calls
- *                should pass in the handle generated on the first call.
- *                Call dst_gssapi_releasecred to delete the context and free
- *                the memory.
  *
  *     Requires:
- *             'outtoken' to != NULL && *outtoken == NULL.
+ *             'outtoken' != NULL && *outtoken == NULL.
+ *             'context'  != NULL && *context  == NULL.
  *
  *     Returns:
- *             ISC_R_SUCCESS   msg was successfully updated to include the
- *                             query to be sent
- *             DNS_R_CONTINUE  transaction still in progress
- *             other           an error occurred while building the message
+ *             ISC_R_SUCCESS           msg was successfully updated to include
+ *                                     the query to be sent
+ *             DNS_R_INVALIDTKEY       an error occurred while accepting the
+ *                                     context
+ *             ISC_R_FAILURE           other error occurred
  */
 
 isc_result_t

diff --git a/lib/dns/tkey.c b/lib/dns/tkey.c
index fdd54efb1ee08d0aaa2eb7e0ace2d69f2f176895..5edd21220d25c02774409758f492f817727534e0 100644 (file)
--- a/lib/dns/tkey.c
+++ b/lib/dns/tkey.c
@@ -494,18 +494,9 @@ process_gsstkey(dns_message_t *msg, dns_name_t *name, dns_rdata_tkey_t *tkeyin,
                return (ISC_R_SUCCESS);
        }
 
-       /*
-        * XXXDCL need to check for key expiry per 4.1.1
-        * XXXDCL need a way to check fully established, perhaps w/key_flags
-        */
-
        intoken.base = tkeyin->key;
        intoken.length = tkeyin->keylen;
 
-       result = dns_tsigkey_find(&tsigkey, name, &tkeyin->algorithm, ring);
-       if (result == ISC_R_SUCCESS)
-               gss_ctx = dst_key_getgssctx(tsigkey->key);
-
        principal = dns_fixedname_initname(&fixed);
 
        /*
@@ -514,17 +505,18 @@ process_gsstkey(dns_message_t *msg, dns_name_t *name, dns_rdata_tkey_t *tkeyin,
        result = dst_gssapi_acceptctx(tctx->gsscred, tctx->gssapi_keytab,
                                      &intoken, &outtoken, &gss_ctx,
                                      principal, tctx->mctx);
-       if (result == DNS_R_INVALIDTKEY) {
+       if (result != ISC_R_SUCCESS) {
                if (tsigkey != NULL)
                        dns_tsigkey_detach(&tsigkey);
                tkeyout->error = dns_tsigerror_badkey;
                tkey_log("process_gsstkey(): dns_tsigerror_badkey");    /* XXXSRA */
                return (ISC_R_SUCCESS);
        }
-       if (result != DNS_R_CONTINUE && result != ISC_R_SUCCESS)
-               goto failure;
+
        /*
-        * XXXDCL Section 4.1.3: Limit GSS_S_CONTINUE_NEEDED to 10 times.
+        * Multi-round GSS-API negotiation (GSS_S_CONTINUE_NEEDED) is
+        * rejected in dst_gssapi_acceptctx(), so if we reach here the
+        * negotiation is complete and the principal must be set.
         */
 
        isc_stdtime_get(&now);
@@ -603,6 +595,10 @@ process_gsstkey(dns_message_t *msg, dns_name_t *name, dns_rdata_tkey_t *tkeyin,
        return (ISC_R_SUCCESS);
 
 failure:
+       if (dstkey == NULL && gss_ctx != NULL) {
+               dst_gssapi_deletectx(tctx->mctx, &gss_ctx);
+       }
+
        if (tsigkey != NULL)
                dns_tsigkey_detach(&tsigkey);
 
@@ -1512,9 +1508,8 @@ dns_tkey_gssnegotiate(dns_message_t *qmsg, dns_message_t *rmsg,
                                  &dstkey, NULL));
 
        /*
-        * XXXSRA This seems confused.  If we got CONTINUE from initctx,
-        * the GSS negotiation hasn't completed yet, so we can't sign
-        * anything yet.
+        * GSS negotiation is complete (CONTINUE returned earlier).
+        * Create the TSIG key from the established context.
         */
 
        RETERR(dns_tsigkey_createfromkey(tkeyname,