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).
Details
Details
- make buildworld WITHOUT_NTP=yes
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| crypto/heimdal/kdc/digest.c | ||
|---|---|---|
| 1322 ↗ | (On Diff #48326) | 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 ↗ | (On Diff #48326) | Bah, this probably needs to free 'output_message_buffer' |
| crypto/heimdal/lib/gssapi/krb5/get_mic.c | ||
| 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. |
| crypto/heimdal/lib/gssapi/krb5/unwrap.c | ||
| 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? |
| crypto/heimdal/lib/gssapi/krb5/verify_mic.c | ||
| 92 ↗ | (On Diff #48326) | Note that this GSS_S_BAD_MIC failure does clear deskey and schedule. |
| crypto/heimdal/lib/gssapi/krb5/wrap.c | ||
| 329 ↗ | (On Diff #48326) | Here I chose to reuse the previously-allocated context to reduce the amount of error handling. |
| crypto/heimdal/lib/hx509/crypto.c | ||
| 230 ↗ | (On Diff #48326) | BN_set_negative() assumes 'bn' is not NULL. |
| 2582 ↗ | (On Diff #48326) | This was already redundant in the existing code. |
| crypto/heimdal/lib/hx509/ks_p11.c | ||
| 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. |
| crypto/heimdal/lib/krb5/crypto-evp.c | ||
| 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. |
| crypto/heimdal/lib/ntlm/heimntlm-protos.h | ||
| 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. |
| kerberos5/Makefile.inc | ||
| 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. |
| kerberos5/include/crypto-headers.h | ||
| 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. |
| kerberos5/Makefile.inc | ||
|---|---|---|
| 11 ↗ | (On Diff #48326) | 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. |
Comment Actions
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
| kerberos5/Makefile.inc | ||
|---|---|---|
| 11 ↗ | (On Diff #48326) | Please try again. I have updated opensslconf.h for all architectures. |
Comment Actions
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 ↗ | (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. |
| crypto/heimdal/lib/gssapi/krb5/get_mic.c | ||
| 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. |
| crypto/heimdal/lib/gssapi/krb5/unwrap.c | ||
| 148 ↗ | (On Diff #48326) | This should clear them and the other was a bug, yes. |
| crypto/heimdal/lib/gssapi/krb5/verify_mic.c | ||
| 92 ↗ | (On Diff #48326) | It doesn't set *minor_status, though, |
| crypto/heimdal/lib/hx509/crypto.c | ||
| 1083 ↗ | (On Diff #48328) | I think this reassignment is not needed. |
| 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) | new_iqmp? |
| crypto/heimdal/lib/hx509/ks_p11.c | ||
| 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. |
| crypto/heimdal/lib/krb5/crypto-rand.c | ||
| 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. |
| crypto/heimdal/lib/krb5/pkinit.c | ||
| 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. |
| crypto/heimdal/lib/ntlm/heimntlm-protos.h | ||
| 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. |
| crypto/heimdal/lib/ntlm/ntlm.c | ||
| 1135 ↗ | (On Diff #48328) | Probably cleanest to also zero answer->length here. |
| 1185 ↗ | (On Diff #48328) | Probably cleanest to zero session->length. |
| 1783 ↗ | (On Diff #48328) | heim_ntlm_free_buf() will zero the length as well (for both ntlm and lm) |
| crypto/heimdal/lib/gssapi/krb5/unwrap.c | ||
|---|---|---|
| 148 ↗ | (On Diff #48326) | (But it's fixed now: https://github.com/heimdal/heimdal/pull/426#issuecomment-423759929) |
| crypto/heimdal/lib/hx509/ks_p11.c | ||
|---|---|---|
| 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? |
| crypto/heimdal/lib/krb5/pkinit.c | ||
| 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. |
| crypto/heimdal/lib/ntlm/ntlm.c | ||
| 1135 ↗ | (On Diff #48328) | I went ahead and used heim_ntlm_free_buf() since it is already used elsewhere in this file. |
| crypto/heimdal/lib/hx509/ks_p11.c | ||
|---|---|---|
| 664 ↗ | (On Diff #48328) | It's fine with EINVAL, so I'd leave it alone for now. |
| crypto/heimdal/lib/gssapi/krb5/unwrap.c | ||
|---|---|---|
| 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. |
| crypto/heimdal/lib/hx509/ks_file.c | ||
| 114 ↗ | (On Diff #48455) | I guess we could hx509_set_error_string() here if we wanted. |
| crypto/heimdal/lib/ntlm/ntlm.c | ||
| 1561 ↗ | (On Diff #48455) | This leaks data.data; need to krb5_data_free(&data) |
| 1695 ↗ | (On Diff #48455) | heim_ntlm_free_buf(infotarget) |
| crypto/heimdal/lib/gssapi/krb5/unwrap.c | ||
|---|---|---|
| 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? |
| crypto/heimdal/lib/ntlm/ntlm.c | ||
| 1695 ↗ | (On Diff #48455) | I chose to reuse the 'out' label similar to the error case a few lines above. |
| crypto/heimdal/lib/gssapi/krb5/unwrap.c | ||
|---|---|---|
| 116 ↗ | (On Diff #48455) | That's true, thanks. |