diff --git a/bin/tests/system/tkeyleak/ns1/dns.keytab b/bin/tests/system/tkeyleak/ns1/dns.keytab new file mode 100644 index 0000000000..d5a09b060a Binary files /dev/null and b/bin/tests/system/tkeyleak/ns1/dns.keytab differ diff --git a/bin/tests/system/tkeyleak/ns1/example.db.in b/bin/tests/system/tkeyleak/ns1/example.db.in new file mode 100644 index 0000000000..dd200dc9bc --- /dev/null +++ b/bin/tests/system/tkeyleak/ns1/example.db.in @@ -0,0 +1,21 @@ +; Copyright (C) Internet Systems Consortium, Inc. ("ISC") +; +; SPDX-License-Identifier: MPL-2.0 +; +; This Source Code Form is subject to the terms of the Mozilla Public +; License, v. 2.0. If a copy of the MPL was not distributed with this +; file, you can obtain one at https://mozilla.org/MPL/2.0/. +; +; See the COPYRIGHT file distributed with this work for additional +; information regarding copyright ownership. + +$TTL 300 +@ IN SOA ns.example. admin.example. ( + 1 ; serial + 3600 ; refresh + 900 ; retry + 604800 ; expire + 300 ; minimum + ) +@ IN NS ns.example. +ns IN A 10.53.0.1 diff --git a/bin/tests/system/tkeyleak/ns1/named.conf.j2 b/bin/tests/system/tkeyleak/ns1/named.conf.j2 new file mode 100644 index 0000000000..f16b53414c --- /dev/null +++ b/bin/tests/system/tkeyleak/ns1/named.conf.j2 @@ -0,0 +1,39 @@ +/* + * Copyright (C) Internet Systems Consortium, Inc. ("ISC") + * + * SPDX-License-Identifier: MPL-2.0 + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * See the COPYRIGHT file distributed with this work for additional + * information regarding copyright ownership. + */ + +options { + query-source address 10.53.0.1; + notify-source 10.53.0.1; + transfer-source 10.53.0.1; + port @PORT@; + pid-file "named.pid"; + listen-on { 10.53.0.1; }; + listen-on-v6 { none; }; + recursion no; + dnssec-validation no; + tkey-gssapi-keytab "dns.keytab"; +}; + +key rndc_key { + secret "1234abcd8765"; + algorithm @DEFAULT_HMAC@; +}; + +controls { + inet 10.53.0.1 port @CONTROLPORT@ allow { any; } keys { rndc_key; }; +}; + +zone "example" { + type primary; + file "example.db"; +}; diff --git a/bin/tests/system/tkeyleak/prereq.sh b/bin/tests/system/tkeyleak/prereq.sh new file mode 100644 index 0000000000..8a68ae7df1 --- /dev/null +++ b/bin/tests/system/tkeyleak/prereq.sh @@ -0,0 +1,21 @@ +#!/bin/sh + +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# SPDX-License-Identifier: MPL-2.0 +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. + +. ../conf.sh + +$FEATURETEST --gssapi || { + echo_i "gssapi not supported - skipping tkeyleak test" + exit 255 +} + +exit 0 diff --git a/bin/tests/system/tkeyleak/setup.sh b/bin/tests/system/tkeyleak/setup.sh new file mode 100644 index 0000000000..24a0026665 --- /dev/null +++ b/bin/tests/system/tkeyleak/setup.sh @@ -0,0 +1,17 @@ +#!/bin/sh -e + +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# SPDX-License-Identifier: MPL-2.0 +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. + +# shellcheck source=conf.sh +. ../conf.sh + +cp ns1/example.db.in ns1/example.db diff --git a/bin/tests/system/tkeyleak/tests_tkeyleak.py b/bin/tests/system/tkeyleak/tests_tkeyleak.py new file mode 100644 index 0000000000..fd97c8540c --- /dev/null +++ b/bin/tests/system/tkeyleak/tests_tkeyleak.py @@ -0,0 +1,145 @@ +# Copyright (C) Internet Systems Consortium, Inc. ("ISC") +# +# SPDX-License-Identifier: MPL-2.0 +# +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, you can obtain one at https://mozilla.org/MPL/2.0/. +# +# See the COPYRIGHT file distributed with this work for additional +# information regarding copyright ownership. + +""" +Regression test for GSS-API context leak via repeated TKEY queries. + +An unauthenticated attacker could exhaust server memory by sending +repeated TKEY queries with crafted SPNEGO NegTokenInit tokens. +Each query triggers gss_accept_sec_context() which returns +GSS_S_CONTINUE_NEEDED and allocates a GSS context. On the unfixed +code path, the context handle in process_gsstkey() is never stored +or freed, leaking ~520 bytes per query. + +The fix rejects GSS_S_CONTINUE_NEEDED in dst_gssapi_acceptctx() and +deletes the context immediately. + +The key distinguishing signal in the TKEY response: + - CONTINUE (vulnerable): error=0, output token present, no TSIG + - BADKEY (fixed): error=17, no output token +""" + +import struct +import time + +import dns.name +import dns.query +import dns.rdataclass +import dns.rdatatype +import dns.rdtypes.ANY.TKEY +import pytest + +import isctest + +pytestmark = pytest.mark.extra_artifacts( + [ + "*/*.db", + ] +) + +TKEY_NAME = dns.name.from_text("test.key.") +GSSAPI_ALGORITHM = dns.name.from_text("gss-tsig.") +TKEY_MODE_GSSAPI = 3 + +# OID 1.2.840.113554.1.2.2 (Kerberos 5) +KRB5_OID = b"\x06\x09\x2a\x86\x48\x86\xf7\x12\x01\x02\x02" + +# OID 1.3.6.1.5.5.2 (SPNEGO) +SPNEGO_OID = b"\x06\x06\x2b\x06\x01\x05\x05\x02" + + +def der_encode(tag, data): + """Encode data in ASN.1 DER TLV format.""" + length = len(data) + if length < 128: + return tag + bytes([length]) + data + if length < 256: + return tag + b"\x81" + bytes([length]) + data + return tag + b"\x82" + struct.pack(">H", length) + data + + +def spnego_negtokeninit(): + """Build a SPNEGO NegTokenInit proposing krb5 without a mechToken. + + This forces gss_accept_sec_context() to return GSS_S_CONTINUE_NEEDED + because the acceptor recognizes the krb5 mechanism but has not + received an actual AP-REQ token yet. + """ + # MechTypeList ::= SEQUENCE OF MechType + mechtype_list = der_encode(b"\x30", KRB5_OID) + # [0] mechTypes + mechtypes = der_encode(b"\xa0", mechtype_list) + # NegTokenInit ::= SEQUENCE { mechTypes, ... } + negtokeninit = der_encode(b"\x30", mechtypes) + # [0] CONSTRUCTED (wrapping NegTokenInit) + wrapped = der_encode(b"\xa0", negtokeninit) + # APPLICATION 0 CONSTRUCTED (SPNEGO OID + body) + return der_encode(b"\x60", SPNEGO_OID + wrapped) + + +def make_tkey_query(token): + """Build a TKEY query with a GSS-API token in the additional section.""" + now = int(time.time()) + tkey_rdata = dns.rdtypes.ANY.TKEY.TKEY( + rdclass=dns.rdataclass.ANY, + rdtype=dns.rdatatype.TKEY, + algorithm=GSSAPI_ALGORITHM, + inception=now, + expiration=now + 86400, + mode=TKEY_MODE_GSSAPI, + error=0, + key=token, + other=b"", + ) + + msg = isctest.query.create(TKEY_NAME, dns.rdatatype.TKEY, dns.rdataclass.ANY) + rrset = msg.find_rrset( + msg.additional, + TKEY_NAME, + dns.rdataclass.ANY, + dns.rdatatype.TKEY, + create=True, + ) + rrset.add(tkey_rdata) + return msg + + +def test_tkey_gssapi_no_continuation(ns1): + """TKEY with a SPNEGO NegTokenInit must be rejected, not continued. + + On unfixed code, gss_accept_sec_context() returns CONTINUE_NEEDED + and the response has error=0 with an output token (the leaked path). + On fixed code, CONTINUE_NEEDED is rejected and the response has + error=BADKEY(17) with no output token. + """ + port = ns1.ports.dns + ip = ns1.ip + + msg = make_tkey_query(spnego_negtokeninit()) + res = dns.query.tcp(msg, ip, port=port, timeout=5) + + assert res is not None + + tkey = get_tkey_answer(res) + assert tkey is not None, "server did not return a TKEY answer" + assert ( + tkey.error != 0 + ), "server returned error=0 (GSS_S_CONTINUE_NEEDED not rejected)" + assert len(tkey.key) == 0, "server returned a continuation token" + + +def get_tkey_answer(response): + """Extract TKEY rdata from a DNS response, or None.""" + for rrset in response.answer: + if rrset.rdtype == dns.rdatatype.TKEY: + for rdata in rrset: + return rdata + return None diff --git a/lib/dns/gssapictx.c b/lib/dns/gssapictx.c index b500e94cda..a2d55de3e0 100644 --- a/lib/dns/gssapictx.c +++ b/lib/dns/gssapictx.c @@ -336,7 +336,14 @@ dst_gssapi_initctx(const dns_name_t *name, isc_buffer_t *intoken, GSS_SPNEGO_MECHANISM, flags, 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", @@ -361,12 +368,6 @@ dst_gssapi_initctx(const dns_name_t *name, isc_buffer_t *intoken, CHECK(isc_buffer_copyregion(outtoken, &r)); } - if (gret == GSS_S_COMPLETE) { - result = ISC_R_SUCCESS; - } else { - result = DNS_R_CONTINUE; - } - cleanup: if (gouttoken.length != 0U) { (void)gss_release_buffer(&minor, &gouttoken); @@ -377,7 +378,7 @@ cleanup: isc_result_t dst_gssapi_acceptctx(const char *gssapi_keytab, isc_region_t *intoken, - isc_buffer_t **outtoken, dns_gss_ctx_id_t *ctxout, + isc_buffer_t **outtokenp, dns_gss_ctx_id_t *ctxout, dns_name_t *principal, isc_mem_t *mctx) { isc_region_t r; isc_buffer_t namebuf; @@ -389,16 +390,11 @@ dst_gssapi_acceptctx(const char *gssapi_keytab, isc_region_t *intoken, isc_result_t result; char buf[1024]; - REQUIRE(outtoken != NULL && *outtoken == NULL); + REQUIRE(outtokenp != NULL && *outtokenp == 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 HAVE_GSSAPI_GSSAPI_KRB5_H || HAVE_GSSAPI_KRB5_H gret = gsskrb5_register_acceptor_identity(gssapi_keytab); @@ -442,8 +438,15 @@ dst_gssapi_acceptctx(const char *gssapi_keytab, isc_region_t *intoken, 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: @@ -456,7 +459,7 @@ dst_gssapi_acceptctx(const char *gssapi_keytab, isc_region_t *intoken, 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))); @@ -467,49 +470,54 @@ dst_gssapi_acceptctx(const char *gssapi_keytab, isc_region_t *intoken, } if (gouttoken.length > 0U) { - isc_buffer_allocate(mctx, outtoken, + isc_buffer_allocate(mctx, outtokenp, (unsigned int)gouttoken.length); GBUFFER_TO_REGION(gouttoken, r); - CHECK(isc_buffer_copyregion(*outtoken, &r)); + CHECK(isc_buffer_copyregion(*outtokenp, &r)); (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))); - CLEANUP(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--; - } - - gss_log(3, "gss-api source name (accept) is %.*s", - (int)gnamebuf.length, (char *)gnamebuf.value); - - GBUFFER_TO_REGION(gnamebuf, r); - isc_buffer_init(&namebuf, r.base, r.length); - isc_buffer_add(&namebuf, r.length); - - CHECK(dns_name_fromtext(principal, &namebuf, dns_rootname, 0)); - } else { - result = DNS_R_CONTINUE; + 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))); + CLEANUP(ISC_R_FAILURE); } + /* + * 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--; + } + + gss_log(3, "gss-api source name (accept) is %.*s", (int)gnamebuf.length, + (char *)gnamebuf.value); + + GBUFFER_TO_REGION(gnamebuf, r); + isc_buffer_init(&namebuf, r.base, r.length); + isc_buffer_add(&namebuf, r.length); + + CHECK(dns_name_fromtext(principal, &namebuf, dns_rootname, 0)); + *ctxout = context; cleanup: + if (result != ISC_R_SUCCESS && *outtokenp != NULL) { + isc_buffer_free(outtokenp); + } + + 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) { diff --git a/lib/dns/include/dst/gssapi.h b/lib/dns/include/dst/gssapi.h index a519487da0..6551e014ce 100644 --- a/lib/dns/include/dst/gssapi.h +++ b/lib/dns/include/dst/gssapi.h @@ -71,18 +71,17 @@ dst_gssapi_acceptctx(const char *gssapi_keytab, isc_region_t *intoken, * 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 dns_gss_ctx_id_t. Subsequent calls - * should pass in the handle generated on the first call. * * 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 a65661670f..b22fed78a4 100644 --- a/lib/dns/tkey.c +++ b/lib/dns/tkey.c @@ -181,30 +181,22 @@ process_gsstkey(dns_message_t *msg, dns_name_t *name, dns_rdata_tkey_t *tkeyin, intoken = (isc_region_t){ tkeyin->key, tkeyin->keylen }; result = dst_gssapi_acceptctx(tctx->gssapi_keytab, &intoken, &outtoken, &gss_ctx, principal, tctx->mctx); - if (result == DNS_R_INVALIDTKEY) { - if (tsigkey != NULL) { - dns_tsigkey_detach(&tsigkey); - } + if (result != ISC_R_SUCCESS) { tkeyout->error = dns_tsigerror_badkey; tkey_log("process_gsstkey(): dns_tsigerror_badkey"); - return ISC_R_SUCCESS; - } - if (result != DNS_R_CONTINUE && result != ISC_R_SUCCESS) { - CHECK(result); + CLEANUP(ISC_R_SUCCESS); } /* - * 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. */ if (dns_name_countlabels(principal) == 0U) { - if (tsigkey != NULL) { - dns_tsigkey_detach(&tsigkey); - } - dst_gssapi_deletectx(tctx->mctx, &gss_ctx); tkeyout->error = dns_tsigerror_badkey; tkey_log("process_gsstkey(): " "completed context with empty principal"); - return ISC_R_SUCCESS; + CLEANUP(ISC_R_SUCCESS); } else if (tsigkey == NULL) { #if HAVE_GSSAPI OM_uint32 gret, minor, lifetime; @@ -283,7 +275,9 @@ cleanup: isc_buffer_free(&outtoken); } - tkey_log("process_gsstkey(): %s", isc_result_totext(result)); + if (result != ISC_R_SUCCESS) { + tkey_log("process_gsstkey(): %s", isc_result_totext(result)); + } return result; } @@ -678,9 +672,8 @@ dns_tkey_gssnegotiate(dns_message_t *qmsg, dns_message_t *rmsg, 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. */ CHECK(dns_tsigkey_createfromkey(tkeyname, DST_ALG_GSSAPI, dstkey, true, false, NULL, rtkey.inception,