Ben Kaduk [Mon, 11 May 2015 17:08:42 +0000 (13:08 -0400)]
Reboot after KfW installs to help the LSA cache
It seems that we need to restart in order to be able to query the
contents of the the LSA cache, even if the only contents of the LSA
cache are what we put there, and even if the Microsoft klist.exe
correctly reports the presence of tickets in the LSA cache.
Ben Kaduk [Tue, 5 May 2015 20:55:41 +0000 (16:55 -0400)]
Supply a hostrealm module to query the registry
Implement a default_realm function that checks the
{HKLM,HKCU}\Software\MIT\Kerberos5\default_realm registry values
on Windows, and just returns KRB5_PLUGIN_NO_HANDLE on Unix.
Ben Kaduk [Fri, 6 Mar 2015 20:42:10 +0000 (15:42 -0500)]
Fix loop to determine MSLSA principal name
When looping over principals, check the i-th entry instead of
looking at the 0-th entry each time through the loop. This would
only affect cases when multiple ticket entries were returned from
the LSA, the first one did not have a valid principal name, but
some other one did. It is expected that all of the returned
ticket entries will always have a valid client principal name, so
this is unlikely to cause any functional difference.
Ben Kaduk [Wed, 5 Dec 2012 17:15:07 +0000 (12:15 -0500)]
KfW shortcuts for make default, change password
Shortcut keys such as these (in the ACCELERATORS entry in the resource
file) are what let users type, e.g., ctrl-t to get to the "get tickets"
dialog directly from the main frame. We had shortcut keys for all the
other buttons already, so add these to complete the set.
The make default and change password functionality were already available
using keyboard-only interfaces via the ribbon access keys (tap alt,
then letters to walk through the tree of controls), but the two forms
of keyboard access are implemented differently.
Ben Kaduk [Fri, 10 Apr 2015 21:33:40 +0000 (17:33 -0400)]
Switch to Windows SDK Ribbon from MFC Ribbon
The MFC Ribbon implementation is not very accessible (e.g., to
screen reading software), whereas the windows ribbon provides
essentially the same functionality and good integration with
screen reading software, including the built-in Windows Narrator.
Remove the RT_RIBBON_XML resource from the resource file and
replace it with an inclusion of the generated kfwribbon.rc file.
Also remove the ribbon1.mfcribbon-ms ribbon description from the
res/ directory. Add the appropriate dependency relation in the
Makefile.
LeashUIApplication implements the IUIUApplication interfaces. It
appears to be difficult to cleanly tear down the underlying
IUIFramework and ribbon, since the WM_DESTROY event is handled by the
parent MFC window, which will not call IUIFramework::Destroy().
Manually inserting a call to IUIFramework::Destroy() in the shutdown
handling of the MFC classes is difficult, since the WM_DESTROY message
is handled by a different window than where the ribbon is initialized,
and the MFC framework will attempt to access window objects
corresponding to the UI Ribbon resources after they are destroyed,
which raises exceptions. It seems best to just go without destroying
the IUIFramework, since its lifecycle matches that of the application
and there will be no leaks during the application lifecycle.
LeashUICommandHandler implements the IUICommandHandler interfaces,
passing messages through to the existing MFC handlers, though the
default values for the various checkbox controls must be duplicated.
The (MFC) CMainFrame creates and maintains a handle to the
LeashUIApplication associated with the ribbon it creates, so that
it can query the height of the ribbon and redraw when the
LeashUIApplication signals that the ribbon size has changed.
Record that the added object files depend on kfwribbon.h, so that
the XML markup is compiled sufficiently early in the build.
Ben Kaduk [Thu, 19 Mar 2015 18:14:02 +0000 (14:14 -0400)]
Mention Visual Studio 2010 SP1 in windows README
The service pack is needed to avoid a linker error due to an
issue with the cvtres.exe utility, which manifests as
LINK: fatal error LNK1123: failure during conversion to COFF: file
invalid or corrupt.
Benjamin Kaduk [Fri, 17 Jan 2014 21:02:13 +0000 (16:02 -0500)]
XML Ribbon markup file
The standard windows library ribbon interface is either constructed
at runtime or specified in an XML file. Since we have a static
set of functionality in our ribbon, it is simplest to just use the
XML file.
This should duplicate the interfaces currently provided by the
MFC ribbon, though the menu items in the file menu are slightly
taller than they used to be.
Use uicc.exe to compile the XML to the binary format and produce
a kfwribon.rc resource file and kfwribbon.h header.
Benjamin Kaduk [Mon, 27 Jan 2014 18:07:38 +0000 (13:07 -0500)]
Import separate large ribbon bitmaps
This is the content from homelarge.bmp split up into the
separate component images, since the windows ribbon has the
(more sane) interface of using a separate resource for each
graphic, instead of expecting them all in a single bitmap which
is sliced up at runtime.
The bitmaps are required to have alpha channels, and it seems that
the easiest way to generate bitmaps with alpha channels is to use
Microsoft Paint, since the normal Unix open-source graphics tools
do not want to output this format.
Benjamin Kaduk [Wed, 22 Jan 2014 05:05:57 +0000 (00:05 -0500)]
Remove MBCS from leash's DEFINES
This is just enabling the use of multi-byte character set in the
MFC library, but we do not appear to make use of this feature.
Visual Studio 2013 gives ominous warnings that support for it may
be removed in future versions, so quiet the build and do not
enable the deprecated feature we are not using.
Ben Kaduk [Tue, 14 Apr 2015 19:33:20 +0000 (15:33 -0400)]
Fix leash crash found in some build environments
When freeing a credentials cache name obtained from
krb5_cc_get_full_name(), the code was using plain free()
instead of the matching krb5_free_string(). If these routines
are picked from different modules at runtime, the mismatch
will cause a crash in free(), so change to using the matched
deallocation function.
In order to use it in leash, it must be declared in Lglobals.h and
the function pointer symbol defined in Leash.cpp.
This file is out of date, and we now use the wiki for the kind of
material it covers. Most of the information here is covered
http://k5wiki.kerberos.org/wiki/Committer_resources
Greg Hudson [Tue, 24 Mar 2015 16:02:37 +0000 (12:02 -0400)]
Prevent requires_preauth bypass [CVE-2015-2694]
In the OTP kdcpreauth module, don't set the TKT_FLG_PRE_AUTH bit until
the request is successfully verified. In the PKINIT kdcpreauth
module, don't respond with code 0 on empty input or an unconfigured
realm. Together these bugs could cause the KDC preauth framework to
erroneously treat a request as pre-authenticated.
CVE-2015-2694:
In MIT krb5 1.12 and later, when the KDC is configured with PKINIT
support, an unauthenticated remote attacker can bypass the
requires_preauth flag on a client principal and obtain a ciphertext
encrypted in the principal's long-term key. This ciphertext could be
used to conduct an off-line dictionary attack against the user's
password.
Greg Hudson [Wed, 4 Mar 2015 19:43:20 +0000 (14:43 -0500)]
Add tests for key rotation and 32-bit keytab kvnos
In t_keytab.py, test that kvnos no longer wrap after 255 or 32767, that
they do wrap from 65535 to 1, and that kadmin ktrem preserves the more
recent key after a wraparound.
Also test edge cases of the 32-bit keytab kvno extension using
hand-crafted keytab entries.
Greg Hudson [Sun, 8 Mar 2015 20:52:11 +0000 (16:52 -0400)]
Adjust keytab kvno workarounds
In krb5_ktfile_get_entry(), change the pivot and fuzzy match
workarounds for kvnos to work with the 32-bit kvno extension. For the
pivot logic, try to recognize kvno wraparound at boundary by looking
at the relative timestamps and the size of the version difference.
For the fuzzy match logic, remember the first match against the low 8
bits of the desired kvno, but keep searching for an exact match.
Greg Hudson [Wed, 4 Mar 2015 19:43:00 +0000 (14:43 -0500)]
Implement 32-bit keytab kvno extension
Heimdal and Shishi support a 32-bit kvno at the end of a keytab entry,
overriding the 8-bit version if present. Implement this in the FILE
keytab type and document it in keytab_file_format.rst.
Greg Hudson [Wed, 4 Mar 2015 22:19:56 +0000 (17:19 -0500)]
Expand kadmin protocol kvno range
Make xdr_krb5_kvno() use xdr_u_int() instead of xdr_u_char(), allowing
it to marshal kvno values up to 32 bits. This change is
backwards-compatible because XDR uses four bytes to marshal char
values and does no bounds checking of char values on decode.
Greg Hudson [Sun, 8 Mar 2015 20:20:07 +0000 (16:20 -0400)]
Use unsigned 16-bit type for key data kvno
Change key_data_kvno from a signed 16-bit field to an unsigned 16-bit
field, since negative values are never meaningful. When adding new
keys, wrap from 65535 to 1 to avoid using the special value 0.
Don't bump the KDB binary version since this change is unlikely to
affect callers.
krb5_ldap_get_value() takes a pointer to int, and should not be passed
a pointer to any integral type which might have a different width.
Use an intermediate variable for each call.
The erroneous calls in ldap_misc.c were passing pointers to int32_t,
which is harmless on all common platforms. The calls in
ldap_tkt_policy.c were passing pointers to long; on big-endian LP64
platforms, the result would be written to the high 32 bits of the long
value.
Tom Yu [Mon, 13 Apr 2015 20:45:35 +0000 (16:45 -0400)]
Remove STRING_BUFFER() macro in gssapi_generic.c
In gssapi_generic.c, struct mech_attr_info_desc included some
gss_buffer_desc members whose length fields were never used.
Additionally, the STRING_BUFFER() macro's computation of the (unused)
length fields was incorrect, causing warnings in some versions of
clang. Remove the problematic STRING_BUFFER() macro and adjust the
array and generic_gss_display_mech_attr() accordingly.
When resetting the ulog header or initializing it from a dump file
kdb_last_t value, instead of setting kdb_num to 0, create a dummy
entry for the last_sno value so that we can remember its timestamp.
With this change, a slave no longer needs to perform two full resyncs
after an upstream header initialization. Dummy entries are never
transmitted to downstream slaves because the iprop protocol never
transmits the kdb_first_sno update; if one is somehow transmitted, the
slave will ignore it because it doesn't have the kdb_commit flag set.
reset_header() is renamed to reset_ulog(), takes a kdb_log_context
parameter, and is responsible for syncing the header. sync_update()
now returns void and aborts if msync() fails, just like sync_header().
A new helper set_dummy() writes a dummy entry and sets the ulog to
point to it.
Adjust kproplog to recognize and display dummy entries. Adjust
t_ulog.c and t_iprop.py for the new behavior. In t_iprop.py, remove a
kpropd -t test which became redundant with the previous test.
Add a run_kpropd_once() method to K5Realm(), and add tests to
t_iprop.py for the cases where no updates are needed, where
incremental updates are needed, and where a full resync is needed
followed by a poll for updates.
If kpropd is asked to run just once, don't exit after starting a full
resync; we want to wait for the fullprop child to process the request,
and then request incremental updates afterwards. Also don't exit from
do_standalone() in the fullprop child, in case multiple full resyncs
are required to get the database up to date.
When kpropd operates in iprop mode, full resyncs are handled by a
child process. After a full resync, we want to poll for incremental
updates, as the dump we received may have come from a pre-existing
dump file which was not current. To make this polling happen
promptly, signal the parent process from the child process after a
dump is received.
With this change, t_iprop.py no longer has to prod kpropd after a full
resync occurs, so remove that logic.
Augment the LDAP KDB module tests to include client principal aliases
as well as server principal aliases. Also revise the server principal
alias tests to include an AS-REQ case. (This requires adjusting the
subsequent test not to assume a ccache containing a TGT.)
For the convenience of developers manually running Python test
scripts, set the executable bits on all of them, and make sure the
first line is always "#!/usr/bin/python".
Greg Hudson [Thu, 26 Mar 2015 16:47:06 +0000 (12:47 -0400)]
Disable principal renames for LDAP
The current principal rename procedure does not work with the LDAP KDB
module, instead having the effect of deleting the principal. The fix
is not easy and requires amending the DAL (see issue #8065). For now,
detect LDAP and error out when a rename operation is attempted.
Greg Hudson [Thu, 19 Mar 2015 17:42:56 +0000 (13:42 -0400)]
Process TGS authdata after transited in KDC
The CAMMAC authorization data container requires a checksum over the
encrypted part of the issued ticket, with the CAMMAC contents
substituted for the authdata field. For this to work, we must
finalize the non-authdata fields of the encrypted ticket part before
adding authdata. Call handle_authdata() after checking and modifying
the transited field and potentially setting the
transited-policy-checked flag.
Also remove a redundant and inoperative conditional change to
enc_tkt_reply.times.starttime which happens after the ticket is
encrypted. We do the same thing right after setting up the ticket
times.
Greg Hudson [Tue, 17 Mar 2015 18:07:38 +0000 (14:07 -0400)]
Fix renewable ticket lifetimes
Commit b0661f9176f5eb2644ba459e1b1e87d3dd502174 removed the starttime
hack in the EncTicketPart decoder. Take this into account when
computing the old lifetime of a ticket we are renewing. Without this
fix, we compute an old lifetime equal to the ticket end time, add that
to the current KDC time, and issue a ticket with a negative end time
due to wraparound. Add a simple test to t_renew.py to detect this by
making sure that a renewed ticket is usable.
This bug appeared only on master and not as part of any release, so
there is no associated ticket.
In clnt_create(), do not set a timeout after creating the handle;
doing so defeats the purpose of keeping track of whether the caller
has set a handle timeout.
Greg Hudson [Sat, 14 Mar 2015 18:21:06 +0000 (14:21 -0400)]
Extend kadmin client timeout to one hour
Retrieving the list of principals can take a long time for some
databases. Extend the libkadm5 client timeout from two minutes to one
hour. (We can't easily remove the timeout entirely.)
Greg Hudson [Fri, 13 Mar 2015 17:30:49 +0000 (13:30 -0400)]
Log invalid restrictions strings
In kadm5int_acl_parse_restrictions(), output a log message if we break
out of the parsing loop with an error. The current structure of the
loop makes it difficult to pinpoint the bad restrictions field, so
just output the whole string.
Greg Hudson [Fri, 13 Mar 2015 16:45:27 +0000 (12:45 -0400)]
Document correct flag names for kadm5.acl
kadm5.acl entries can include restrictions which can force flag values
on or off. These flag values are parsed with krb5_string_to_flags(),
which means the flag names are the ones for default_principal_flags,
not the ones for kadmin addprinc/modprinc.
Greg Hudson [Thu, 12 Mar 2015 20:36:33 +0000 (16:36 -0400)]
Fix scope of kadmind ACL wildcard back-references
In kadm5int_acl_find_entry(), clear the wildcard back-references list
for each acl entry. Otherwise the wildcards we process can affect
back-references for later entries.
Solly Ross [Thu, 5 Mar 2015 18:22:58 +0000 (13:22 -0500)]
Import names immediately with COMPOSITE_EXPORT
RFC 6680 specifies that GSS_Export_name_composite() "outputs a token that
"can be imported with GSS_Import_name(), using GSS_C_NT_COMPOSITE_EXPORT
as the name type...". Therefore, in the gss_import_name mechglue, we
should perform the import process imediately when either
GSS_C_NT_COMPOSITE_EXPORT or GSS_C_NT_EXPORT_NAME are used (not just
for the later, as is the current functionality).
The naming extension test was also updated to display the result
of importing with GSS_C_NT_COMPOSITE_EXPORT in addition to
GSS_C_NT_EXPORT_NAME.
Greg Hudson [Mon, 23 Feb 2015 20:47:21 +0000 (15:47 -0500)]
Add test KDB module
Add a simple read-only KDB module which can be used to exercise KDB
behavior which the DB2 module cannot reach. Right now it supports
very basic get_principal functionality, aliases, and delegation
policy; in the future it could issue referrals or sign authdata.
Greg Hudson [Thu, 26 Feb 2015 20:02:37 +0000 (15:02 -0500)]
Fix kadmin script mode command-not-found error
In ss_wrapper.c, if ss_execute_command() returns an error, we should
call ss_perror() with *args as the third argument and not request
(which is NULL). Expand out the conditional into three commented
branches for greater clarity, since the error-handling is no longer
identical for the ss_execute_command() and ss_execute_line() cases.
Greg Hudson [Mon, 9 Feb 2015 17:38:06 +0000 (12:38 -0500)]
Use preauth timestamp in PKINIT clpreauth module
Use the timestamp from the KDC's preauth-required error when
generating a PKAuthenticator in pa_pkinit_gen_req(), to allow PKINIT
authentication to succeed despite client clock skew if kdc_timesync is
set.
Because this timestamp is unauthenticated (unless FAST is used), an
attacker could induce a legitimate client to generate a
PKAuthenticator for a future timestamp. But replaying this request in
the future would only cause the KDC to issue a ticket which the
attacker cannot decrypt.
Thomas Calderon [Fri, 6 Feb 2015 14:55:34 +0000 (15:55 +0100)]
Check timestamp in PKINIT kdcpreauth module
RFC 4556 requires the KDC to check the PKAuthenticator timestamp in
order to prevent replays after the five-minute clock skew window. (A
replay attack has minimal value; it only causes the KDC to issue a
ticket which an attacker cannot decrypt.)
[ghudson@mit.edu: rewrote commit message; squashed with typo fix;
style fixes]
Greg Hudson [Sat, 31 Jan 2015 05:29:59 +0000 (00:29 -0500)]
Use kadmin script mode in Python tests
In k5test, rename kadmin_local to kadminl and remove the run_kadminl()
K5Realm method. Update all scripts to use realm.run([kadminl, 'cmd',
...]). run_kadmin() still exists but takes an argument array instead
of a query string.
Where we touch test code, rename "output" to "out" (since "output" is
a function name exported by k5test.py), elide ":normal" from salt
strings, and use expressions like realm.krbtgt_princ instead of
manually composed principal names where appropriate. In
t_kadmin_acl.py, get rid of the delprinc() helper since the equivalent
is now concise enough to be written out each time. In t_policy.py,
remove some inoperative getprinc invocations and reorder some tests
which didn't correspond to their comment headers.
Greg Hudson [Fri, 30 Jan 2015 17:48:15 +0000 (12:48 -0500)]
Support kadmin script mode
Add support for a command and argments to be specified on the kadmin
command line, with script-friendly behavior. kadmin_startup() now
yields either a request string or a request argv array, and sets
script_mode in the argv array case. Informational messages now go
through info() and are suppressed if script_mode is set. Prompts and
warning messages are also suppressed in script mode. Error messages
indicating a failure now go through error() and set exit_status if
script_mode is set. The extended com_err() hook is always installed
so that com_err messages go through error() and set exit_status.
getopt() is now invoked with a leading '+' to suppress Gnu getopt
argument reordering behavior, so that invokers don't need to pass "--"
to prevent query options from being treated as kadmin options.
Non-Gnu getopt implementations should harmlessly treat '+' as a valid
flag option, which has no effect as it will reach the same default
label in the switch statement.
Greg Hudson [Tue, 9 Dec 2014 17:37:44 +0000 (12:37 -0500)]
Fix krb5_read_message handling [CVE-2014-5355]
In recvauth_common, do not use strcmp against the data fields of
krb5_data objects populated by krb5_read_message(), as there is no
guarantee that they are C strings. Instead, create an expected
krb5_data value and use data_eq().
In the sample user-to-user server application, check that the received
client principal name is null-terminated before using it with printf
and krb5_parse_name.
CVE-2014-5355:
In MIT krb5, when a server process uses the krb5_recvauth function, an
unauthenticated remote attacker can cause a NULL dereference by
sending a zero-byte version string, or a read beyond the end of
allocated storage by sending a non-null-terminated version string.
The example user-to-user server application (uuserver) is similarly
vulnerable to a zero-length or non-null-terminated principal name
string.
The krb5_recvauth function reads two version strings from the client
using krb5_read_message(), which produces a krb5_data structure
containing a length and a pointer to an octet sequence. krb5_recvauth
assumes that the data pointer is a valid C string and passes it to
strcmp() to verify the versions. If the client sends an empty octet
sequence, the data pointer will be NULL and strcmp() will dereference
a NULL pointer, causing the process to crash. If the client sends a
non-null-terminated octet sequence, strcmp() will read beyond the end
of the allocated storage, possibly causing the process to crash.
uuserver similarly uses krb5_read_message() to read a client principal
name, and then passes it to printf() and krb5_parse_name() without
verifying that it is a valid C string.
The krb5_recvauth function is used by kpropd and the Kerberized
versions of the BSD rlogin and rsh daemons. These daemons are usually
run out of inetd or in a mode which forks before processing incoming
connections, so a process crash will generally not result in a
complete denial of service.
Tom Yu [Mon, 9 Feb 2015 20:02:20 +0000 (15:02 -0500)]
Add configure checks for portability assumptions
Check a few portability assumptions:
* Integers are two's complement. Testing that the bitwise complement
of the representation of -1 is zero is sufficient, because C99
6.2.6.2 only allows sign and magnitude, one's complement, and two's
complement as representations.
* Integer values with sign bit one and value bits zero are valid and
not trap representations. C99 6.2.6.2 allows such a value to be a
trap representation. Testing that the declared integer value bounds
are asymmetric in magnitude is sufficient.
* Conversion of an unsigned integer value that is not representable in
the corresponding signed type preserves the bit pattern. C99
6.3.1.3 says this is implementation-defined, or raises an
implementation-defined signal. Exhaustively checking for the
desired behavior is prohibitive, so this spot check will have to do.
Tom Yu [Wed, 4 Feb 2015 22:01:14 +0000 (17:01 -0500)]
Avoid uninitialized data in t_prf.c
In t_prf.c, make sure that the partially initialized, faked-up
structures gss_union_ctx_id_desc and krb5_gss_ctx_id_rec are zeroed.
This avoids uninitialized reads in gss_pseudo_random(), which can
cause intermittent test failures on some platforms.
Greg Hudson [Wed, 4 Feb 2015 18:03:20 +0000 (13:03 -0500)]
Bump DAL major version for iterate change
Commit ab009b8568d9b64b7e992ecdb98114e895b4a7ff for issue #7977
changed the signature of krb5_db_iterate() and properly bumped
KRB5_KDB_API_VERSION from 7 to 8. It also changed the signature of
the DAL iterate() function, but did not bump
KRB5_KDB_DAL_MAJOR_VERSION. Bump that version from 4 to 5 now.
Greg Hudson [Mon, 29 Dec 2014 18:17:56 +0000 (13:17 -0500)]
Fix gssrpc data leakage [CVE-2014-9423]
[MITKRB5-SA-2015-001] In svcauth_gss_accept_sec_context(), do not copy
bytes from the union context into the handle field we send to the
client. We do not use this handle field, so just supply a fixed
string of "xxxx".
In gss_union_ctx_id_struct, remove the unused "interposer" field which
was causing part of the union context to remain uninitialized.
Greg Hudson [Mon, 29 Dec 2014 18:27:42 +0000 (13:27 -0500)]
Fix kadmind server validation [CVE-2014-9422]
[MITKRB5-SA-2015-001] In kadmind's check_rpcsec_auth(), use
data_eq_string() instead of strncmp() to check components of the
server principal, so that we don't erroneously match left substrings
of "kadmin", "history", or the realm.
Greg Hudson [Sat, 27 Dec 2014 19:16:13 +0000 (14:16 -0500)]
Fix kadm5/gssrpc XDR double free [CVE-2014-9421]
[MITKRB5-SA-2015-001] In auth_gssapi_unwrap_data(), do not free
partial deserialization results upon failure to deserialize. This
responsibility belongs to the callers, svctcp_getargs() and
svcudp_getargs(); doing it in the unwrap function results in freeing
the results twice.
In xdr_krb5_tl_data() and xdr_krb5_principal(), null out the pointers
we are freeing, as other XDR functions such as xdr_bytes() and
xdr_string().
Greg Hudson [Wed, 5 Nov 2014 16:58:04 +0000 (11:58 -0500)]
Fix gss_process_context_token() [CVE-2014-5352]
[MITKRB5-SA-2015-001] The krb5 gss_process_context_token() should not
actually delete the context; that leaves the caller with a dangling
pointer and no way to know that it is invalid. Instead, mark the
context as terminated, and check for terminated contexts in the GSS
functions which expect established contexts. Also add checks in
export_sec_context and pseudo_random, and adjust t_prf.c for the
pseudo_random check.
Greg Hudson [Mon, 26 Jan 2015 23:38:16 +0000 (18:38 -0500)]
Remove starttime hack in EncTicketPart decoder
The EncTicketPart decoder sets starttime to authtime if it wasn't
included in the ASN.1 value. This is problematic for upcoming CAMMAC
work, as we will need to re-encode a received EncTicketPart to check
the KDC verifier. Remove that behavior and just use opt_kerberos_time
for the starttime field. Adjust krb5_decode_test.c to match the new
decoder behavior.
Similarly, remove the process_tgs_req() code which sets starttime in
the header ticket if it isn't set. Add a comment explaining the
unrelated code adjacent to it.
check_tgs_times() used the ticket starttime without checking if it was
present. Add a fallback to times->authtime, and narrow the function
contract to make the implementation more concise.
There is a similar hack in the EncKDCRepPart decoder; leave that alone
for now.
Greg Hudson [Tue, 27 Jan 2015 03:34:49 +0000 (22:34 -0500)]
Remove special case for multi-hop SAM-2
Revert f20a77e879d203cdcb1bdbf9dc8e604a5187c88f (issue #7571). The
special case is no longer needed, as we are now resetting the tried
list for each KDC_ERR_PREAUTH_REQUIRED message.
In the KDC, allow kdcpreauth modules to return
KDC_ERR_MORE_PREAUTH_DATA_REQUIRED as defined in RFC 6113.
In libkrb5, treat this code like KDC_ERR_PREAUTH_REQUIRED. clpreauth
modules can use the modreq parameter to distinguish between the first
and subsequent KDC messages. We assume that the error padata will
include an element of the preauth mech's type, or at least of a type
recognized by the clpreauth module.
Also reset the list of previously attempted preauth types for both
kinds of errors. That list is really only appropriate for retrying
after a failed preauth attempt, which we don't currently do. Add an
intermediate variable for the reply code to avoid a long conditional
expression.
[ghudson@mit.edu: adjust get_in_tkt.c logic to avoid needing a helper
function; clarify commit message]
Simo Sorce [Tue, 20 Jan 2015 18:48:34 +0000 (13:48 -0500)]
Do not loop on principal unknown errors
If the canonicalize flag is set, the MIT KDC always return the client
principal when KRB5_KDC_ERR_C_PRICIPAL_UNKNOWN is returned.
Check that this is really a referral by testing that the returned
client realm differs from the requested one.
[ghudson@mit.edu: simplified and narrowed is_referral() contract.
Note that a WRONG_REALM response with e-data or FAST error padata
could now be passed through k5_preauth_tryagain() if it has an empty
crealm or a crealm equal to the requested client realm. Such a
response is unexpected in practice and there is nothing dangerous
about handling it this way.]
Greg Hudson [Sun, 4 Jan 2015 01:14:31 +0000 (20:14 -0500)]
Note skipped tests
In Python test scripts, use skipped() or skip_rest() as appropriate
when skipping tests. For Makefile-conditionalized tests, append to
$(SKIPTESTS) when skipping.
Greg Hudson [Sun, 4 Jan 2015 01:09:11 +0000 (20:09 -0500)]
Add framework for tracking skipped tests
In k5test.py, add functions skipped() and skip_rest() which output a
message about skipping tests (even without the verbose flag) and also
add a note to the "skiptests" file at the top of the build tree. In
the top-level make check, empty out skiptests at the beginning and
display it at the end. Add a subsitution for the skiptests file to
pre.in so that other makefiles can append to it.
Greg Hudson [Tue, 16 Dec 2014 17:57:56 +0000 (12:57 -0500)]
Fix bugs in previous cc_file.c changes
In fcc_destroy and krb5int_fcc_new_unique, call set_errmsg_filename
before deleting the cache handle, or else the reference to
data->filename is a use after free.
In set_errmsg_filename, do nothing if the code is 0, as we don't have
an error to annotate.
Greg Hudson [Wed, 14 Jan 2015 18:10:39 +0000 (13:10 -0500)]
Check for null *iter_p in profile_iterator()
In profile_iterator(), return PROF_MAGIC_ITERATOR if *iter_p is NULL,
instead of dereferencing a null pointer, as we did prior to 1.10.
Correct calling code will not trigger this case, but incorrect code
has been reported in the field.
Nicolas Williams [Thu, 30 Oct 2014 00:42:49 +0000 (19:42 -0500)]
Include file ccache name in error messages
When a FILE ccache method returns an error, append the filename to the
standard message for the code. Remove code to set extended messages
in helper functions as they would just be overwritten.
Also change the interpretation of errno values. Treat ENAMETOOLONG as
KRB5_FCC_NOFILE instead of KRB5_FCC_INTERNAL, since it has an external
cause and a name that long can't be opened by normal means. Treat
EROFS as KRB5_FCC_PERM. Treat ENOTDIR and ELOOP as KRB5_FCC_NOFILE
instead of KRB5_FCC_PERM as both errors imply that the full pathname
doesn't exist. Treat EBUSY and ETXTBSY as KRB5_CC_IO instead of
KRB5_FCC_PERM as they indicate a conflict rather than a permission
issue.
[ghudson@mit.edu: renamed set_error to set_errmsg_filename; removed
now-inoperative code to set extended messages in helper functions;
trimmed changes to interpret_errno; clarified and shortened commit
message]
Greg Hudson [Tue, 7 Oct 2014 16:12:11 +0000 (12:12 -0400)]
Use OFD locks where available
Linux 3.15 has added OFD locks, which contend with POSIX file locks
but are owned by the open file description instead of the process.
Use these in krb5_lock_file where available, for safer concurrency
behavior.
Ben Kaduk [Wed, 19 Nov 2014 17:04:46 +0000 (12:04 -0500)]
Support keyless principals in LDAP [CVE-2014-5354]
Operations like "kadmin -q 'addprinc -nokey foo'" or
"kadmin -q 'purgekeys -all foo'" result in principal entries with
no keys present, so krb5_encode_krbsecretkey() would just return
NULL, which then got unconditionally dereferenced in
krb5_add_ber_mem_ldap_mod().
Apply some fixes to krb5_encode_krbsecretkey() to handle zero-key
principals better, correct the test for an allocation failure, and
slightly restructure the cleanup handler to be shorter and more
appropriate for the usage. Once it no longer short-circuits when
n_key_data is zero, it will produce an array of length two with both
entries NULL, which is treated as an empty list by the LDAP library,
the correct behavior for a keyless principal.
However, attributes with empty values are only handled by the LDAP
library for Modify operations, not Add operations (which only get
a sequence of Attribute, with no operation field). Therefore, only
add an empty krbprincipalkey to the modlist when we will be performing a
Modify, and not when we will be performing an Add, which is conditional
on the (misspelled) create_standalone_prinicipal boolean.
CVE-2014-5354:
In MIT krb5, when kadmind is configured to use LDAP for the KDC
database, an authenticated remote attacker can cause a NULL
dereference by inserting into the database a principal entry which
contains no long-term keys.
In order for the LDAP KDC backend to translate a principal entry
from the database abstraction layer into the form expected by the
LDAP schema, the principal's keys are encoded into a
NULL-terminated array of length-value entries to be stored in the
LDAP database. However, the subroutine which produced this array
did not correctly handle the case where no keys were present,
returning NULL instead of an empty array, and the array was
unconditionally dereferenced while adding to the list of LDAP
operations to perform.
Versions of MIT krb5 prior to 1.12 did not expose a way for
principal entries to have no long-term key material, and
therefore are not vulnerable.
Greg Hudson [Fri, 5 Dec 2014 19:01:39 +0000 (14:01 -0500)]
Fix LDAP misused policy name crash [CVE-2014-5353]
In krb5_ldap_get_password_policy_from_dn, if LDAP_SEARCH returns
successfully with no results, return KRB5_KDB_NOENTRY instead of
returning success with a zeroed-out policy object. This fixes a null
dereference when an admin attempts to use an LDAP ticket policy name
as a password policy name.
CVE-2014-5353:
In MIT krb5, when kadmind is configured to use LDAP for the KDC
database, an authenticated remote attacker can cause a NULL dereference
by attempting to use a named ticket policy object as a password policy
for a principal. The attacker needs to be authenticated as a user who
has the elevated privilege for setting password policy by adding or
modifying principals.
Queries to LDAP scoped to the krbPwdPolicy object class will correctly
not return entries of other classes, such as ticket policy objects, but
may return success with no returned elements if an object with the
requested DN exists in a different object class. In this case, the
routine to retrieve a password policy returned success with a password
policy object that consisted entirely of zeroed memory. In particular,
accesses to the policy name will dereference a NULL pointer. KDC
operation does not access the policy name field, but most kadmin
operations involving the principal with incorrect password policy
will trigger the crash.
Greg Hudson [Mon, 8 Dec 2014 20:30:25 +0000 (15:30 -0500)]
Fix LDAP tests when sasl.h not found
Do not try to run the SASL EXTERNAL auth test if we could not define a
useful interact function. With current libraries the interact
function is asked for an authorization name, and the bind fails if it
gets an unsuccessful result or if no interaction function is defined.
Nicolas Williams [Wed, 12 Nov 2014 21:49:37 +0000 (15:49 -0600)]
Use new error message wrapping APIs
Define internal names k5_prendmsg and k5_wrapmsg and use them where we
amend error messages. This slightly changes the error message when we
fail to construct FAST AP-REQ armor, decrypt a FAST reply, or store
credentials in a gic_opts output ccache. Adjust the test suite for
the latter of those changes.
[ghudson@mit.edu: define and use internal names for brevity; pull in
test fix from later commit; expand commit message; fix redundant
separators in LDAP messages]
Nicolas Williams [Wed, 12 Nov 2014 21:47:53 +0000 (15:47 -0600)]
Add new error message wrapping APIs
Add four new public APIs for wrapping error messages:
krb5_prepend_error_message, krb5_vprepend_error_message,
krb5_wrap_error_message, and krb5_vwrap_error_message. The first two
functions are from Heimdal and allow a prefix to be added to the
existing message for a code. The latter two functions also allow the
code to be changed.
[ghudson@mit.edu: rename krb5_prepend_error_message2 to
krb5_wrap_error_message; clarify doxygen comments and put them in the
proper form; implement krb5_prepend_error_message in terms of
krb5_wrap_error_message; fix leak and null context handling in
krb5_wrap_error_message; rewrite commit message]
Ben Kaduk [Mon, 24 Nov 2014 23:23:32 +0000 (18:23 -0500)]
Don't fdopen() in append mode in cc_file.c
Implementations of fdopen() are inconsistent about the state of
the file offset after fdopen(., "a+") -- some position the stream
at the end of the file immediately (e.g., Solaris), for both reading
and writing, but others let reads occur from the beginning of the
file (e.g., glibc).
As it turns out, we only ever write to the file descriptor, not
through stdio, so opening the file with O_APPEND and using fdopen()
with "r+b" should give us sufficient append semantics, while
more portably letting the stream read from the beginning of the file.
Ben Kaduk [Thu, 20 Nov 2014 20:44:04 +0000 (15:44 -0500)]
Avoid infinite loop on duplicate keysalts
When duplicate suppression was requested, we would enter an
infinite loop upon encountering a duplicate entry, a bug
introduced in commit 0918990bf1d8560d74473fc0e41d08d433da1a15
and thus present in release 1.13.
Rework the conditional to avoid the loop, at the expense of
additional indentation for some of the code.
Greg Hudson [Tue, 4 Nov 2014 15:13:11 +0000 (10:13 -0500)]
Fix minor cleanup issue in file ccache
If we fail to open the cache file in fcc_initialize, we could wind up
calling close(-1) which is harmless but incorrect. Avoid this by
initializing fd and conditionalizing its cleanup.
Greg Hudson [Wed, 5 Nov 2014 19:12:35 +0000 (14:12 -0500)]
Fix input race condition in t_skew.py
In two of the kinit tests run by t_skew.py, we expect kinit to exit
before reading the password. If we supply a password input for those
commands, we can fail with a broken pipe exception if the master
process tries to write the password after the slave process exits.
Also correctly check the output of the last kinit invocation.
Greg Hudson [Mon, 3 Nov 2014 22:27:00 +0000 (17:27 -0500)]
Fix spurious gcc warning in cc_file.c
gcc 4.6.3 (present in Ubuntu 12.04) is smart enough to look at
get_size and see that it does not always assign to *size_out, but not
smart enough to figure out that it always assigns to *size_out when it
returns 0. As a result, it outputs two warnings which we treat as
errors. Add an initial assignment to *size_out at the beginning of
get_size to work around this.
Greg Hudson [Sun, 12 Oct 2014 22:46:17 +0000 (18:46 -0400)]
Use stdio reads, O_APPEND writes in FILE ccache
Remove open file state from the cache handle, use stdio for reading,
use single O_APPEND writes for writing, and use O_CLOEXEC when
opening. Keep the file handle open during iteration. These changes
simplify the code, fix some concurrency issues, and reduce the
dependency on POSIX file locks. We still acquire file locks for
compatibility with older code, and in case O_APPEND writes aren't
concurrency-atomic.
Helper functions change as follows:
* open_cache_file yields a stdio handle, and only opens and locks.
* close_cache_file takes a stdio handle.
* read_header (new) reads the file header and yields a version.
* invalidate_cache and fcc_lseek are no longer needed.
* get_size, read_bytes, and load_bytes operate on a stdio handle.
* read32, read16, load_data, load_principal, and load_cred operate on
a stdio handle and version.
* write_bytes, store32, store16, and store_principal are no longer
needed.
fcc_initialize now takes responsibility for writing the header and
default client principal, using a single write.
Greg Hudson [Tue, 7 Oct 2014 00:09:27 +0000 (20:09 -0400)]
Remove cc_file.c global lookup table
The FILE ccache type maintains a global reference-counted table of
handles, which is perhaps an imperfect workaround for POSIX
per-process file locks. Remove this table, since we plan to maintain
read fds in cursors and use O_APPEND writes to render locking less
important.
Greg Hudson [Mon, 6 Oct 2014 13:47:10 +0000 (09:47 -0400)]
Remove KRB5_TC_OPENCLOSE handling in FILE ccache
Stop processing the KRB5_TC_OPENCLOSE flag in cc_file.c; always reopen
the file instead. This will be replaced with more efficient cursor
handling. Also remove some unused KRB5_TC_OPENCLOSE macros in scc.h.
projects / thirdparty / krb5.git / log
