Greg Hudson [Sat, 18 Jan 2014 18:03:32 +0000 (13:03 -0500)]
Fix gss_pseudo_random leak on zero length output
Nobody is likely to ever ask for zero bytes of output from
gss_pseudo_random, but if they do, just return an empty buffer without
allocating. Otherwise we leak memory because gss_release_buffer
doesn't do anything to buffers with length 0.
Greg Hudson [Thu, 16 Jan 2014 16:32:10 +0000 (11:32 -0500)]
Avoid assertion failure in error_message
r17942 added a call to get_thread_buffer in the first part of
error_message, prior to the call to com_err_initialize. This can
cause an assertion failure from k5_getspecific if error_message is
called on a system error before any other com_err functions are
called. Move the initialization call earlier to prevent this.
Greg Hudson [Wed, 15 Jan 2014 17:51:42 +0000 (12:51 -0500)]
Clean up GSS krb5 acquire_accept_cred
Use a cleanup handler instead of releasing kt in multiple error
clauses. Wrap a long line and fix a comment with a missing word.
Rewrap the function arguments to use fewer lines.
Tom Yu [Tue, 14 Jan 2014 20:43:35 +0000 (15:43 -0500)]
Remove mentions of krb5-send-pr
Start the process of deprecating krb5-send-pr. In practice, it causes
frustration for some users, and free-form email is good enough for
most bug reports.
Continue to install krb5-send-pr for now, but plan to remove it from
the tree in the future, probably replaced by a script that instructs
the user to send email manually.
Greg Hudson [Mon, 13 Jan 2014 17:02:09 +0000 (12:02 -0500)]
Don't produce context deletion token in krb5 mech
RFCs 2743 and 4121 recommend that implementations produce empty tokens
from gss_delete_sec_context, and trying to produce one can cause
gss_delete_sec_context to fail on a partially established context.
Patch from Tomas Kuthan.
Greg Hudson [Fri, 10 Jan 2014 16:54:13 +0000 (11:54 -0500)]
Restrict AES-NI support to ELF platforms for now
Since we explicitly specify the ELF object format when building
iaesx86.s or iaesx64.s, we need to restrict it to operating systems we
know to be ELF platforms. Otherwise we can break the build on OS X,
which uses the Mach-O object format.
Greg Hudson [Thu, 9 Jan 2014 05:18:44 +0000 (00:18 -0500)]
Work around Linux session keyring write behavior
If the session keyring matches the user session keyring, write
explicitly to the user session keyring. Otherwise the kernel might
create a new session keyring for the process, making the resulting
cache collection invisible to other processes.
Tom Yu [Tue, 7 Jan 2014 21:37:46 +0000 (16:37 -0500)]
Refactor krb5_string_to_keysalts()
Use various standard C library functions rather than rolling our own.
Previous code spent many lines reimplementing realloc(), strpbrk(),
strtok_r(), etc.
Make a separate string_to_keysalt() parser for an individual keysalt
pair, which for now is private and destructive.
Tom Yu [Mon, 6 Jan 2014 22:17:02 +0000 (17:17 -0500)]
Make salt defaulting work for keysalts
Make krb5_string_to_keysalts() default to only ":" as a key:salt
separator character. Change most of its callers to pass NULL so they
get the default separators.
Tom Yu [Mon, 6 Jan 2014 21:32:50 +0000 (16:32 -0500)]
Default to normal salt instead of "ignore"
krb5_string_to_keysalts() treats an empty salt field as -1 ("ignore"),
rather than as the normal salttype. Default to normal instead, so
that omitting a salttype works as expected.
Greg Hudson [Fri, 3 Jan 2014 18:50:48 +0000 (13:50 -0500)]
Mark AESNI files as not needing executable stacks
Some Linux systems now come with facilities to mark the stack as
non-executable, making it more difficult to exploit buffer overrun
bugs. For this to work, object files built from assembly need a
section added to note whether they require an executable stack.
Patch from Dhiru Kholia with comments added. More information at:
https://bugzilla.redhat.com/show_bug.cgi?id=1045699
https://wiki.gentoo.org/wiki/Hardened/GNU_stack_quickstart
Tom Yu [Wed, 1 Jan 2014 00:43:28 +0000 (19:43 -0500)]
Test bogus KDC-REQs
Send encodings that are invalid KDC-REQs, but pass krb5_is_as_req()
and krb5_is_tgs_req(), to make sure that the KDC recovers correctly
from failures in decode_krb5_as_req() and decode_krb5_tgs_req(). Also
send an encoding that isn't a valid KDC-REQ.
rbasch [Mon, 16 Dec 2013 15:54:41 +0000 (10:54 -0500)]
Log service princ in KDC more reliably
Under some error conditions, the KDC would log "<unknown server>" for
the service principal because service principal information is not yet
available to the logging functions. Set the appropriate variables
earlier.
do_as_req.c: After unparsing the client, immediately unparse the
server before searching for the client principal in the KDB.
do_tgs_req.c: Save a pointer to the client-requested service
principal, to make sure it gets logged if an error happens before
search_sprinc() successfully completes.
[tlyu@mit.edu: commit message; fix TGS to catch more error cases]
Greg Hudson [Thu, 19 Dec 2013 18:33:33 +0000 (13:33 -0500)]
Allow realm in kadm5_init service names
Previously, if you passed a service name with a realm part to a
kadm5_init function, you would get a KRB5_PARSE_MALFORMED error
because the code would internally append its own '@realm' suffix
before parsing the name. Fix this as follows:
Change gic_iter so instead of producing a full service name, it
produces a krb5_principal which is taken from the cred it acquires.
Pass the client and full service name around as principals, rather
than strings, and use the gss_nt_krb5_principal name type to import
them in setup_gss(). Don't append a realm to the input service name;
instead, pass the input service name directly to the gic functions
(which do not need a realm in the service name and will ignore the
realm if one is present). For the INIT_CREDS case, parse the input
service name with KRB5_PRINCIPAL_PARSE_IGNORE_REALM and then set the
realm.
Greg Hudson [Thu, 19 Dec 2013 17:22:47 +0000 (12:22 -0500)]
Simplify libkadm5 client realm initialization
The "realm" variable in init_any is used only to fill in the realm of
the service principal in gic_iter(). The service principal realm
should always be the realm we looked up config parameters for, so we
can supply that realm to get_init_creds() unconditionally and
eliminate the case where we use the client principal realm.
Also get rid of an outdated comment and an #if 0 block we will never
need again, and use SNPRINTF_OVERFLOW to check the snprintf result.
Greg Hudson [Fri, 20 Dec 2013 16:06:52 +0000 (11:06 -0500)]
Use an extended com_err hook in klist
Add an adapted version of extended_com_err_fn from kinit to klist and
use it. In do_ccache(), rely on the ccache type to set a reasonable
message if krb5_cc_set_flags() or krb5_cc_get_principal() fails due to
a nonexistent or unreadable ccache, and don't confuse the user with
the name of the ccache operation that failed.
Nalin Dahyabhai [Thu, 5 Dec 2013 18:54:09 +0000 (13:54 -0500)]
Set an error message when keyring get_princ fails
When attempting to use a keyring cache that doesn't exist, set an error
message when we fail to read a principal name, as we do when we return
the same error code when using a file ccache.
[ghudson: removed unnecessary check for d->name nullity.]
Greg Hudson [Fri, 20 Dec 2013 04:47:22 +0000 (23:47 -0500)]
Test for verto_set_flags in system libverto
libkrad relies on verto_set_flags, which was added to libverto in
release 0.2.4. Make sure the system libverto has this function before
choosing it over the built-in version.
Zhanna Tsitkov [Thu, 19 Dec 2013 18:08:56 +0000 (13:08 -0500)]
Move kprop error explanation into Troubleshooting
The plan is to make Troubleshooting section of the documentation a
one-stop-shop place for all error diagnostics, explanations and possible
solutions. The relocation of kprop error messages descriptions is part of
this consolidation effort.
Greg Hudson [Wed, 18 Dec 2013 18:08:25 +0000 (13:08 -0500)]
Add a test program for krb5_copy_context
This test program isn't completely proof against the kind of mistakes
we've made with krb5_copy_context in the past, but it at least
exercises krb5_copy_context and can detect some kinds of bugs.
Greg Hudson [Wed, 18 Dec 2013 20:03:03 +0000 (15:03 -0500)]
Fix krb5_copy_context
krb5_copy_context has been broken since 1.8 (it broke in r22456)
because k5_copy_etypes crashes on null enctype lists. Subsequent
additions to the context structure were not reflected in
krb5_copy_context, creating double-free bugs. Make k5_copy_etypes
handle null input and account for all new fields in krb5_copy_context.
Reported by Arran Cudbard-Bell.
Simo Sorce [Tue, 17 Dec 2013 21:15:14 +0000 (16:15 -0500)]
Let SPNEGO display mechanism errors
To avoid potential recursion we use a thread local variable that tells
us whether the ancestor was called via spnego_gss_display_name(). If
we detect recursion, we assume that we returned a com_err code like
ENOMEM and call error_message(); in the worst case that will result in
an "Unknown error" message.
[ghudson@mit.edu: Edited comments and commit message; removed an
unneeded line of code.]
Greg Hudson [Tue, 17 Dec 2013 21:56:41 +0000 (16:56 -0500)]
Clarify klist -s documentation
The documentation for klist -s erroneously suggests that it doesn't
affect the exit status behavior and that it merely checks for the
existence of the ccache (only mentioning the expired ticket check at
the end). Make it clearer and simpler, but avoid going into a lot of
detail about the nature of the expiration check.
Greg Hudson [Mon, 16 Dec 2013 22:09:00 +0000 (17:09 -0500)]
Don't require krb5.conf without KRB5_DNS_LOOKUP
For a long time we have allowed krb5 contexts to be initialized in the
absence of krb5.conf--but only if KRB5_DNS_LOOKUP is defined,
presumably on the theory that no KDCs could be contacted without
either DNS support or profile configuration. But locate plugins could
provide the ability to find KDCs, and some libkrb5 operations (such as
IAKERB initiation) could succeed without needing to locate KDCs.
Also get rid of the profile_in_memory context flag, since we don't use
it any more.
Greg Hudson [Mon, 16 Dec 2013 20:37:56 +0000 (15:37 -0500)]
Fix GSS krb5 acceptor acquire_cred error handling
When acquiring acceptor creds with a specified name, if we fail to
open a replay cache, we leak the keytab handle. If there is no
specified name and we discover that there is no content in the keytab,
we leak the keytab handle and return the wrong major code. Memory
leak reported by Andrea Campi.
Simo Sorce [Fri, 13 Dec 2013 17:00:41 +0000 (12:00 -0500)]
Fix memory leak in SPNEGO initiator
If we eliminate a mechanism from the initiator list because
gss_init_sec_context fails, free the memory for that mech OID before
removing it from the list.
Greg Hudson [Mon, 16 Dec 2013 16:35:42 +0000 (11:35 -0500)]
Remove unneeded check in SPNEGO initiator
In init_ctx_cont, if the response token contains no fields, we set a
return value but don't actually quit out of the function. We do not
need this check (we will fail later on if a piece of required
information isn't present), so just remove it. Reported by
simo@redhat.com.
Greg Hudson [Tue, 10 Dec 2013 17:04:18 +0000 (12:04 -0500)]
Fix SPNEGO one-hop interop against old IIS
IIS 6.0 and similar return a zero length reponse buffer in the last
SPNEGO packet when context initiation is performed without mutual
authentication. In this case the underlying Kerberos mechanism has
already completed successfully on the first invocation, and SPNEGO
does not expect a mech response token in the answer. If we get an
empty mech response token when the mech is complete during
negotiation, ignore it.
[ghudson@mit.edu: small code style and commit message changes]
Greg Hudson [Sun, 8 Dec 2013 23:05:26 +0000 (18:05 -0500)]
Allow ":port" suffixes in sn2princ hostnames
MSSQLSvc principal names can contain a ":port" or ":instance" trailer
on the hostname part. If we see that in the hostname argument of
krb5_sname_to_principal(), remove it before canonicalizing the
hostname and put it back on afterwards.
Greg Hudson [Fri, 6 Dec 2013 01:32:05 +0000 (20:32 -0500)]
Fix S4U2Self against non-FAST KDCs
When we added FAST TGS support in 1.11, we broke S4U2Self against KDCs
which don't support FAST, because the S4U2Self padata is only present
within the FAST request. For now, duplicate that padata in the outer
request so that both FAST and non-FAST KDCs can see it.
Greg Hudson [Tue, 3 Dec 2013 07:05:46 +0000 (02:05 -0500)]
Edit README.asn1
Add another blank line before section headers. Avoid contractions.
Change some whiches to thats where it seems appropriate. Fix some
missing or extra words.
Greg Hudson [Mon, 25 Nov 2013 16:46:47 +0000 (11:46 -0500)]
Correctly log IPv6 addresses in kadmind
Define client_addr() in server_stubs.c and use it consistently in that
file and ipropd_svc.c to get the client address from a transport
handle. In it, call getpeername() on the client socket and use
inet_ntop() on the result, instead of using inet_ntoa() on the IPv4
socket address. Provide a log_badauth2 callback to GSSRPC, so that we
get a transport handle instead of an IPv4 socket address, and use
client_addr() within it instead of inet_ntoa().
Greg Hudson [Mon, 25 Nov 2013 16:33:35 +0000 (11:33 -0500)]
Add new versions of log_badauth gssrpc callbacks
libgssrpc supports two callbacks for gss_accept_sec_context failures
on servers (one for AUTH_GSS and one for AUTH_GSSAPI), which are
IPv4-specific. Provide an alternate version which supplies the
transport handle instead of the address, so that we can get the
address via the file descriptor for TCP connections.
Greg Hudson [Thu, 21 Nov 2013 22:30:54 +0000 (17:30 -0500)]
Improve default ccache name API documentation
Document the lifetime and caching behavior of the
krb5_cc_default_name() return value. Document that
krb5_cc_set_default_name() may be called with NULL to purge the cached
value. Correct a typo in the krb5_cc_default() summary and explicitly
reference krb5_cc_default_name().
Greg Hudson [Thu, 21 Nov 2013 21:18:27 +0000 (16:18 -0500)]
Add another kadmin ACL test for backreferences
Add a test using backreferences which don't correspond directly to
principal components, to verify that *N refers to the Nth wildcard and
not the Nth component.
Greg Hudson [Mon, 18 Nov 2013 23:59:17 +0000 (18:59 -0500)]
Clarify lockout replication issues in docs
In the "KDC replication and account lockout" section of lockout.rst,
specifically call out kprop and incremental propagation as the
mechanisms which do not replicate account lockout state, and add a
note that KDCs using LDAP may not be affected by that section's
concerns.
Greg Hudson [Sun, 17 Nov 2013 17:37:09 +0000 (12:37 -0500)]
Remove dangling --with-kdc-kdb-update references
This configure option hasn't done anything since 1.8, so don't mention
it in configure --help or the documentation. The disable_last_success
and disable_lockout DB options are now used to turn it off.
Greg Hudson [Sat, 16 Nov 2013 04:38:15 +0000 (23:38 -0500)]
Remove a warning in AES string-to-key
On 32-bit platforms, the code to translate an iteration count of 0 to
2^32 can trigger a compiler warning. Since we will basically never
accept an iteration count that high (right now we reject anything
above 2^24), just reject it out of hand.
Simo Sorce [Thu, 14 Nov 2013 22:23:59 +0000 (17:23 -0500)]
Add support to store time offsets in cc_keyring
The code follows the same model used for the memory ccache type. Time
offsets are stored in each credential cache in a special key just like
the principal name. Legacy session caches do not store timestamps as
legacy code would fail when iterating over the new offset key.
[ghudson@mit.edu: minor formatting changes; note legacy session
exception in commit message]
Nalin Dahyabhai [Mon, 11 Nov 2013 18:10:08 +0000 (13:10 -0500)]
Catch more strtol() failures when using KEYRINGs
When parsing what should be a UID while resolving a KEYRING ccache
name, don't just depend on strtol() to set errno when the residual
that we pass to it can't be parsed as a number. In addition to
checking errno, pass in and check the value of an "endptr".
Greg Hudson [Wed, 6 Nov 2013 18:33:04 +0000 (13:33 -0500)]
Clarify realm and dbmodules configuration docs
In kdc_conf.rst, add examples showing how to configure a realm
parameter and a database parameter. Document that the default DB
configuration section is the realm name, and use that in the example.
Move the db_module_dir description to the end of the [dbmodules]
documentation since it is rarely used and could confuse a reader about
the usual structure of the section.
A related but more minor vulnerability requires authentication to
exploit, and is only present if a third-party KDC database module can
dereference a null pointer under certain conditions.
Ben Kaduk [Wed, 30 Oct 2013 18:51:12 +0000 (14:51 -0400)]
Clean up the code to eliminate some clang warnings
In ure.c, though k is a short, the literal 1 is of type 'int', and
so the operation 'k + 1' is performed at the (32-bit) width of int,
and therefore the "%d" format string is correct.
In accept_sec_context.c, the 'length' field of krb5_data is an
unsigned type, so checking for a negative value has no effect.
In net-server.c, the helper routine rtm_type_name() is only used
in code that is disabled with #if 0 conditionals; make the
definition also disabled in the same way to avoid warnings of an
unused function.
In kdc_authdata.c, equality checks in double parentheses elicit
a warning from clang. The double-parentheses idiom is normally used
to indicate that an assignment is being performed, but the value of
the assignment is also to be used as the value for the conditional.
Since assignment and equality checking differ only by a single
character, clang considers this worthy of a warning. Since the extra
set of parentheses is redundant and against style, it is correct to
remove them.
In several places (sim_server.c, dump.c, kdb5_destroy.c,
ovsec_kadmd.c), there are declarations of extern variables relating
to getopt() functionality that are now unused in the code. Remove
these unused variables.
Ben Kaduk [Wed, 30 Oct 2013 18:11:40 +0000 (14:11 -0400)]
Make set_cloexec_fd return void
We never check its return value (causing clang to emit warnings),
and its use is primarily in cases where we should continue processing
in the event of failure. Just ignore errors from the underlying
fcntl() call (if present) and treat this operation as best-effort.
Ben Kaduk [Tue, 10 Jul 2012 14:14:52 +0000 (10:14 -0400)]
Avoid deprecated krb5_get_in_tkt_with_keytab
The kprop code has been pretty unloved, and uses some routines that
are marked as deprecated (which show up as warnings in the build log).
Use the documented replacement for krb5_get_in_tkt_with_keytab,
krb5_get_init_creds_keytab, instead. As a bonus, there is no longer
a side effect of a credentials cache that needs to be destroyed.
The also-deprecated function krb5_get_in_tkt_with_skey was backending
to it when no keyblock was passed in; we can unroll the call to
krb5_get_init_creds_keytab ourselves as the documented workaround.
While here, improve style compliance with regards to cleanup.
The setkey test just wants to know whether it can use the key it
just put into a keytab to get credentials; as such the recommended
krb5_get_init_creds_keytab is quite sufficient.
While here, use that interface to request the particular enctype
as well, reducing the scope of an XXX comment.
Ben Kaduk [Tue, 3 Jul 2012 14:27:20 +0000 (10:27 -0400)]
Remove last uses of "possibly-insecure" mktemp(3)
Many libc implementations include notations to the linker to generate
warnings upon references to mktemp(3), due to its potential for
insecure operation. This has been the case for quite some time,
as was noted in RT #6199. Our usage of the function has decreased
with time, but has not yet disappeared entirely. This commit
removes the last few instances from our tree.
kprop's credentials never need to hit the disk, so a MEMORY ccache
is sufficient (and does not need randomization).
store_master_key_list is explicitly putting keys on disk so as to
do an atomic rename of the stash file, but since the stash file
should be in a root-only directory, we can just use a fixed name
for the temporary file. When using this fixed name, we must detect
(and error out) if the temporary file already exists; add a test to
confirm that we do so.
Ben Kaduk [Wed, 18 Jul 2012 14:05:33 +0000 (10:05 -0400)]
Clean up stash file error handling
The comment previously failed to match the behavior. The intent was
that if we failed to write out the entire stash file into the
temporary location, we should remove the partial file. However, the
code was actually checking whether the *real* stash file existed,
not whether the temporary one existed.
It is safe to always try to unlink the partial file, and not worry
about whether it already exists.
Ben Kaduk [Mon, 4 Nov 2013 18:09:13 +0000 (13:09 -0500)]
Use retval, not errno, when stashing master keys
The krb5_db_store_master_key{,_list} functions return a
krb5_error_code, and do not necessarily set errno on failure.
Use the correct variable while reporting errors with com_err().
Greg Hudson [Wed, 30 Oct 2013 22:22:00 +0000 (18:22 -0400)]
Clarify kpropd standalone mode documentation
The kpropd -S option is no longer needed to run kpropd in standalone
mode, but its functionality is not deprecated; standalone mode is
automatically activated when appropriate. Clarify the kpropd
documentation on standalone mode to avoid giving the impression that
the mode is deprecated.
Greg Hudson [Mon, 28 Oct 2013 15:23:11 +0000 (11:23 -0400)]
Improve LDAP KDB initialization error messages
In krb5_ldap_initialize, don't just blat the LDAP error into the
extended message; give an indication of which LDAP operation we were
trying to do and show what parameters we gave to it.
(Also, krb5_set_error_message can handle a null context argument, so
don't bother to check before calling.)
Greg Hudson [Mon, 28 Oct 2013 17:09:15 +0000 (13:09 -0400)]
Accept anonymous GSS names in kadmind
The krb5 implementation of gss_display_name() reports the name type as
GSS_C_NT_ANONYMOUS if the client uses an anonymous principal. Accept
this name type in gss_name_to_string and gss_to_krb5_name so that
anonymous kadmin can work.
Also improve code hygiene: call gss_name_to_string from
gss_to_krb5_name to reduce code repetition; use gss_oid_equal instead
of pointer comparison for name types; and don't assume that the
gss_display_name result buffer is zero-terminated.
Greg Hudson [Sun, 27 Oct 2013 00:17:10 +0000 (20:17 -0400)]
Fix decoding of mkey kvno in mkey_aux tl-data
krb5_dbe_lookup_mkey_aux was decoding a 16-bit value directly into an
int, resulting in the wrong value on big-endian platforms. The
consequences are mostly invisible because we ignore this field and try
all mkey_aux nodes in krb5_def_fetch_mkey_list.
Ben Kaduk [Fri, 25 Oct 2013 17:33:23 +0000 (13:33 -0400)]
Add tests for different salt combinations
Create a principal with a pair of enctypes using different salt types.
Confirm that the non-default salt type appears only once in the principal's
key list.
Also verify that the afs3 salt type is rejected by non-DES enctypes
The afs3 salt type is for compatibility with AFS-3 kaservers, which
are roughly krb4. As such, it only makes sense for single-DES
enctypes. The PBKDF2 and arcfour enctypes correctly reject the
key-creation parameters from the afs3 salt, but triple-DES currently
does not.
Ben Kaduk [Fri, 25 Oct 2013 18:00:29 +0000 (14:00 -0400)]
Reset key-generation parameters for each enctype
In add_key_pwd, initialize s2k_params to NULL inside the loop over
enctypes instead of outside the loop, so that if the afs3 salt type
is used it does not contaminate later enctype/salt pairs in the list.