Page MenuHomeFreeBSD

Update heimdal to build against OpenSSL 1.1.

Authored by jhb on Sep 21 2018, 6:53 PM.



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

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Sep 21 2018, 6:53 PM
jhb marked 15 inline comments as done.Sep 21 2018, 7:11 PM
jhb added inline comments.
1322 ↗(On Diff #48326)

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

659 ↗(On Diff #48326)

Bah, this probably needs to free 'output_message_buffer'

116 ↗(On Diff #48326)

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.

148 ↗(On Diff #48326)

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?

92 ↗(On Diff #48326)

Note that this GSS_S_BAD_MIC failure does clear deskey and schedule.

329 ↗(On Diff #48326)

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

230 ↗(On Diff #48326)

BN_set_negative() assumes 'bn' is not NULL.

2582 ↗(On Diff #48326)

This was already redundant in the existing code.

659 ↗(On Diff #48326)

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.

46 ↗(On Diff #48326)

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.

90 ↗(On Diff #48326)

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.

11 ↗(On Diff #48326)

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 ↗(On Diff #48326)

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 ↗(On Diff #48326)

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

21 ↗(On Diff #48326)

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.

jkim added a reviewer: bjk.Sep 21 2018, 7:44 PM
jhb marked an inline comment as done.Sep 21 2018, 8:49 PM
jhb added inline comments.
11 ↗(On Diff #48326)

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

jhb updated this revision to Diff 48328.Sep 21 2018, 8:52 PM

Fixed the various issues I noted earlier.

jhb added a comment.Sep 22 2018, 12:38 AM

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 jhb@TEST.FOO
sh /etc/rc.d/onestop kdc
jkim marked an inline comment as done.Sep 22 2018, 2:49 AM
jkim added inline comments.
11 ↗(On Diff #48326)

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

bjk requested changes to this revision.Sep 22 2018, 3:44 AM

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.

1322 ↗(On Diff #48326)

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.

105 ↗(On Diff #48328)

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.

148 ↗(On Diff #48326)

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

92 ↗(On Diff #48326)

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.)

1083 ↗(On Diff #48328)

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 ↗(On Diff #48328)

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

2612 ↗(On Diff #48328)

(This could just return directly)

3033 ↗(On Diff #48328)


223 ↗(On Diff #48328)

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

664 ↗(On Diff #48328)

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

66 ↗(On Diff #48328)

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

172 ↗(On Diff #48328)

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

90 ↗(On Diff #48326)

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.

1135 ↗(On Diff #48328)

Probably cleanest to also zero answer->length here.

1185 ↗(On Diff #48328)

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

1783 ↗(On Diff #48328)

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

This revision now requires changes to proceed.Sep 22 2018, 3:44 AM
bjk added inline comments.Sep 22 2018, 5:38 PM
148 ↗(On Diff #48326)
emaste added a reviewer: hrs.Sep 24 2018, 5:21 PM
jhb marked 14 inline comments as done.Sep 25 2018, 7:11 PM
jhb added inline comments.
223 ↗(On Diff #48328)

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 ↗(On Diff #48328)

Would you rather me change it to abort?

172 ↗(On Diff #48328)

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.

1135 ↗(On Diff #48328)

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

jhb updated this revision to Diff 48455.Sep 25 2018, 7:12 PM
jhb marked 2 inline comments as done.
  • Various fixes from bjk@
bjk added inline comments.Oct 2 2018, 2:43 AM
664 ↗(On Diff #48328)

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

bjk added inline comments.Oct 2 2018, 3:44 PM
109 ↗(On Diff #48455)

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.

114 ↗(On Diff #48455)

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

1561 ↗(On Diff #48455)

This leaks; need to krb5_data_free(&data)

1695 ↗(On Diff #48455)


jhb updated this revision to Diff 48655.Oct 2 2018, 7:05 PM
jhb marked 12 inline comments as done.
  • More bjk feedback.
jhb added inline comments.Oct 2 2018, 7:05 PM
116 ↗(On Diff #48455)

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

1695 ↗(On Diff #48455)

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

bjk accepted this revision.Oct 2 2018, 9:25 PM
bjk added inline comments.
116 ↗(On Diff #48455)

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.

This revision is now accepted and ready to land.Oct 2 2018, 9:25 PM
jhb updated this revision to Diff 48679.Oct 3 2018, 5:02 PM
  • Fix build on mips (32-bit and using GCC instead of clang)
This revision now requires review to proceed.Oct 3 2018, 5:03 PM
emaste accepted this revision.Oct 3 2018, 5:26 PM
This revision is now accepted and ready to land.Oct 3 2018, 5:26 PM
bjk accepted this revision.Oct 3 2018, 5:44 PM
jkim accepted this revision.Oct 3 2018, 6:05 PM
This revision was automatically updated to reflect the committed changes.