389-ds-base

#49973 Ticket 49972 - use-after-free in case of several parallel krb authentication
Closed by spichugi. Opened by tbordaz.
tbordaz/389-ds-base ticket_49972  into  master

Download 49973.patch

Bug Description:
When several threads (RA) authenticates to the same host and at the same time
There is a good chance they will share the same credential cache.
If one authentication fails, the thread will clear the cache (krb5_cc_destroy)
although others threads may still use it.

Fix Description:
The best approach is to drop using krb5 function and use gssapi.
It is a quite intrusive change and a simplest temporary fix will serialize
all krb5 calls.

During initialization of the interaction structure (sasl), if using gssapi mechanism,
the calls to krb5 functions are serialized with a lock.
Then the lock is released for the authentication and cleanup.
Cleanup needs to be serialized as well as it calls krb5_cc_destroy.
The fix consist to acquire the lock over initialization/authentication/cleanup.
So only one RA can authenticate at the same time.

https://pagure.io/389-ds-base/issue/49972

Reviewed by: ?

Platforms tested: F27 & F28

Flag Day: no

Doc impact: no

rebased onto 3846f8a213bb9d403d3409ba059c6107f3fa17c2

rebased onto 3846f8a213bb9d403d3409ba059c6107f3fa17c2

While this fixes it, holy moly, this is going to wreck krb5 performance ....

You are right, it will serialize the krb5 authentication.
Alternatives were:
- do not call krb5_cc_destroy on authentication failure (invalid cred)
- rewrite the code to use gssapi.

Both options were looking risky (Had a concern regarding potential leak for the first and large change for the second). The performance drop is likely acceptable as it is quite rare that RAs want to auth at the same time and small delay to start replication session will not impact replication itself

Neither of these are good options :|

I think that given the trend of krb5 to be more and more unused by clients and consumers, I think that this is an acceptable change for now, but surely someone will eventually raise a complaint ...

I would prefer, and suggested as much in our IRC conversation, to not call krb5_cc_destroy. The usage of it in 389ds is not correct if other libraries (SASL, GSSAPI) have its contents open; it will race.

If we don't call the cc_destroy, then we'll leak memory. What's the correct behaviour here @rharwood ?

@rharwood, I do agree DS should not manipulate directly anything from krb5 and we should move to gssapi or sasl. Now the current fix is a simple hardening (with perf impact) as moving to gssapi/sasl looks larger/more_difficult fix.

DS is using sasl/gssapi for incoming connection and krb5 for outgoing. My understanding is that the ccache allocated for outgoing connection should not be used while authentication of incoming connections. Am I correct ?
As a consequence, I think the only case of others components using the same ccache is when several threads are doing krb5 on outgoing connection at the same time. The fix will prevent this.

The transient failure (invalid cred) looks very frequent (almost systematic) at least during the first connection attempts. If not calling cc_destroy triggers a leak, it may have an significant impact as the replication threads are tuned to retry authentication quite frequently (each 3s).

If we don't call the cc_destroy, then we'll leak memory. What's the correct behaviour here @rharwood ?

I don't know where this idea came from, but it's not right.

If it helps, think of ccaches as refcounted. Any call that opens one (krb5_cc_resolve, krb5_cc_default, etc.) increments the refcount; krb5_cc_close decrements it. When MEMORY ccaches have refcount 0, they're deleted. krb5_cc_destroy invalidates the cache; using it afterward is not permitted.

rebased onto b44ef4e272caa2a9bd513e9dfd7f2879a9fa35f8

After some additional tests the removal of cc_destroy is not work (see BZ comment 39). So the fix for that bug remains serialization of krb authentication of outgoing connections.
Anyone volunteer to review it ?

I would review this. Where can I find the changes?

Thanks @firstyear !
The changes are those attached to this PR. You already reviewed but did not ACK. Your concerns were performance impacts but somehow acceptable (https://pagure.io/389-ds-base/pull-request/49973#comment-66237).

Okay: Well if the patch hasn't change I'm okay to ack this, but the warning is still performance. I think this will hurt gssapi auth performance, but I also don't think that gssapi is widely deployed in pure 389 instances, so this is only going to affect IPA.

rebased onto ff9387bb97c306ad9c7d430bf58e094333c4f1b3

Pull-Request has been merged by tbordaz

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3032

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.