Page MenuHomeFreeBSD
Differential D43009

kerberos: Fix numerous segfaults when using weak crypto
ClosedPublic
Actions

Authored by cy on Dec 12 2023, 8:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Aug 30, 10:18 AM
Unknown Object (File)
Sun, Aug 30, 8:32 AM
Unknown Object (File)
Sun, Aug 30, 6:20 AM
Unknown Object (File)
Sun, Aug 30, 6:10 AM
Unknown Object (File)
Sun, Aug 30, 5:35 AM
Unknown Object (File)
Sun, Aug 30, 5:35 AM
Unknown Object (File)
Tue, Aug 25, 9:43 AM
Unknown Object (File)
Tue, Aug 25, 5:02 AM
View All Files
Subscribers
bdrewery

Details

Reviewers
emaste
markj
imp
sjg
khorben
Commits
rGcb350ba7bf7c: kerberos: Fix numerous segfaults when using weak crypto
Summary

Weak crypto is provided by the openssl legacy provider which is
not load by default. Load the legacy providers as needed.

When the legacy provider is loaded into the default context the default
provider will no longer be automatically loaded. Without the default
provider the various kerberos applicaions and functions will abort().

PR: 272835
MFC after: 3 days

Test Plan

Tested locally in a jail and currently running on all my systems.

Tested by the person who reported PR/272835.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

cy created this revision.Dec 12 2023, 8:54 PM
Herald added a subscriber: bdrewery. · View Herald TranscriptDec 12 2023, 8:54 PM
cy requested review of this revision.Dec 12 2023, 8:54 PM
emaste added a reviewer: khorben.Dec 12 2023, 10:02 PM

Looking now. For future patch updates would you generate with full context (e.g. git diff -U9999999 or git show -U9999999?

emaste added a comment.Dec 12 2023, 10:06 PM

curious about inconsistent fbsd_ossl_provider_load calls - i.e. return value checked / not checked / (void) cast

Overall looks like a reasonable approach

Makefile.inc1
2625

update comment, maybe just libroken dependencies: or so, such that it doesn't get out of date again

cy added a comment.Dec 12 2023, 11:07 PM
In D43009#980734, @emaste wrote:

Looking now. For future patch updates would you generate with full context (e.g. git diff -U9999999 or git show -U9999999?

Looks like git-format-patch has a -U argument. I'll try that.

cy updated this revision to Diff 131297.Dec 12 2023, 11:08 PM

Use git-format-patch -U.

cy marked an inline comment as done.Dec 12 2023, 11:15 PM

Comment adjusted.

cy updated this revision to Diff 131298.Dec 12 2023, 11:17 PM

Fixup comment.

khorben added inline comments.Dec 13 2023, 4:54 PM
kerberos5/lib/libroken/fbsd_ossl_provider_load.c
22–23

It might be good to return the exact value of errno as set by the call to atexit(3) in case of failures. According to its manual page and to its source code, ENOMEM is the only likely one, but atexit(3) from the libc is a weak symbol and might be overridden.

From what I can tell OSSL_PROVIDER_load(3) simply returns OK/FAIL and is not expected to set errno to any specific value, so EINVAL is probably as good as it gets.

cy marked an inline comment as done.Dec 13 2023, 7:42 PM
cy updated this revision to Diff 131364.Dec 13 2023, 7:49 PM

Implement latest review suggestion.

cy updated this revision to Diff 131680.Dec 21 2023, 4:31 PM

Implement Ed's suggestion to universally check return codes.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 11 2024, 1:32 PM
Closed by commit rGcb350ba7bf7c: kerberos: Fix numerous segfaults when using weak crypto (authored by cy). · Explain Why
This revision was automatically updated to reflect the committed changes.
cy added a commit: rGcb350ba7bf7c: kerberos: Fix numerous segfaults when using weak crypto.
cy added a reverting change: rG3091cdb11fb0: Revert "kerberos: Fix numerous segfaults when using weak crypto".Jan 12 2024, 7:43 AM
sjg added inline comments.Jan 13 2024, 5:19 PM
kerberos5/lib/libroken/Makefile
78

Rest of SRCS is nicely sorted (as it should be), either slot this into the right spot,
or if you want to call it out as an addition use SRCS+=

89–90

you can remove the extra line

Revision Contents
Changeset List

PathSize
5 lines
crypto/
heimdal/
lib/
kadm5/
4 lines
1 line
krb5/
2 lines
1 line
5 lines
roken/
1 line
kerberos5/
include/
4 lines
4 lines
lib/
libroken/
11 lines
40 lines
libexec/
kdc/
2 lines
share/
mk/
2 lines

Diff 131297

View Options

Makefile.inc1

Loading...
View Options

crypto/heimdal/lib/kadm5/create_s.c

Loading...
View Options

crypto/heimdal/lib/kadm5/kadm5_locl.h

Loading...
View Options

crypto/heimdal/lib/krb5/context.c

Loading...
View Options

crypto/heimdal/lib/krb5/crypto.c

Loading...
View Options

crypto/heimdal/lib/krb5/salt.c

Loading...
View Options

crypto/heimdal/lib/roken/version-script.map

Loading...
View Options

kerberos5/include/crypto-headers.h

Loading...
View Options

kerberos5/include/fbsd_ossl_provider.h

Loading...
View Options

kerberos5/lib/libroken/Makefile

Loading...
View Options

kerberos5/lib/libroken/fbsd_ossl_provider_load.c

Loading...
View Options

kerberos5/libexec/kdc/Makefile

Loading...
View Options

share/mk/src.libnames.mk

Loading...