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.
Instead of freeing host->ai on return from submit_request in the
http_proxy path, stash the proxy's ai in another member host->freeai
which will be freed when host is freed.
Assumption: All hosts created in submit_request cease to be used
before any of them are freed, so it's safe to pick one host to hang
the proxy's ai on and free the ai when that host is freed.
fix https://github.com/heimdal/heimdal/issues/1205
Apple clang version 14.0.0 (clang-1400.0.17.3.1) fails the build
because stds.h defines `fallthrough` as a macro which is then
expanded when base.h evaluates
# if __has_attribute(fallthrough) && __clang_major__ >= 5
The macOS SDK defines `DISPATCH_FALLTHROUGH` as the macro instead
of `fallthrough`.
This change replaces the use of `fallthrough` in the tree with
`HEIM_FALLTHROUGH` and updates the declaration in configure logic
to define `HEIM_FALLTHROUGH` based upon existing definitions
(if any) of `fallthrough` or `DISPATCH_FALLTHROUGH`.
The pseudo keyword 'fallthrough' is defined such that case statement
blocks must end with any of these keywords:
* break;
* fallthrough;
* continue;
* goto <label>;
* return [expression];
*
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes
The macro is defined either as
__attribute__((__fallthrough__))
or as
do {} while (0) /* fallthrough */
not including the semicolon.
This change implements the Linux kernel style and updates several locations
where "/*fallthrough*/ and /* FALLTHROUGH */ were not previously replaced.
Externally imported code such as libedit, libtommath and sqlite are
restored to their unaltered state.
Change-Id: I69db8167b0d5884f55d96d72de3059a0235a1ba3
Let the KRB5_SENDTO_DONE and KRB5_SENDTO_FAILED action states be
handled by the provided switch blocks. This ensures that the
'ret' value is set appropriately before exiting the loop.
Change-Id: I56f4cea83efd86203a9a7a36cf48c80f632cf779
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.
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/
The combination of 8740528b24
("Windows-compatible sentinel socket type and value") and
d497d7e4a7 ("krb5_sendto_kdc:
failover for multiple AAAA/A RRs on one domain") broke
all the send to kdc loop on Windows. rk_socket_t is
a HANDLE and rk_INVALID_SOCKET is the max value. Therefore,
no valid socket will be larger and all communications
will fail.
Change-Id: I3464f78d67b19f14050ad7a01738fb32bac99385
We found that the libraries behaviour when dealing with domains with
more than one entry in them is slightly suboptimal. The situation
was
kdc1 IN A 1.2.3.4
kdc1 IN AAAA ff02::1
I.e. a single hostmame with both IPv6 and IPv4 addresses. When we
run krb5_sendto_kdc on a box with only IPv4 addresses, there is a
3s delay before it fails back to the IPv4 address. This is because
the library sets the 2nd address on each hostname to be 3s in the
future and each additional one another 3s.
We change wait_response() s.t. if one is able to make progress, we
iterate over the list of hosts and move them all 1s forward. We
also modify submit_request() to skip hosts if host_connect() fails.
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).
Prior to this change a KDC response of KRB5KDC_ERR_SVC_UNAVAILABLE
would result in the client looping forever. Setting the action to
KRB5_SENTO_CONTINUE repeats the current loop without altering the
current state. Hence the infinite loop.
As of this change, the action is set to KRB5_SENDTO_RESET which
forces the current kdc's response to be cleared and then to retry.
If KRB5KDC_ERR_SVC_UNAVAILABLE continues to be returned, the retry
limit will be reached and the loop will end.
This bug was filed by multiple sources including Samba and ScottUrban
on github.
Change-Id: If1611be0ada3422cefae89541ed3b3df1f6efe29
If the hostname was already set, a typo in a test meant we were not
freeing it. While we're at it, handle the unlikely possibility that
the existing pointer is passed as the new value.
Patchset 58ff480763 calls write() and read()
on rk_socket_t objects which on Windows are SOCKETs not C RTL file
descriptors. This patchset uses krb5_net_write() and krb5_net_read()
in place of the direct write() and read() operations.
Change-Id: I3f7d4756357d432e4e62910f9a36824eb188b6e4
Currently the Heimdal code calls connect(2) on TCP connexions to
the KDC without setting O_NONBLOCK. This code implements a
timed_connect() function which will in the case of SOCK_STREAM
sockets put the socket into non-blocking mode prior to calling
connect and use select(2) to apply the configured timeout to connect
completion. This does not entirely solve the problem of potential
timeouts in the code as it is still possible to block while writing
to the socket. A proper implementation would also likely start
new connexions after a short interval before timing out existing
connexions and return the results from the first KDC which successfully
responds but we did not do that yet.
This patch is from heimdal-1-5-branch patches:
5b55e4429caed27b32aac4bc5930f2672a43f273
6b66321b271ee4672e70ad349ec796dd755cf897
2e12c7f3e8dca7e1696ebd92199617ce413565e7
Squashed together along with a quick shadowed variable warning fix
to allow it to compile with --enable-developer.
Looks like they defined basename() in string.h and ntohs/htonl are
implemented in terms of __bswap16() which is a macro with tmp
variables and so one cannot embed one call to ntohs/htons in another.
Not good but we workaround this limitation in glibc.
