New option [libdefaults] block_dns = true to block all DNS queries. #1155

riastradh · 2023-06-09T01:22:01Z

(draft pending automatic tests)

riastradh · 2023-06-15T00:10:04Z

@nicowilliams Is there an example of a libkrb5 test that sets krb5.conf that I can easily parrot?

(I hope the existing tests don't behave differently depending on what is in /etc/krb5.conf or ~/.krb5/config?)

nicowilliams · 2023-06-15T06:28:48Z

@riastradh yes: lib/krb5/test_cc.c, which uses krb5_set_config() to set a configuration as if it'd been read from a file.

kadmin/kadm_conn.c

nicowilliams · 2023-06-22T01:10:22Z

kdc/altsecid_gss_preauth_authorizer.c

+        krb5_set_error_message(context, ret, "DNS blocked when finding AD DC");
+	return ret;
+    }
+


@lhoward do you want a config param here for specifying LDAP servers to use?

lib/krb5/get_addrs.c

nicowilliams · 2023-06-22T01:13:46Z

lib/krb5/krbhst.c

@@ -718,6 +728,13 @@ plugin_get_hosts(krb5_context context,
 {
    struct plctx ctx = { type, kd, 0 };

+    /*
+     * XXX Need a way to pass this through -- unsure if any of this is


The plugins get the krb5_context, so they get to check this parameter. I say let's trust them.

Do they? How does it get passed through here?

heimdal/lib/krb5/krbhst.c

Lines 687 to 701 in 8ac4266

static KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL

plcallback(krb5_context context,

const void *plug, void *plugctx, void *userctx)

{

const krb5plugin_service_locate_ftable *locate = plug;

struct plctx *plctx = userctx;

if (locate->minor_version >= KRB5_PLUGIN_LOCATE_VERSION_2)

return locate->lookup(plugctx, plctx->flags, plctx->type, plctx->kd->realm, 0, 0, add_locate, plctx->kd);

if (plctx->flags & KRB5_PLF_ALLOW_HOMEDIR)

return locate->old_lookup(plugctx, plctx->type, plctx->kd->realm, 0, 0, add_locate, plctx->kd);

return KRB5_PLUGIN_NO_HANDLE;

}

Oy, yeah, most of our plugins do pass the context -- I'd just assumed we did it here too. You can add flags like KD_LARGE_MSG and set them on the flags field of the instance of struct krb5_krbhst_data that plctx->kd points to.

nicowilliams · 2023-06-22T01:17:03Z

I'm not sure that trying to get getaddrinfo() and gethostbyname() to not use DNS is a worthy goal -- I'm not sure that we can at all, and maybe if we can then not portably. We can stop at not doing SRV and TXT (and, someday, URI) resource record lookups.

riastradh · 2023-06-22T01:22:53Z

I'm not sure that trying to get getaddrinfo() and gethostbyname() to not use DNS is a worthy goal -- I'm not sure that we can at all, and maybe if we can then not portably.

Why would getaddrinfo with AI_NUMERICHOST and no AI_CANONNAME ever do DNS lookups? Are there any serious implementations that do more than just parse the IP address (i.e., inet_pton) in that case?

If there are, I could change it to call inet_pton explicitly instead -- somewhat more involved change but perhaps more confidence-inspiring.

We can stop at not doing SRV and TXT (and, someday, URI) resource record lookups.

No, that defeats the purpose -- my goal for #1154 is to guarantee that Heimdal leaks nothing into DNS queries. This is critical for location privacy. (Not sufficient -- that will also require #1151 -- but necessary.)

nicowilliams · 2023-06-22T01:46:59Z

Why would getaddrinfo with AI_NUMERICHOST and no AI_CANONNAME ever do DNS lookups? Are there any serious implementations that do more than just parse the IP address (i.e., inet_pton) in that case?

If you don't pass an IP to getaddrinfo() I still expect it to look up the given name's addresses.

We can stop at not doing SRV and TXT (and, someday, URI) resource record lookups.

No, that defeats the purpose -- my goal for #1154 is to guarantee that Heimdal leaks nothing into DNS queries. This is critical for location privacy. (Not sufficient -- that will also require #1151 -- but necessary.)

What I'm saying is that is that leaving out AI_CANONNAME and adding AI_NUMERICHOST and AI_NUMERICSERV ~~is not enough~~ may not be enough: you have to also make sure that the "name" passed to getaddrinfo() is an IP address. Granted, anyone who configures block_dns = true probably would also configure KDCs in /etc/krb5.conf using only IPs. But maybe you're right that inet_pton() is the right option here.

EDIT: But I'm ok with documenting that if you set block_dns = true then you must use IPs for KDCs and such in /etc/krb5.conf.

riastradh · 2023-06-22T18:18:40Z

Why would getaddrinfo with AI_NUMERICHOST and no AI_CANONNAME ever do DNS lookups? Are there any serious implementations that do more than just parse the IP address (i.e., inet_pton) in that case?

If you don't pass an IP to getaddrinfo() I still expect it to look up the given name's addresses.

What effect is AI_NUMERICHOST supposed to have, then?

From POSIX:

If the AI_NUMERICHOST flag is specified, then a non-null nodename string supplied shall be a numeric host address string. Otherwise, an [EAI_NONAME] error is returned. This flag shall prevent any type of name resolution service (for example, the DNS) from being invoked.

If the AI_NUMERICSERV flag is specified, then a non-null servname string supplied shall be a numeric port string. Otherwise, an [EAI_NONAME] error shall be returned. This flag shall prevent any type of name resolution service (for example, NIS+) from being invoked.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/getaddrinfo.html

I'm not sure if AI_CANONNAME actually matters when AI_NUMERICHOST is passed; I put it in there out of paranoia in case some clever implementation decided to look up a PTR record and then chase CNAMEs of the result. However, I suspect that part is unnecessary, because POSIX also prescribes:

A numeric host address string is not a "name", and thus does not have a "canonical name" form; no address to host name translation is performed.

EDIT: But I'm ok with documenting that if you set block_dns = true then you must use IPs for KDCs and such in /etc/krb5.conf.

Not quite. With just this change, yes: hostnames instead of IP addresses won't work (but will fail safe without DNS leaks) after this change. But after adding SOCKS proxy support (and/or HTTP CONNECT proxy support), hostnames can be passed through verbatim to the proxy without any DNS resolution beforehand.

nicowilliams · 2023-06-22T18:56:32Z

OK, thanks, that resolves my getaddrinfo() questions.

All network traffic to KDC goes through the SOCKS4a proxy if it is configured. This is deliberately kept simple -- and is not generalized to SOCKS4 or SOCKS5 or other types of proxies -- so it is easy to audit for network and DNS leaks. (SOCKS5 might be OK but takes more work to implement.) XXX Need to audit Heimdal for other kinds of traffic too. XXX Need to combine with heimdal#1155 to plug DNS leaks. XXX Need to figure out where the socks4a.c code should go. fix heimdal#1151

All network traffic to KDC goes through the SOCKS4a proxy if it is configured. This is deliberately kept simple -- and is not generalized to SOCKS4 or SOCKS5 or other types of proxies -- so it is easy to audit for network and DNS leaks. (SOCKS4 works in IP addresses, and so invites DNS leaks. SOCKS5 can be OK, if used judiciously, but takes more work to implement.) XXX Need to audit Heimdal for other kinds of traffic too outside libkrb5. Subsequent changes on this branch will address other parts of libkrb5. XXX Need to combine with heimdal#1155 to plug DNS leaks. XXX Need to figure out where the socks4a.c code should go. fix heimdal#1151

nicowilliams · 2024-01-04T02:16:13Z

Is this ready? It's marked draft.

All network traffic to KDC goes through the SOCKS4a proxy if it is configured. This is deliberately kept simple -- and is not generalized to SOCKS4 or SOCKS5 or other types of proxies -- so it is easy to audit for network and DNS leaks. (SOCKS4 works in IP addresses, and so invites DNS leaks. SOCKS5 can be OK, if used judiciously, but takes more work to implement.) XXX Need to audit Heimdal for other kinds of traffic too outside libkrb5. Subsequent changes on this branch will address other parts of libkrb5. XXX Need to combine with heimdal#1155 to plug DNS leaks. XXX Need to figure out where the socks4a.c code should go. fix heimdal#1151

riastradh · 2024-01-04T23:22:46Z

Is this ready? It's marked draft.

Other than the conflict resolution needed since merging #1206 (should be quick & easy), this doesn't have any automatic tests at the moment. I manually tested it, and verified no DNS queries with ktrace, back when I drafted it originally. Up to you to decide whether that's acceptable!

All network traffic to KDC goes through the SOCKS4a proxy if it is configured. This is deliberately kept simple -- and is not generalized to SOCKS4 or SOCKS5 or other types of proxies -- so it is easy to audit for network and DNS leaks. (SOCKS4 works in IP addresses, and so invites DNS leaks. SOCKS5 can be OK, if used judiciously, but takes more work to implement.) XXX Need to audit Heimdal for other kinds of traffic too outside libkrb5. Subsequent changes on this branch will address other parts of libkrb5. XXX Need to combine with heimdal#1155 to plug DNS leaks. XXX Need to figure out where the socks4a.c code should go. fix heimdal#1151

nicowilliams · 2024-01-04T23:35:01Z

To test this sort of thing we'd really need to use containers in the tests (unless already running in a container) so we can start a DNS server.

All network traffic to KDC goes through the SOCKS4a proxy if it is configured. This is deliberately kept simple -- and is not generalized to SOCKS4 or SOCKS5 or other types of proxies -- so it is easy to audit for network and DNS leaks. (SOCKS4 works in IP addresses, and so invites DNS leaks. SOCKS5 can be OK, if used judiciously, but takes more work to implement.) XXX Need to audit Heimdal for other kinds of traffic too outside libkrb5. Subsequent changes on this branch will address other parts of libkrb5. XXX Need to combine with heimdal#1155 to plug DNS leaks. XXX Need to figure out where the socks4a.c code should go. fix heimdal#1151

riastradh · 2024-01-05T01:22:45Z

To test this sort of thing we'd really need to use containers in the tests (unless already running in a container) so we can start a DNS server.

Not really -- a good test would be to make some krb5_* or GSS-API calls in an executable program that defines the getaddrinfo symbol to call abort() if it doesn't have AI_NUMERICHOST|AI_NUMERICSERV set (and to call inet_pton otherwise to implement the usual functionality).

It's not quite the same audit as ktracing and filtering for networking syscalls, but it should work on any elven platform.

Documented and verified, not yet implemented.

If block_dns is set, call getaddrinfo with AI_NUMERICHOST set and AI_CANONNAME clear. Some paths may not have set AI_CANONNAME, but it's easier to audit this way when the getaddrinfo prelude is uniform across call sites, and the compiler can optimize it away.

Exception: In lib/kafs/common.c, we don't have a krb5_context in which to check.

Previously, the hostname was initialized to `localhost'. If it was not cleared by init_syslog, init_logger_addr (via openlog) would query gethostbyname to find the IP address of `localhost', which will essentially always be 127.0.0.1. But if it was cleared by init_syslog, init_logger_addr would return 127.0.0.1 anyway. This way, it always returns 127.0.0.1 in the event of no init_syslog call, and avoids a DNS lookup. You can always force a DNS lookup by passing `localhost' to init_syslog explicitly, of course. I'm not sure if anything even uses this as a fallback in Heimdal, but let's avoid leaving a rake to step on.

nicowilliams · 2024-01-05T17:30:45Z

Not really -- a good test would be to make some krb5_* or GSS-API calls in an executable program that defines the getaddrinfo symbol to call abort() if it doesn't have AI_NUMERICHOST|AI_NUMERICSERV set (and to call inet_pton otherwise to implement the usual functionality).

It's not quite the same audit as ktracing and filtering for networking syscalls, but it should work on any elven platform.

I'm not sure that symbol interposition will work reliably on all platforms (Windows/MSYS/Cygwin?), but, yes, it's an idea.

The fact that we generally can't rely on DNS servers on non-standard ports when using the system resolver means we eventually have to aim for a container-ish test scheme. An alternative would be to have our own resolver (which is a scary prospect, though it would have a number of benefits, but mainly it would be a tremendous time suck when we already don't have the time for it).

riastradh · 2024-01-05T18:54:09Z

If you're happy accepting this change without automatic tests, fine by me -- can add automatic tests in a subsequent change.

nicowilliams · 2024-01-05T19:30:18Z

Maybe we can have a very simple interposition-based test after all. If interposition fails then no problem: the test passes, and if interposition succeeds then the test will work. And we can add a test that interposition does work so that we can know from the test logs.

We build variants of kinit and test_acquire_cred that define their own symbols rk_dns_lookup, gethostbyname, gethostbyname2, and getaddrinfo to print a message and abort. For getaddrinfo, we abort only if the caller failed to specify AI_NUMERICHOST; otherwise we use dlsym(RTLD_NEXT, "getaddrinfo") instead. The new test tests/gss/check-nodns is like tests/gss/check-basic, but uses kinit_auditdns and test_acquire_cred_auditdns to verify that no DNS resolution happens. This test should work and be effective on ELF platforms where the getaddrinfo function is implemented by the symbol `getaddrinfo'. On non-ELF platforms it may not be effective -- and on platforms where the getaddrinfo function is implemented by another symbol (like `__getaddrinfo50') it may not work, but we can cross that bridge when we come to it. Verified manually that the test fails, with the expected error message and abort, without `block_dns = yes' in krb5-nodns.conf. No automatic test of the mechanism for now because it might not work on some platforms. XXX check-nodns.in is copypasta of check-basic.in, should factor out the common parts so they don't get out of sync.

riastradh · 2024-01-07T21:58:45Z

I drafted an automatic test that uses the symbol interposition technique I described above:

New source file appl/test/auditdns.c implements stubs for rk_dns_lookup, gethostbyname, gethostbyname2, and getaddrinfo. (For getaddrinfo, we allow calls with AI_NUMERICHOST and no AI_CANONNAME, and use dlsym(RTLD_NEXt, "getaddrinfo") to call the real one. Could maybe use inet_pton instead to make this more robust in case the symbol name for the getaddrinfo function changes.)
New non-installed tools kinit_auditdns, test_acquire_cred_auditdns are like the non-auditdns ones but have auditdns.o linked in.
New test tests/gss/check-nodns is just like tests/gss/check-basic but uses kinit_auditdns and test_acquire_cred_auditdns, and uses block_dns = yes and 127.0.0.1 instead of localhost in the krb5 config.

This test could stand some tidying, so if you want to merge the bulk of the branch first and defer the test to another PR to be merged separately, that would be reasonable.

All network traffic to KDC goes through the SOCKS4a proxy if it is configured. This is deliberately kept simple -- and is not generalized to SOCKS4 or SOCKS5 or other types of proxies -- so it is easy to audit for network and DNS leaks. (SOCKS4 works in IP addresses, and so invites DNS leaks. SOCKS5 can be OK, if used judiciously, but takes more work to implement.) XXX Need to audit Heimdal for other kinds of traffic too outside libkrb5. Subsequent changes on this branch will address other parts of libkrb5. XXX Need to combine with heimdal#1155 to plug DNS leaks. XXX Need to figure out where the socks4a.c code should go. fix heimdal#1151

appl/test/auditdns.c

nicowilliams · 2024-01-08T17:22:11Z

@riastradh it's been merged. Arguably tests/kdc/ would have been a better place for this test since you're not modifying GSS code, but that's ok.

riastradh · 2024-01-08T17:28:28Z

@riastradh it's been merged. Arguably tests/kdc/ would have been a better place for this test since you're not modifying GSS code, but that's ok.

Thanks! The two main things that are critical to test are:

kinit to get tgt from kdc
gssapi session establishment to get service ticket from kdc

Copying tests/gss/check-basic seemed like an easier path to testing both of those, with a minimum of unnecessary other testing, than anything I found under tests/kdc. But I'm not attached to this arrangement; feel free to move it around.

jsutton24 · 2024-02-13T23:35:30Z

lib/krb5/krbhst.c

-    if(context->srv_lookup) {
+    if (krb5_config_get_bool(context, NULL, "libdefaults", "block_dns",
+	    NULL) &&
+	context->srv_lookup) {


Why are we running the following code only if block_dns is true? Correct me if I’m wrong, but shouldn’t this condition (and the following two) be the other way round?

I’ll submit a PR to fix this.

Oops -- yes, I think I got this backwards, thanks for the review!

riastradh force-pushed the riastradh-20230608-issue1154-blockdns branch from 116339b to b8a3b9b Compare June 21, 2023 23:04

nicowilliams reviewed Jun 22, 2023

View reviewed changes

kadmin/kadm_conn.c Show resolved Hide resolved

nicowilliams reviewed Jun 22, 2023

View reviewed changes

lib/krb5/get_addrs.c Show resolved Hide resolved

nicowilliams reviewed Jun 22, 2023

View reviewed changes

riastradh mentioned this pull request Jan 2, 2024

New option [libdefaults] socks4a_proxy. #1204

Open

riastradh force-pushed the riastradh-20230608-issue1154-blockdns branch from b8a3b9b to 5c9f0de Compare January 3, 2024 02:21

Taylor R Campbell added 4 commits January 5, 2024 02:05

krb5.conf(5): New block_dns option.

bffcc9f

Documented and verified, not yet implemented.

Ensure all calls to rk_dns_lookup are headed by a block_dns check.

7ef516b

Exception: In lib/kafs/common.c, we don't have a krb5_context in which to check.

riastradh force-pushed the riastradh-20230608-issue1154-blockdns branch from 5c9f0de to 90bfc48 Compare January 5, 2024 03:38

riastradh marked this pull request as ready for review January 5, 2024 18:53

nicowilliams reviewed Jan 8, 2024

View reviewed changes

appl/test/auditdns.c Show resolved Hide resolved

nicowilliams merged commit ad23636 into heimdal:master Jan 8, 2024
6 checks passed

riastradh mentioned this pull request Jan 9, 2024

block_dns as implemented fails to block reverse DNS (PTR) lookups #1214

Closed

2 tasks

jsutton24 reviewed Feb 13, 2024

View reviewed changes

jsutton24 mentioned this pull request Feb 18, 2024

krb5: Correct inverted conditions #1230

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New option [libdefaults] block_dns = true to block all DNS queries. #1155

New option [libdefaults] block_dns = true to block all DNS queries. #1155

riastradh commented Jun 9, 2023

riastradh commented Jun 15, 2023

nicowilliams commented Jun 15, 2023

nicowilliams Jun 22, 2023

nicowilliams Jun 22, 2023

riastradh Jun 22, 2023

nicowilliams Jun 22, 2023

nicowilliams commented Jun 22, 2023 •

edited

Loading

riastradh commented Jun 22, 2023 •

edited

Loading

nicowilliams commented Jun 22, 2023 •

edited

Loading

riastradh commented Jun 22, 2023 •

edited

Loading

nicowilliams commented Jun 22, 2023

nicowilliams commented Jan 4, 2024

riastradh commented Jan 4, 2024

nicowilliams commented Jan 4, 2024

riastradh commented Jan 5, 2024

nicowilliams commented Jan 5, 2024

riastradh commented Jan 5, 2024

nicowilliams commented Jan 5, 2024

riastradh commented Jan 7, 2024

nicowilliams commented Jan 8, 2024

riastradh commented Jan 8, 2024

jsutton24 Feb 13, 2024 •

edited

Loading

jsutton24 Feb 18, 2024

riastradh Feb 18, 2024

	static KRB5_LIB_FUNCTION krb5_error_code KRB5_LIB_CALL
	plcallback(krb5_context context,
	const void plug, void plugctx, void *userctx)
	{
	const krb5plugin_service_locate_ftable *locate = plug;
	struct plctx *plctx = userctx;

	if (locate->minor_version >= KRB5_PLUGIN_LOCATE_VERSION_2)
	return locate->lookup(plugctx, plctx->flags, plctx->type, plctx->kd->realm, 0, 0, add_locate, plctx->kd);

	if (plctx->flags & KRB5_PLF_ALLOW_HOMEDIR)
	return locate->old_lookup(plugctx, plctx->type, plctx->kd->realm, 0, 0, add_locate, plctx->kd);

	return KRB5_PLUGIN_NO_HANDLE;
	}

New option [libdefaults] block_dns = true to block all DNS queries. #1155

New option [libdefaults] block_dns = true to block all DNS queries. #1155

Conversation

riastradh commented Jun 9, 2023

riastradh commented Jun 15, 2023

nicowilliams commented Jun 15, 2023

nicowilliams Jun 22, 2023

Choose a reason for hiding this comment

nicowilliams Jun 22, 2023

Choose a reason for hiding this comment

riastradh Jun 22, 2023

Choose a reason for hiding this comment

nicowilliams Jun 22, 2023

Choose a reason for hiding this comment

nicowilliams commented Jun 22, 2023 • edited Loading

riastradh commented Jun 22, 2023 • edited Loading

nicowilliams commented Jun 22, 2023 • edited Loading

riastradh commented Jun 22, 2023 • edited Loading

nicowilliams commented Jun 22, 2023

nicowilliams commented Jan 4, 2024

riastradh commented Jan 4, 2024

nicowilliams commented Jan 4, 2024

riastradh commented Jan 5, 2024

nicowilliams commented Jan 5, 2024

riastradh commented Jan 5, 2024

nicowilliams commented Jan 5, 2024

riastradh commented Jan 7, 2024

nicowilliams commented Jan 8, 2024

riastradh commented Jan 8, 2024

jsutton24 Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

jsutton24 Feb 18, 2024

Choose a reason for hiding this comment

riastradh Feb 18, 2024

Choose a reason for hiding this comment

nicowilliams commented Jun 22, 2023 •

edited

Loading

riastradh commented Jun 22, 2023 •

edited

Loading

nicowilliams commented Jun 22, 2023 •

edited

Loading

riastradh commented Jun 22, 2023 •

edited

Loading

jsutton24 Feb 13, 2024 •

edited

Loading