We later subtract 8 when calculating the length of the output message
buffer. If padlength is excessively high, this calculation can underflow
and result in a very large positive value.
Now we properly constrain the value of padlength so underflow shouldn't
be possible.
Samba BUG: https://bugzilla.samba.org/show_bug.cgi?id=15134
Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
If len_len is equal to total_len - 1 (i.e. the input consists only of a
0x60 byte and a length), the expression 'total_len - 1 - len_len - 1',
used as the 'len' parameter to der_get_length(), will overflow to
SIZE_MAX. Then der_get_length() will proceed to read, unconstrained,
whatever data follows in memory. Add a check to ensure that doesn't
happen.
Samba BUG: https://bugzilla.samba.org/show_bug.cgi?id=15134
Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
By decrementing 'pad' only when we know it's safe, we ensure we can't
stray backwards past the start of a buffer, which would be undefined
behaviour.
In the previous version of the loop, 'i' is the number of bytes left to
check, and 'pad' is the current byte we're checking. 'pad' was
decremented at the end of each loop iteration. If 'i' was 1 (so we
checked the final byte), 'pad' could potentially be pointing to the
first byte of the input buffer, and the decrement would put it one
byte behind the buffer.
That would be undefined behaviour.
The patch changes it so that 'pad' is the byte we previously checked,
which allows us to ensure that we only decrement it when we know we
have a byte to check.
Samba BUG: https://bugzilla.samba.org/show_bug.cgi?id=15134
Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
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 definitions of memcpy(), memmove(), and memset() state that
the behaviour is undefined if any of the pointer arguments are
NULL, and some compilers are known to make use of this to
optimise away existing NULL checks in the source.
Change-Id: I489bc256e3eac7ff41d91becb0b43aba73dbb3f9
Link: https://www.imperialviolet.org/2016/06/26/nonnull.html
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
80f3194a76
("gssapi/krb5/{export,import}_sec_context: make smaller tokens.")
stored the source principal when it should have stored the target
principal.
Change-Id: Ife6b137f9fe8f63cdb78b4212f74d502080ec2a2
We have a Heimdal special where when the acceptor sends back an error
token for clock skew or ticket-not-yet-valid errors then the acceptor
application will get GSS_S_CONTINUE_NEEDED from gss_accept_sec_context()
so that the initiator may retry with the same context.
But we were retaining the auth_context, which means that when the
initiator does send a new token, the acceptor leaks memory because
krb5_verify_ap_req2() doesn't clean up the auth_context on reuse. The
end result is that we leak a lot in those cases.
The implementation of GSS name attributes for Kerberos (or any mechanism
with more than a tiny handful) is much nicer as a table-driven
implementation.
We now have stubs for setting and deleting attributes as well, though
these currently always fail.
When unsigned char values are shifted, they are promoted to int (unless
sizeof(int) == sizeof(char)). This means that the change in be708ca3cf
ultimately leads to a sign extension bug.
This adds Kerberos mechanism support for:
- composite principal name export/import
- getting rudimentary name attributes from GSS names using
gss_get_name_attribute():
- all (raw) authorization data from the Ticket
- all (raw) authorization data from the Authenticator
- transit path
- realm
- component count
- each component
- gss_inquire_name()
- gss_display_name_ext() (just for the hostbased service name type
though)
The test exercises almost all of the functionality, except for:
- getting the PAC
- getting authz-data from the Authenticator
- getting the transit path
TBD (much) later:
- amend test_context to do minimal name attribute checks as well
- gss_set_name_attribute() (to request authz-data)
- gss_delete_name_attribute()
- getting specific authorization data elements via URN fragments (as
opposed to all of them)
- parsing the PAC, extracting SIDs (each one as a separate value)
- some configurable local policy (?)
- plugin interface for additional local policy
Setting `dns_lookup_realm = false` in `[libdefaults]` and setting name
canon rules that force the empty realm causes destination-TGT delegation
to break because the client doesn't know the service's realm.
Because MIT and Heimdal check that the (unauthenticated plaintext)
sname/realm of the Ticket in the KDC reply matches the sname/srealm in
the enc-part of the KDC reply, we know we can trust the realm of the
ticket found in the ccache. So use that.
Samba3 sends an AP-REQ, rather than 8003, checksum in a Kerberos inital context
token. This regressed in #835 as we forgot to set the
KRB5_CRYPTO_FLAG_ALLOW_UNKEYED_CHECKSUM flag before processing the AP-REQ
checksum in this path.
Although not required to address bad code generation in
some versions of gcc 9 and 10, a coding style that requires
explicit comparison of the result to zero before use is
both clearer and would have avoided the generation of bad
code.
This change converts all use of cmp function usage from
```
if (strcmp(a, b) || !strcmp(c, d)) ...
```
to
```
if (strcmp(a, b) != 0 || strcmp(c, d)) == 0
```
for all C library cmp functions and related:
- strcmp(), strncmp()
- strcasecmp(), strncasecmp()
- stricmp(), strnicmp()
- memcmp()
Change-Id: Ic60c15e1e3a07e4faaf10648eefe3adae2543188
As documented in Russell O'Connor's blog, Heimdal when compiled with
some versions of gcc 9 and 10 would generate incorrect behaviors from
_gssapi_verify_mic_arcfour(), _gssapi_unwrap_arcfour(),
_gssapi_unwrap_iov_arcfour() and _gssapi_unwrap_iov_arcfour().
As a result of the bug, code of the form
if (memcmp(a, "\x00\x00\x00\x00"))
and
cmp = memcmp(a, "\x00\x00\x00\x00")
will be compiled as if it were written as
if (strcmp(a, "\x00\x00\x00\x00"))
and
cmp = strcmp(a, "\x00\x00\x00\x00")
but not
if (memcmp(a, "\x00\x00\x00\x00") != 0)
and
cmp = (memcmp(a, "\x00\x00\x00\x00") != 0)
Bad code is generated whenever one of the parameters to memcmp()
is a constant with at least one NUL in the first four octets and
the return value is used immediated without a boolean comparison.
The gcc bug 95189 has since been fixed.
This change applies a defensive programming technique to avoid
the broken code generation.
Change-Id: I1db2a561735317cb6cac66a0ec9caf5443e65e03
Link: https://r6.ca/blog/20200929T023701Z.html
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95189
Reported-by: Buck Huppmann (buckh@pobox.com) <buckh@pobox.com>
ad3acc2aef ("lib/gssapi/krb5:
implement gss_[un]wrap_iov[_length] with arcfour-hmac-md5")
introduced a duplicate block of code.
This change removes the unnecessary code duplication.
Change-Id: I557c590aea51b73dc25d6ce4be4ea84b9dfadd9f
Correctly implement gss_krb5_ccache_name() in terms of
gss_set_sec_context_option(GSS_KRB5_CCACHE_NAME_X). The previous implementation
was a NOOP.
Note: global ccache name should really be thread-specific rather than global.
Closes#803.
The functions for storing and retrieving GSS OIDs and buffers from
krb5_storage, added in 6554dc69, are generally useful. Move those into private
_gss_mg_XXX() API and update gss_{export,import}_{cred,sec_context} to use them
where appropriate.
The recently introduced gss_mg_name_to_oid() function supported looking up
dynamically loaded mechanisms by name, but did not support partial matches or
the legacy "Kerberos 5" name as supported by gss_name_to_oid().
Consolidate these into a single function, and also add support for dynamically
loaded mechanisms to gss_oid_to_name().
API behavior difference: the Kerberos mechanism is now referred to by "krb5"
rather tha "Kerberos 5", although for legacy compatibility gss_name_to_oid()
will recognize the old name. However, gss_oid_to_name() will return "krb5". The
anticipated impact is minimal as these are not standard GSS-APIs and do not
appear to have any public usage outside Heimdal.
When using these functions with gss_init_sec_context(), we noticed
that some things were missing and some needed to be made optional.
ctx->order may be NULL, ctx->ac->authenticator needs to be filled
out, and ctx->state needs be stored.
Note: SPNEGO still needs a little more work.
