Update heimdal to build against OpenSSL 1.1.

[jhb]

Summary

This is only compile tested. I haven't run it at all (and don't use
Kerberos myself, so not even sure how to test it).

Test Plan

• make buildworld WITHOUT_NTP=yes

[jhb]

crypto/heimdal/kdc/digest.c

1322

Ironically, heimdal doesn't check for malloc failures for other dynamic libcrypto allocations here and in several other places.

crypto/heimdal/lib/gssapi/krb5/arcfour.c

659

Bah, this probably needs to free 'output_message_buffer'

crypto/heimdal/lib/gssapi/krb5/get_mic.c

127

This needs to drop the ctx_id_mutex and probably needs to memset 'deskey' and 'schedule' on failure. Some other places I moved the malloc up above the mutex, so I should probably do that here as well.

crypto/heimdal/lib/gssapi/krb5/unwrap.c

151

I'm not sure if this should memset deskey and schedule. The error return on line 143 for GSS_S_BAD_MIC doesn't clear them, but perhaps that is a bug?

crypto/heimdal/lib/gssapi/krb5/verify_mic.c

92

Note that this GSS_S_BAD_MIC failure does clear deskey and schedule.

crypto/heimdal/lib/gssapi/krb5/wrap.c

329–331

Here I chose to reuse the previously-allocated context to reduce the amount of error handling.

crypto/heimdal/lib/hx509/crypto.c

230

BN_set_negative() assumes 'bn' is not NULL.

2582

This was already redundant in the existing code.

crypto/heimdal/lib/hx509/ks_p11.c

659

The old code just didn't check if 'n' and 'e' were NULL per the comment, but it doesn't seem like RSA can work without them. However, given the old code failing with an error rather than calling _hx509_abort() seemed more reasonable.

crypto/heimdal/lib/krb5/crypto-evp.c

46

This function returns void so there wasn't an easy way to handle failure here more gracefully. Part of the issue is that the old code assumed that mallocing the 'key' object was the only thing that could fail, but now we can't assume a static size for the EVP contexts, so we end up with a struct containing pointers to two EVP contexts.

crypto/heimdal/lib/ntlm/heimntlm-protos.h

90

I changed this function to return an int to avoid just calling abort() if malloc failed. I'm not sure how public of an API this is such that changing it to an int is ok or if we need to resort to abort() instead. This is the only function whose API I changed in this fashion.

kerberos5/Makefile.inc

11

Having warnings enabled was really helpful, and I'm surprised we currently don't have them enabled at all for kerberos5. I haven't yet tested a build with GCC to see if the CWARNFLAGS need to be pan-compiler rather than just for clang.

12

Several places in kerberos use abs() on time_t values and clang warns about passing a long when time_t is 64-bits to abs() and wants to use labs() instead. I chose not to fix those and just suppress the warnings for now to avoid noise in the diff.

13

heimdal has krb5_get_err_text() marked as deprecated, but all of the tools use it still.

kerberos5/include/crypto-headers.h

21

This appears to be a stub for older versions of OpenSSL? In 1.1 these are functions rather than macros, so the #ifndef didn't work properly to detect the existing version.

[jhb]

kerberos5/Makefile.inc

11

I tried to build with TARGET_ARCH=mips to test a GCC build but that failed because only the x86 openssl-conf.h.in has been updated.

[jhb]

Fixed the various issues I noted earlier.

[jhb]

I did some very simple testing which seemed to work:

make installworld DESTDIR=/ufs/krb
make distribution DESTDIR=/ufs/krb
mount -t devfs foo /ufs/krb/dev
chroot /ufs/krb
kadmin -r TEST.FOO -l init TEST.FOO
kadmin -r TEST.FOO -l add jhb
vi /etc/krb5.conf
sh /etc/rc.d/onestart kdc
kinit [e-mail]
sh /etc/rc.d/onestop kdc

[jkim]

kerberos5/Makefile.inc

11

Please try again. I have updated opensslconf.h for all architectures.

[bjk]

I think there are several places where the error path needs to zero the length field in an output structure as well as freeing the pointer within.
Usually there is a helper function to do this in a concise fashion, though the prevailing style of any given file is not always to use it.
Also some stylistic comments that do not necessarily require action.

crypto/heimdal/kdc/digest.c

1322

