While we no longer have a decoder CHOICE element 0 bug, we did still
have one encode and copy and free that was leading to a memory leak (and
_save trashing) prior to the fix for
asn1: Fix 1-byte leaks in der_copy_octet_string()
This commit fixes that.
We sometimes do things like `memset(&cert, 0, sizeof(cert))` then
`copy_Certificate(&cert, &cert_copy)`, and then we end up leaking a
byte in `der_copy_octet_string()` due to it having this code:
```C
der_copy_octet_string (const heim_octet_string *from, heim_octet_string *to)
{
assert(from->length == 0 || (from->length > 0 && from->data != NULL));
if (from->length == 0)
to->data = calloc(1, 1);
else
to->data = malloc(from->length);
...
}
```
The traces where this happens always involve the `_save` field of
`Name` or `TBSCertificate`.
This code was assuming that length 0 octet strings are expected to have
a non-NULL `data`, probably in case the C library's allocator returns
non-NULL pointers for `malloc(0)`, but then, why not just call
`malloc(0)`? But calling `malloc(0)` would then still lead to this leak
in on such systems.
Now, `der_free_octet_string()` does unconditionally `free()` the
string's `data`, so the leak really is not there but elsewhere, probably
in `lib/asn1/template.c:_asn1_free()`, but it clearly does
`der_free_octet_string()` the `_save` field of types that have it.
5398425c introduced support for propagating ASN.1 default values to the emitted
JSON, but it neglected to quote string values, which caused ASN.1 parsing
errors. Correct this.
OpenSSL's d2i_ECPrivateKey() is deprecated, so we have to use
d2i_PrivateKey(), but d2i_PrivateKey() wants the whole PKCS#8 blob so it
can know what kind of key it is. So we need to let the hx509 EC layer
get that blob. The internal APIs need some refactoring, so for now we
use a hack where we try to parse the private key with and without the
PKCS#8 wrapper.
At some point before we make an 8.0 release we'll probably just remove
all the legacy, weak ciphers and hashes (except MD5, most likely).
To drop these we'll have to re-generate PKCS#12 test samples using
stronger PBEs, and possible add new PBE types.
This check is admittedly lame. But it's all I have time for at the
moment. A better check would be a program that includes the correct
headers and succeeds if the OpenSSL version macro indicates it's at
3.0 or higher. Or perhaps we could run the openssl(1) version command-
line and parse its output. But checking for functions that are in 3.0
and not 1.1 will do for the time being.
This has no real effect, but make things more clear
and matches the pattern for krb5_sendto_context().
Signed-off-by: Stefan Metzmacher <metze@samba.org>
This matches krb5_init_creds_step() from MIT. The only
difference is the type 'krb5_realm' (Heimdal) vs. 'krb5_data' (MIT).
krb5_error_code KRB5_CALLCONV
krb5_init_creds_step(krb5_context context,
krb5_init_creds_context ctx,
krb5_data *in,
krb5_data *out,
krb5_data *realm,
unsigned int *flags);
NOTE: commit 1cdc9d5f3c
"krb5: export krb5_init_creds_step()" exported
krb5_init_creds_step() the first time, but that's
not in any released version, so it should be fine
to fix up the prototype in order to make the
function actually useful for external callers.
Signed-off-by: Stefan Metzmacher <metze@samba.org>
It should not return pointers to the internal state,
this matches the way the krb5_init_creds_step() works in MIT.
NOTE: commit 1cdc9d5f3c
"krb5: export krb5_init_creds_step()" exported
krb5_init_creds_step() the first time, but that's
not in any released version, so it should be fine
to change the behavior as there can't be any
external users of the function.
Signed-off-by: Stefan Metzmacher <metze@samba.org>
_krb5_fast_anon_pkinit_step() should not set
KRB5_INIT_CREDS_STEP_FLAG_CONTINUE if it doesn't generate any output.
And krb5_init_creds_step() needs to return if
_krb5_fast_anon_pkinit_step() returned with
KRB5_INIT_CREDS_STEP_FLAG_CONTINUE set.
As that means the recursive call to krb5_init_creds_step()
generated output that should be send to a KDC and the
KDC response if needed as input for the next step.
Signed-off-by: Stefan Metzmacher <metze@samba.org>
The current prototype of krb5_init_creds_step() is completely
useless as the caller has no way to know the destination
realm for the out blob.
The only internal caller of krb5_init_creds_step()
passes hostinfo=NULL and this commit makes it more obvious that hostinfo
is always NULL.
NOTE: commit 1cdc9d5f3c
"krb5: export krb5_init_creds_step()" exported
krb5_init_creds_step() the first time, but that's
not in any released version, so it should be fine
to fix up the prototype.
The aim is to remove hostinfo from the krb5_init_creds_step() internals
completely and move krb5_init_creds_step() to a useful prototype
where it returns the destination realm for the out packet.
Which means the prototype will mostly match the one MIT is using:
krb5_error_code KRB5_CALLCONV
krb5_init_creds_step(krb5_context context,
krb5_init_creds_context ctx,
krb5_data *in,
krb5_data *out,
krb5_data *realm,
unsigned int *flags);
Follow up patches demonstrate that the hostinfo related code
in pk_verify_host() is actually dead code as all layers
just passed down the NULL value from krb5_init_creds_get().
Signed-off-by: Stefan Metzmacher <metze@samba.org>
Heimdal's HDB plugin interface, and hence Samba's KDC that depends upon
it, doesn't work on 32-bit builds due to structure fields being arranged
in the wrong order. This problem presents itself in the form of
segmentation faults on 32-bit systems, but goes unnoticed on 64-bit
builds thanks to extra structure padding absorbing the errant fields.
This commit reorders the HDB plugin structure fields to prevent crashes
and introduces a common macro to ensure every plugin presents a
consistent interface.
Samba BUG: https://bugzilla.samba.org/show_bug.cgi?id=15110
Signed-off-by: Joseph Sutton <josephsutton@catalyst.net.nz>
Record when a CHOICE field is promoted from IMPLICIT to EXPLICIT and convey
this in the ASN.1 compiler's JSON output, so that other tools (e.g. which have
a representation isomorphic to the original ASN.1) may use it.
While the local stack pointers could be thought of as "only"
numbers that are not invalidated by the memory they point at
being freed, any use of the pointer after the free is undefined
and so warned about (at best).
gcc version 12.2.1 20220819 (Red Hat 12.2.1-1) (GCC)
Signed-off-by: Andrew Bartlett <abartlet@samba.org>
We can't rely on having every KDC support FAST and should still
support S4U2Self against such a KDC.
We also have the order of the PA-DATA elements "corrected",
KRB5_PADATA_TGS_REQ followed by KRB5_PADATA_FX_FAST and
finally KRB5_PADATA_FOR_USER. While the inner PA-DATA
only contains KRB5_PADATA_FOR_USER.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=15002
Signed-off-by: Stefan Metzmacher <metze@samba.org>
This is needed when a service provides support for newer entry.etypes
without having a key for that encryption type yet.
It is triggered with svc_use_strongest_session_key=TRUE.
Signed-off-by: Stefan Metzmacher <metze@samba.org>
As described by the C standard, __func__ is a variable, not a macro.
Hence this #ifndef check does not work as intended, and only serves to
unconditionally disable __func__. A nonoperating __func__ prevents
cmocka operating correctly, so remove this definition.
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>
Change-Id: Ieac3937b9e86f39e84c0c056ffd649e44b292099
On systems where 'unsigned long' is 32-bits and the 'size'
parameter is set to 8 and the bytes are:
0x78 0x00 0x00 0x00 0x00 0x00 0x00 0x00
When 'i' becomes 4 'v' will be 0 again. As 'unsigned long' is only
able to hold 4 bytes.
Change the type of 'v' from 'unsigned long' to 'uint64_t' which
matches the type of the output parameter 'value'.
Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
CVE: CVE-2022-42898
Samba-BUG: https://bugzilla.samba.org/show_bug.cgi?id=15203
