This addresses part of https://github.com/heimdal/heimdal/issues/1214
to audit potential network leaks with [libdefaults] block_dns = yes.
NI_NUMERICHOST is _probably_ sufficient -- we probably won't see many
systems using NIS to look up service names by number if we fail to
specify NI_NUMERICSERV, and such systems probably require careful
auditing of their own. And I don't know of any way NI_NUMERICSCOPE
could trigger network leaks. But named scope ids are such a niche
option with IPv6 that setting it to forestall concerns can't hurt
much, and it makes reviewing easier if we just unconditionally flip
on all the numeric-only options.
This change has two parts:
1. Provide our own local implementation of numeric-only getaddrinfo
in auditdns.c used to audit for DNS leaks, rather than deferring
to dlsym(RTLD_NEXT, "getaddrinfo"), in terms of inet_pton.
To keep review and implementation simple, this is limited to
AI_NUMERICHOST _and_ AI_NUMERICSERV -- this requires that we
arrange to pass AI_NUMERICSERV in callers too.
2. Wherever we implement block_dns, set AI_NUMERICSERV in addition to
AI_NUMERICHOST as needed by the new auditdns.c getaddrinfo.
(In principle this might also avoid other network leaks -- POSIX
guarantees no name resolution service will be invoked, and gives
NIS+ as an example.)
One tiny semantic change to avoid tripping over the auditor:
kadmin(8) now uses the string "749" rather than the string
"kerberos-adm". (Currently we don't audit kadmin(8) for DNS leaks
but let's avoid leaving a rake to step on.) Every other caller I
found is already guaranteed to pass a numeric service rather than
named service to getaddrinfo.
fix https://github.com/heimdal/heimdal/issues/1212
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.
Coverity thinks `handle` in lib/krb5/send_to_kdc.c:krb5_sendto_context()
at 1241 can be NULL, leading to a NULL derefence in `get_next()`. This
is an attempt to fix this by having `get_next()` check handle for NULL.
If getaddrinfo() succeeds and returns the gTLD name collision
address the result is ignored but the allocated addrinfo was not
freed.
If allocation of the krb5_krbhst_info structure fails the addrinfo
would also be leaked.
Change-Id: I94111e081cba9548f57ad7b7e7cbea3faab7502c
Sync with most changes in AS/TGS client from Apple's Heimdal-597.121.1
(opensource.apple.com).
Changes include:
- FAST support in TGS client
- Refactored pre-auth client to be more easily extensible
- Pin KDC host and AD site name in API calls
Note the completely refactored TGS client loop is not imported as that was
considered too intrusive.
Add a new krbhst type, KRB5_KRBHST_TKTBRIDGEAP to krb5_krbhst_init_flags(),
that looks for the DNS SRV record kerberos-tkt-bridge. This is to support a new
PADL project.
This is the second of two commits in a series that must be picked together.
This series of two commits moves parts of lib/krb5/ infrastructure
functionality to lib/base/, leaving behind wrappers.
Some parts of libkrb5 are entirely generic or easily made so, and could
be useful in various parts of Heimdal that are not specific to the krb5
API, such as:
- lib/gssapi/ (especially since the integration of NegoEx)
- lib/hx509/
- bx509d (which should really move out of kdc/)
For the above we need to move these bits of lib/krb5/:
- lib/krb5/config_file.c (all of it, leaving forwardings behind)
- lib/krb5/config_reg.c (all of it)
- lib/krb5/plugin.c (all of it, leaving forwardings behind)
- lib/krb5/log.c (all of it, ditto)
- lib/krb5/heim_err.et (all of it)
And because of those two, these too must also move:
- lib/krb5/expand_path.c (all of it, leaving forwardings behind)
- lib/krb5/warn.c (just the warning functions, ditto)
The changes to the moved files are mostly quite straightforward and are
best reviewed with --word-diff=color.
We're also creating a heim_context and a heim API to go with it. But
it's as thin as possible, with as little state as necessary to enable
this move. Functions for dealing with error messages use callbacks.
Moving plugin.c does have one knock-on effect on all users of the old
krb5 plugin API (which remains), which is that a global search and
replace of struct krb5_plugin_data to struct heim_plugin_data was
needed, though the layout and size of that structure doesn't change, so
the ABI doesn't either.
As well, we now build lib/vers/ and lib/com_err/ before lib/base/ so as
to be able to move lib/krb5/heim_err.et to lib/base/ so that we can make
use of HEIM_ERR_* in lib/base/, specifically in the files that moved.
Once this is all done we'll be able to use config files and plugins in
lib/hx509/, we'll be able to move bx509d out of kdc/, and so on.
Most if not all of the new functions in lib/base/ are Heimdal-private,
thus calling conventions for them are not declared.
Status:
- builds and passes CIs (Travis, Appveyor)
- ran make check-valgrind and no new leaks or other memory errors
- ready for review
HOW TO REVIEW:
$ # Review file moves:
$ git log --stat -n1 HEAD^
$
$ # Review changes to moved files using --word-diff=color
$ git log -p -b -w --word-diff=color HEAD^..HEAD \
lib/base/config_file.c \
lib/base/config_reg.c \
lib/base/expand_path.c \
lib/base/warn.c \
lib/krb5/config_file.c \
lib/krb5/config_reg.c \
lib/krb5/expand_path.c \
lib/krb5/warn.c
$
$ # Review the whole thing, possibly adding -b and/or -w, and
$ # maybe --word-diff=color:
$ git log -p origin/master..HEAD
$ git log -p -b -w origin/master..HEAD
$ git log -p -b -w --word-diff=color origin/master..HEAD
TBD (future commits):
- make lib/gssapi use the new heimbase functions
- move kx509/bx509d common code to lib/hx509/ or other approp. location
- move bx509d out of kdc/
Refactor plugin framework to use a single list of loaded plugins; add a new
plugin API where DSOs export a load function that can declare dependencies and
export multiple plugins; refactor kadm5 hook API to use krb5 plugin framework.
More information in krb5-plugin(7).
The size of portstr is too small to print an integer.
Instead just let snprintf do the work.
This fixes building with GCC 7.1
Based on feedback by Jeffrey Altman
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12930
(Inspired by Samba commit abd74c3ba5e3ee3f5320bff6ed7dff4fbcb79373)
Signed-off-by: Andrew Bartlett <abartlet@samba.org>
This reverts commit ccb63bb0aa, which was
unnecessary and broke tests/kdc/check-kadmin (and other things).
host->port happens to be an unsigned short, so that promotion to an integer in
the snprintf() call is safe in that the promoted value will still be
non-negative, and no larger than an unsigned short's maximum value. We're
still assuming that 7 bytes is sufficient to hold the text representation of
that maximum value, which indeed it is, assuming sizeof(unsigned short) == 2
and CHAR_BIT == 8, which are fair assumptions here. A better patch, if we
needed it, would be to just make portstr[] an array of 11 char, or perhaps make
it a VLA (but we can't yet use VLAs, I don't think, because of older Windows
systems that must be supported still).
The original motivation was to avoid extra timeouts when the network is
broken. However this doesn't avoid one of the timeouts and adds
complexity and introduced bugs.
To really suppress search lists use ndots.
Apending '.' to the hostname passed to `getaddrinfo()` is good for
avoiding extra timeouts when the search list is non-empty and the
network is broken, but searches in /etc/hosts are typically inhibited
then. The fix is to try again without the trailing '.' if the first
lookup failed for any reason other than a timeout.
In srv_find_realm() the conditional for testing whether an entry
is the invalid gTLD response was inverted. Refactor the conditional
into a helper function is_invalid_tld_srv_target(). Use the helper
to simplify the conditional making it easier to confirm that the
test is correct.
Change-Id: I3220753b5585ac535862c4617030377c7a1f4bbe
As per
https://www.icann.org/en/system/files/files/name-collision-mitigation-01aug14-en.pdf
prior to a new top-level domain being put into service there is controlled
interuption service which will return explicit responses to DNS A, MX, SRV, and TXT
queries that can be used to detect private namespace collisions.
When performing fallback_get_hosts() check the AF_INET responses to ensure
that they are not the gTLD name collision address 127.0.53.53. If so, add
an error message to the context and return KRB5_KDC_UNREACH.
Write a warning to the log (if any).
Change-Id: I2578f13948b8327cc3f06542c1e489f02410143a
As per
https://www.icann.org/en/system/files/files/name-collision-mitigation-01aug14-en.pdf
prior to a new top-level domain being put into service there is a
controlled interuption service which will return explicit responses to DNS
A, MX, SRV, and TXT queries that can be used to detect private namespace collisions.
Modify SRV records lookups to detect the special hostname returned in the
SRV response, skip the response, and record an appropriate error if it is detected.
Write a warning to the log (if any).
Change-Id: I47e049b617e39e49939bc92d513a547de1d04624