Yup, I have several things that segfault when my credentials expire. They tend to behave better (though still not great) when linked against MIT Kerberos.

crypto/heimdal/lib/gssapi/krb5/get_mic.c

105

This is basically inlining _gsskrb5_release_buffer(), but the present path of not introducing a call to that function from a file with no existing references seems like a fine plan.

crypto/heimdal/lib/gssapi/krb5/unwrap.c

151

This should clear them and the other was a bug, yes.
Looks like it even is still a bug in upstream's master.

crypto/heimdal/lib/gssapi/krb5/verify_mic.c

92

It doesn't set *minor_status, though,
(minor status values are basically implementation defined, though I think there's a decent case that upstream ought to be assigning some value to this field.)

crypto/heimdal/lib/hx509/crypto.c

1083

I think this reassignment is not needed.
(DSA_set0_key() is not actually going to fail here, either, though that doesn't mean we should skip the error handling.)

1124

Also no need to clear ret here, as it will get reassigned shortly.

2612

(This could just return directly)

3033

new_iqmp?

crypto/heimdal/lib/hx509/ks_p11.c

223

This kind of not-quite locking might be unsafe on something like CHERI, but IIUC it is okay for all our platforms.

664

Calling _hx509_abort() would have been fine here, yeah.

crypto/heimdal/lib/krb5/crypto-rand.c

66

As something of a side note, it was pretty impressive how much stuff broke when I disabled EGD support in the default openssl configuration.

crypto/heimdal/lib/krb5/pkinit.c

172

This error handling might be cleaner to make all three assignments once and do a single check, though this certainly works as-is.

crypto/heimdal/lib/ntlm/heimntlm-protos.h

90

it's unclear why we even install this generated file, but we do. I am not sure how much precedent we have for changing prototypes from void to int return value.

crypto/heimdal/lib/ntlm/ntlm.c

1135

Probably cleanest to also zero answer->length here.

1185

Probably cleanest to zero session->length.
I guess there's heim_ntlm_free_buf()?

1783

heim_ntlm_free_buf() will zero the length as well (for both ntlm and lm)

[bjk]

crypto/heimdal/lib/gssapi/krb5/unwrap.c

151

(But it's fixed now: https://github.com/heimdal/heimdal/pull/426#issuecomment-423759929)

[jhb]

crypto/heimdal/lib/hx509/ks_p11.c

223

Even on CHERI it is ok as loads and stores of pointers are still atomic and that's all you need for this to work.

664

Would you rather me change it to abort?

crypto/heimdal/lib/krb5/pkinit.c

172

Yes, other places do that and I generally chose to match the existing style of whatever the current code used (either one check at the end or individual checks). It probably would be cleaner as you suggest though.

crypto/heimdal/lib/ntlm/ntlm.c

1135

I went ahead and used heim_ntlm_free_buf() since it is already used elsewhere in this file.

[jhb]

• Various fixes from bjk@

[bjk]

crypto/heimdal/lib/hx509/ks_p11.c

664

It's fine with EINVAL, so I'd leave it alone for now.

[bjk]

crypto/heimdal/lib/gssapi/krb5/unwrap.c

109

One could perhaps argue that we should zero out &deskey before returning, though there's already a path a few lines later that returns without clearing it.

crypto/heimdal/lib/hx509/ks_file.c

114

I guess we could hx509_set_error_string() here if we wanted.
Also it looks like we leak clear.data.

crypto/heimdal/lib/ntlm/ntlm.c

1563

This leaks data.data; need to krb5_data_free(&data)

1697

heim_ntlm_free_buf(infotarget)

[jhb]

• More bjk feedback.

[jhb]

crypto/heimdal/lib/gssapi/krb5/unwrap.c

116

I think this should actually be clearing 'deskey' and not schedule in which case that fixes the issue you noted about a missing clear?

crypto/heimdal/lib/ntlm/ntlm.c

1697

I chose to reuse the 'out' label similar to the error case a few lines above.

[bjk]

crypto/heimdal/lib/gssapi/krb5/unwrap.c

116

That's true, thanks.
Apparently the other case I was looking at was different between upstream's master (what I had open locally) and our code here, so the FreeBSD version doesn't have the bad codepath.

[jhb]

• Fix build on mips (32-bit and using GCC instead of clang)