Greg Hudson [Fri, 10 May 2013 18:01:48 +0000 (14:01 -0400)]
Assume mutex locking cannot fail
Locking and unlocking a non-recursive mutex is a simple memory
operation and should not fail on any reasonable platform with correct
usage. A pthread mutex can return EDEADLK on lock or EPERM on unlock,
or EINVAL if the mutex is uninitialized, but all of these conditions
would reflect serious bugs in the calling code.
Change the k5_mutex_lock and k5_mutex_unlock wrappers to return void
and adjust all call sites. Propagate this change through
k5_cc_mutex_lock and k5_cc_mutex_unlock as well.
Greg Hudson [Tue, 14 May 2013 02:59:35 +0000 (22:59 -0400)]
Rename internal Camellia symbols
Symbols from the NTT Camellia sources, used in the builtin crypto
provider, could conflict with symbols from other libraries such as
OpenSSL's libcrypto. Rename those like we rename the Gladman AES
symbols.
Tom Yu [Fri, 3 May 2013 20:26:46 +0000 (16:26 -0400)]
Fix kpasswd UDP ping-pong [CVE-2002-2443]
The kpasswd service provided by kadmind was vulnerable to a UDP
"ping-pong" attack [CVE-2002-2443]. Don't respond to packets unless
they pass some basic validation, and don't respond to our own error
packets.
Some authors use CVE-1999-0103 to refer to the kpasswd UDP ping-pong
attack or UDP ping-pong attacks in general, but there is discussion
leading toward narrowing the definition of CVE-1999-0103 to the echo,
chargen, or other similar built-in inetd services.
Thanks to Vincent Danen for alerting us to this issue.
Some fixes, some use of different APIs which seem to clean things up
better, with the goal of being able to cleanly shut down NSS when we're
done using it.
* Use PK11_FreeSlot() instead of SECMOD_CloseUserDB() to close a
database opened with SECMOD_OpenUserDB().
* Fix a typo and use PK11_DestroyGenericObject() instead of
PK11_DestroyGenericObjects() to destroy one object.
* Use SECMOD_DestroyModule() instead of SECMOD_UnloadUserModule()
to close a module loaded with SECMOD_LoadUserModule().
* crypto_check_for_revocation_information(): don't leak a reference
to the CRL, or to intermediate issuers.
* Don't leak a reference to a PEM private key.
When the PEM module is given an encrypted key, it changes its token
flags to indicate that a password is required (by setting needs-login)
to signal the application that we need to supply a password to decrypt
it. Attempts to load any other items will fail until the flag is
cleared.
If we detect that the flag is set after we've attempted to load a
private key, attempt to "log in" to the "token" using a password. Even
if we fail, the token will reset its needs-login flag, which is
necessary before we can import anything else.
Get better at locating the just-loaded certificate
When loading certificates using the PEM module, use a better method for
finding the just-loaded certificate that will still work if we've
already got a copy of the certificate loaded somewhere else.
In PKINIT NSS crypto code, load certificates first
When using NSS's CMS API to generate signed-data messages, we identify
the key that we want to use for signing by specifying a certificate.
The library then looks up the corresponding private key when it needs to
generate the signature. This lookup fails if a certificate and a its
corresponding private key were loaded key-first, but succeeds if they
were loaded certificate-first (RHBZ#859535). To work around this,
switch to loading the certificate first. (We switch to using different
_pkinit_identity_crypto_file pointers for each instead of reusing just
one, so the diff is messier than it might have been.)
When DEBUG=1, log why we fail to log in to a token
Use PORT_ErrorToName() to let us print an error name instead of an error
code in a couple of debug messages, since in practice we just end up
looking up the code in <secerr.h> anyway.
Nalin Dahyabhai [Thu, 10 Jan 2013 20:39:15 +0000 (15:39 -0500)]
Traverse tokens like we do with OpenSSL for NSS
When PKINIT is built with NSS, change how it traverses tokens to match
the way it's done when built using OpenSSL: ignore slot names (we used
to treat the token label as a possible slot label, too), and either only
look at the token with the specified label, or the first token if a no
token label was specified.
Nalin Dahyabhai [Tue, 18 Dec 2012 22:41:11 +0000 (17:41 -0500)]
Make the text of NSS's prompts look like OpenSSL's
When PKINIT is built with NSS, make the text of prompts that we issue to
the user better match the text we use when we build with OpenSSL: ask
for a pass phrase when we're asking about a hardware token, ask for a
password the rest of the time, and take advantage of translations for
requests for a password.
Nalin Dahyabhai [Mon, 14 Jan 2013 18:57:54 +0000 (13:57 -0500)]
Don't fail if a candidate certificate has no SANs
When we're doing certificate matching and we're asked for the list of
SAN values for a certifiate, and it contains none, don't return an
error, as that will eventually cause the module to just return an error.
Instead, just return an empty list of SAN values so that processing will
continue on to check if other certificates match.
Nalin Dahyabhai [Tue, 26 Feb 2013 22:59:01 +0000 (17:59 -0500)]
Make reassembled PKCS11 names parseable
The reassembled names used "," as a separator between attributes, when
passed-in values use ":". This was due to the original submitter being
confused - they weren't intended to be different.
Fix a typo that caused us to not skip removing "."
When attempting to clean the files out from our temporary directory,
correct the test which was supposed to let us skip over "." and ".." so
that we actually don't try to remove them with remove().
The test, as submitted, included a copy/paste error which caused it to
test PKINIT using unencrypted PKCS12 bundles twice, and to not test a
DIR: location containing unencrypted PEM-formatted keys at all.
Encrypted timestamp and encrypted challenge cannot succeed if the
client has no long-term key matching the request enctypes, so do not
offer them in that case.
Add a new have_client_keys callback to the kdcpreauth interface,
allowing modules to efficiently check whether the client DB entry has
any keys matching the request enctypes.
Greg Hudson [Wed, 1 May 2013 17:07:36 +0000 (13:07 -0400)]
Don't use portmapper in RPC tests
On many Linux systems, due to what is arguably a bug in rpcbind, the
portmapper doesn't allow service registration from non-root processes.
This causes the RPC tests to be frequently skipped. Modify the tests
so that they don't need the portmapper, by grabbing the port number
from the server process and passing it to the client.
Greg Hudson [Wed, 1 May 2013 18:40:31 +0000 (14:40 -0400)]
Disable UDP pass of gssrpc tests on all platforms
The AUTH_GSSAPI flavor of rpc authentication uses IP address channel
bindings. These are broken over UDP, because svcudp_recv() fails to
get the destination address of incoming packets (it tries to use the
recvmsg() msg_name field to get the destination IP address, which
instead gets the source address; see ticket #5540).
There is no simple or comprehensive way to fix this; using IP_PKTINFO
is a fair amount of code and only works on some platforms. It's also
not very important--nobody should be using AUTH_GSSAPI except perhaps
for compatibility with really old kadmin, and kadmin only runs over
TCP. Since the gssrpc tests are closely wedded to AUTH_GSSAPI, the
simplest fix is to only run the TCP pass.
Greg Hudson [Wed, 1 May 2013 19:52:40 +0000 (15:52 -0400)]
Disable the gssrpc expired-cred test
The "expired" test in expire.exp tries to authenticate to the server
process with an expired TGT (obtained using kinit -l -1m). Using an
expired TGT to get an expired service cred no longer works after
#6948. We could use kinit -S to get an expired service cred, but
krb5_get_credentials won't return expired service cred from the cache
(even before #6948). We could use time offsets to simulate clock skew
between the client and server process, but that would be difficult
because the test programs don't have access to the krb5_context
objects used by the client or server process. Since we don't have a
simple workaround, disable the test.
We want to generate a KRB5_AP_ERR_TKT_EXPIRED code when the TGT is
expired, like we would if we tried the TGT against the KCD. To make
this work, separate the helpers for getting local and crossrealm
cached TGTs. For a crossrealm TGT, match against the endtime, as
there could be multiple entries. For a local TGT, find any match, but
check if it's expired. The cache_code field is no longer needed after
this change, so get rid of it.
Nalin Dahyabhai [Thu, 10 Jan 2013 23:30:04 +0000 (18:30 -0500)]
Add various client-authenticating PKINIT tests
Add tests for non-anonymous PKINIT:
* FILE: with no password
* FILE: with a password
* DIR: with no password
* DIR: with a password
* PKCS12: with no password
* PKCS12: with a password
* PKCS11: with a password, if soft-pkcs11.so is found via ctypes
[ghudson@mit.edu: reformatted to 79 columns; removed intermediate
success() calls]
Ben Kaduk [Thu, 18 Apr 2013 22:18:57 +0000 (18:18 -0400)]
Fix doc build
The addition of the KRB5_PADATA_AS_CHECKSUM macro in d7d74867952f caused
the doxygen bridge to emit a new RST file. This file was not included in
the API reference toctree, causing a build failure in maintainer-mode.
An RFC 6113 KrbFastReq contains a padata sequence and a KDC-REQ-BODY,
neither of which contain the msg-type field found in a KDC-REQ. So
when we decode the FAST request, the resulting krb5_kdc_req structure
has a msg_type of 0. Copy msg_type from the outer body, since we make
use of it in further KDC processing.
Tom Yu [Wed, 10 Apr 2013 03:47:54 +0000 (23:47 -0400)]
Allow config of dh_min_bits < 2048
Allow configuration to override the default dh_min_bits of 2048 to
1024. Disallow configuration of dh_min_bits < 1024, but continue to
default to 2048.
Tom Yu [Fri, 29 Mar 2013 02:57:55 +0000 (22:57 -0400)]
Simplify pkinit_check_dh_params
Change pkinit_check_dh_params() to take two DH* parameters, and only
compare p and g, because q is fully determined by them and might be
missing.
Also refactor some parameter checks into check_dh_wellknown() that
were previously done separately in the pkinit_process_td_dh_params()
and server_check_dh().
Wait ten seconds for a TCP connection to succeed or fail before moving
on. During this wait time, other TCP connections will be serviced if
we already initiated them, but no new TCP connections will be created
and no UDP packets will be retransmitted.
[ghudson@mit.edu: minor adjustments; commit message]
Replace the end_time field of struct select_state with an endtime
argument to cm_select_or_poll, expressed in milliseconds since the
epoch. Add a helper function to get the current time in that format.
Use a millisecond interval argument to service_fds for consistency.
Since net-server.c now uses libverto, only sendto_kdc.c consumes cm.c.
Move stuff out of cm.c and cm.h into sendto_kdc.c and get rid of them.
Change the sendto_kdc callback (used by chpw.c) to receive the socket
descriptor instead of the entire conn_state structure, and move the
declarations into os-proto.h. struct remote_address also needs to be
in os-proto.h so that trace.c and t_trace.c can use it. k5_curtime
isn't needed since k5-platform.h now guarantees the presence of
gettimeofday().
In struct conn_state, collect together the fields for the remote
address and put them in a substructure. Pass this substructure to
trace logging macros instead of the entire conn_state structure, so
that trace.c doesn't have to know about the whole structure.
Since krb5 1.3, krb5_get_host_realm (and therefore
krb5_sname_to_principal) has refused hostnames which appear to be
numeric addresses--with the exception of 1.6, which was ignoring
errors from clean_hostname. In specialized environments, it may be
desirable to use IP addresses in service principal names, and there's
no compelling reason for us to get in the way of that.
Move the numeric address check out of k5_clean_hostname into a new
helper function, and simply skip the domain-based mechanisms if it
returns true. Factor out the [domain_realm] search into a second new
helper function to make it easier to skip.
After k5memdup0 was slightly modified in 31124ffb81e8c0935403a9fdc169dead5ecaa777, some older versions of gcc
complain about outpos being possibly used before it is initialized.
This can't actually happen, but we can silence the error and also
simplify how outpos is initialized.
By a strict reading of the C standard, memcpy and memcmp have
undefined behavior if their pointer arguments aren't valid object
pointers, even if the length argument is 0. Compilers are becoming
more aggressive about breaking code with undefined behavior, so we
should try to avoid it when possible.
In a krb5_data object, we frequently use NULL as the data value when
the length is 0. Accordingly, we should avoid copying from or
comparing the data field of a length-0 krb5_data object. Add checks
to our wrapper functions (like data_eq and k5_memdup) and to code
which works with possibly-empty krb5_data objects. In a few places,
use wrapper functions to simplify the code rather than adding checks.
Modify t_credstore.c to be more flexible and adjust t_gssapi.py
accordingly. Add a test to t_client_keytab.py which acquire creds
using a programmatically specified client keytab.
Simo Sorce [Thu, 28 Mar 2013 16:53:01 +0000 (12:53 -0400)]
Add support for client keytab from cred store
The new credential store extensions added support for specifying a
specific ccache name and also a specific keytab to be used for accepting
security contexts, but did not add a way to specify a client keytab
to be used in conjunction with the Keytab initiation support added also
in 1.11
This patch introduces a new URN named client_keytab through which a
specific client_keytab can be set when calling gss_acquire_cred_from()
and Keytab Initiation will use that keytab to initialize credentials.
Greg Hudson [Fri, 29 Mar 2013 06:22:12 +0000 (02:22 -0400)]
Fix errno hygiene in kadmind write_pid_file
fclose() might overwrite the errno value from fprintf, causing us to
return success when we shouldn't. Record the errno value at the time
of the fprintf failure.
Greg Hudson [Fri, 29 Mar 2013 06:13:04 +0000 (02:13 -0400)]
Simplify krb5_ldap_readpassword
There's no need to check whether the file exists and is readable
before opening it, and setting an extended error message which is just
strerror_r() of the errno value isn't useful.
Greg Hudson [Thu, 28 Mar 2013 20:43:30 +0000 (16:43 -0400)]
Using k5-int.h data helpers for some functions
Use empty_data(), alloc_data(), and make_data() in some appropriate
places. This has the side effect of initializing the krb5_data magic
field, which can placate debugging tools.
Greg Hudson [Mon, 25 Mar 2013 21:38:41 +0000 (17:38 -0400)]
Fix a trivial file leak writing kadmind pid file
If we fail to write the pid to the pid file, we should still close the
file before returning from write_pid_file(). The consequences of this
bug are trivial because kadmin is just going to exit regardless.
Reported by Will Fiveash <will.fiveash@oracle.com>.
Greg Hudson [Mon, 25 Mar 2013 16:42:49 +0000 (12:42 -0400)]
Export verto_set_flags from libverto
When the bundled libverto was updated from 0.2.2 to 0.2.5,
verto_set_flags should have been added to libverto.exports along with
the other new functions.
Simo Sorce [Sat, 16 Mar 2013 19:23:03 +0000 (15:23 -0400)]
Fix import_sec_context with interposers
The code was correctly selecting the mechanism to execute, but it was
improperly setting the mechanism type of the internal context when the
selected mechanism was that of an interposer and vice versa.
When an interposer is involved the internal context is that of the
interposer, so the mechanism type of the context needs to be the
interposer oid. Conversely, when an interposer re-enters gssapi and
presents a token with a special oid, the mechanism called is the real
mechanism, and the context returned is a real mechanism context. In
this case the mechanism type of the context needs to be that of the
real mechanism.
Greg Hudson [Sun, 24 Mar 2013 05:28:13 +0000 (01:28 -0400)]
Move a bunch of stuff out of k5-int.h
Move internal declarations from k5-int.h to more localized headers
(like int-proto.h) where appropriate. Rename many symbols whose
prototypes were moved to use the k5_ prefix instead of krb5int_.
Remove some unused declarations or move them to the single source file
they were needed in. Remove krb5_creds_compare since it isn't used
any more.
Ben Kaduk [Thu, 21 Mar 2013 15:49:49 +0000 (11:49 -0400)]
Rebuild NOTICE for 2013
Also exclude copyright.rst from the notice.txt build, as maintainer-mode
builds error out due to the "document isn't included in any toctree"
warning otherwise produced.