krb5_child: fix enterprise principal parsing in keep-alive sessions by ikerexxe · Pull Request #8351 · SSSD/sssd

ikerexxe · 2026-01-14T07:54:49Z

When keep-alive sessions transition between command types (e.g., from SSS_PAM_PREAUTH to SSS_PAM_AUTHENTICATE), enterprise principal settings were not being updated, causing parsing inconsistencies in complex AD environments.

This change ensures that when the backend sends updated enterprise principal settings for different command types, the principals are correctly re-parsed with the appropriate flags, fixing UPN handling in multi-domain AD environments.

gemini-code-assist

Code Review

This pull request aims to fix enterprise principal parsing in keep-alive sessions by re-parsing principals when settings are updated. The overall approach is correct, but the implementation introduces several critical use-after-free vulnerabilities. In multiple places, memory is freed but the corresponding pointers are not nullified. If a subsequent operation fails, these dangling pointers can be freed again in a later call, leading to a crash. I've provided specific comments and suggestions to fix these issues.

src/providers/krb5/krb5_child.c

src/util/sss_krb5.c

src/providers/krb5/krb5_child.c

aplopez

LGTM

ikerexxe · 2026-01-22T15:45:27Z

@aplopez do you mind taking another look?

aplopez · 2026-01-22T15:57:27Z

@aplopez do you mind taking another look?

Looking better now.

sumit-bose · 2026-01-23T13:28:07Z

/gemini review

gemini-code-assist

Code Review

The pull request effectively addresses the issue of enterprise principal parsing inconsistencies in keep-alive sessions. By ensuring that krb5_req settings, including use_enterprise_princ, validate, posix_domain, and send_pac, are correctly updated between commands, and by re-parsing the principal with appropriate flags when necessary, the changes fix UPN handling in complex AD environments. The introduction of a conditional update logic for kr->princ and proper management of kr->creds and kr->name contribute to the correctness and robustness of the Kerberos child process.

gemini-code-assist · 2026-01-23T13:31:20Z

src/providers/krb5/krb5_child.c

+    /* Update settings that may change between commands */
+    dest->use_enterprise_princ = src->use_enterprise_princ;
+    dest->validate = src->validate;
+    dest->posix_domain = src->posix_domain;
+    dest->send_pac = src->send_pac;


The update of use_enterprise_princ, validate, posix_domain, and send_pac in krb5_req_update is critical for ensuring that subsequent Kerberos operations in a keep-alive session use the correct configuration. Without this, the principal parsing logic in k5c_setup might operate with stale flags, leading to incorrect behavior, especially in dynamic AD environments.

gemini-code-assist · 2026-01-23T13:31:20Z

src/providers/krb5/krb5_child.c

+    kerr = sss_krb5_parse_name_flags(kr->ctx, kr->upn, parse_flags, &princ);
    if (kerr != 0) {
        KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
        return kerr;


The introduction of a temporary princ and the subsequent comparison with kr->princ before updating is a robust way to handle changes in the principal. This prevents unnecessary re-initialization if the principal remains the same and correctly updates it when needed, which is vital for the PR's objective.

gemini-code-assist · 2026-01-23T13:31:20Z

src/providers/krb5/krb5_child.c

+    ret = k5c_setup(kr, offline);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "k5c_setup failed during keep-alive [%d]: %s\n",
+              ret, sss_strerror(ret));
+        goto done;


Calling k5c_setup after krb5_req_update is essential. This ensures that any updated settings received from the backend (like use_enterprise_princ) are immediately applied to the krb5_req structure before further processing, which is crucial for correct principal parsing and credential handling.

gemini-code-assist · 2026-01-23T13:31:20Z

src/providers/krb5/krb5_child.c

+    if (kr->princ == NULL || !krb5_principal_compare(kr->ctx, kr->princ, princ)) {
+        DEBUG(SSSDBG_TRACE_FUNC, "Updating principal\n");
+        if (kr->princ != NULL) {
+            krb5_free_principal(kr->ctx, kr->princ);
+        }
+        kr->princ = princ;
+    } else {
+        DEBUG(SSSDBG_TRACE_FUNC, "Principal unchanged, keeping existing\n");
+        krb5_free_principal(kr->ctx, princ);
+    }


The change to free kr->princ only when it's different from the newly parsed princ (or if kr->princ was initially NULL) is a good memory management practice. It avoids potential memory leaks if the principal changes during a keep-alive session.

gemini-code-assist · 2026-01-23T13:31:20Z

src/providers/krb5/krb5_child.c

+    if (kr->princ_orig == NULL) {
+        kerr = krb5_parse_name(kr->ctx, kr->upn, &kr->princ_orig);
+        if (kerr != 0) {
+            KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
+            return kerr;
+        }
    }


Moving the krb5_parse_name call for kr->princ_orig inside an if (kr->princ_orig == NULL) block improves efficiency by preventing redundant parsing of the original principal if it has already been set.

gemini-code-assist · 2026-01-23T13:31:20Z

src/providers/krb5/krb5_child.c

+    sss_krb5_free_unparsed_name(kr->ctx, kr->name);
+    kr->name = NULL;


The explicit freeing of kr->name before unparsing a new one is good practice to prevent memory leaks and ensure kr->name always reflects the current kr->princ.

gemini-code-assist · 2026-01-23T13:31:20Z

src/providers/krb5/krb5_child.c

+    if (kr->creds != NULL) {
+        krb5_free_cred_contents(kr->ctx, kr->creds);
+    } else {
+        kr->creds = calloc(1, sizeof(krb5_creds));
+        if (kr->creds == NULL) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "calloc failed.\n");
+            return ENOMEM;
+        }
    }


The conditional freeing of kr->creds contents before potential reallocation ensures that the creds structure is always in a clean state, preventing memory leaks or data corruption if it was previously used.

sumit-bose

Hi,

thank you for the patch, ACK

bye,
Sumit

When keep-alive sessions transition between command types (e.g., from SSS_PAM_PREAUTH to SSS_PAM_AUTHENTICATE), enterprise principal settings were not being updated, causing parsing inconsistencies in complex AD environments. This change ensures that when the backend sends updated enterprise principal settings for different command types, the principals are correctly re-parsed with the appropriate flags, fixing UPN handling in multi-domain AD environments. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com> Reviewed-by: Alejandro López <allopez@redhat.com> Reviewed-by: Sumit Bose <sbose@redhat.com>

sssd-bot · 2026-01-23T19:01:24Z

The pull request was accepted by @sumit-bose with the following PR CI status:

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

sumit-bose · 2026-01-24T17:22:55Z

Hi,

sorry for being late, but I wonder if this shouldn't be backported to sssd-2-12 and cherry-pick into #8267 similar to the fix for #8331 ?

bye,
Sumit

ikerexxe · 2026-01-26T08:21:25Z

sorry for being late, but I wonder if this shouldn't be backported to sssd-2-12 and cherry-pick into #8267 similar to the fix for #8331 ?

Yes, this should go in sssd-2-12

gemini-code-assist bot reviewed Jan 14, 2026

View reviewed changes

src/providers/krb5/krb5_child.c Outdated Show resolved Hide resolved

src/providers/krb5/krb5_child.c Show resolved Hide resolved

src/util/sss_krb5.c Outdated Show resolved Hide resolved

ikerexxe force-pushed the fix-upn-parsing branch 2 times, most recently from 1e56bc9 to a06b02e Compare January 15, 2026 10:44

ikerexxe marked this pull request as ready for review January 15, 2026 13:12

ikerexxe added the Waiting for review label Jan 15, 2026

alexey-tikhonov requested review from aplopez and sumit-bose January 15, 2026 13:34

alexey-tikhonov added the no-backport This should go to target branch only. label Jan 15, 2026

aplopez reviewed Jan 21, 2026

View reviewed changes

src/providers/krb5/krb5_child.c Show resolved Hide resolved

ikerexxe force-pushed the fix-upn-parsing branch from a06b02e to 388d902 Compare January 22, 2026 09:29

aplopez reviewed Jan 22, 2026

View reviewed changes

src/providers/krb5/krb5_child.c Outdated Show resolved Hide resolved

aplopez approved these changes Jan 22, 2026

View reviewed changes

alexey-tikhonov assigned aplopez and sumit-bose Jan 22, 2026

ikerexxe force-pushed the fix-upn-parsing branch from 388d902 to d5799c5 Compare January 22, 2026 15:43

gemini-code-assist bot reviewed Jan 23, 2026

View reviewed changes

sumit-bose approved these changes Jan 23, 2026

View reviewed changes

sumit-bose added Accepted and removed Waiting for review labels Jan 23, 2026

sssd-bot force-pushed the fix-upn-parsing branch from d5799c5 to cc649a0 Compare January 23, 2026 19:01

alexey-tikhonov merged commit dd3cd95 into SSSD:master Jan 24, 2026
16 checks passed

alexey-tikhonov added backport-to-sssd-2-12 and removed no-backport This should go to target branch only. labels Jan 26, 2026

sssd-bot mentioned this pull request Jan 26, 2026

[autobackport: sssd-2-12] krb5_child: fix enterprise principal parsing in keep-alive sessions #8401

Merged

		sss_krb5_free_unparsed_name(kr->ctx, kr->name);
		kr->name = NULL;

Conversation

ikerexxe commented Jan 14, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aplopez left a comment

Choose a reason for hiding this comment

Uh oh!

ikerexxe commented Jan 22, 2026

Uh oh!

aplopez commented Jan 22, 2026

Uh oh!

sumit-bose commented Jan 23, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

sumit-bose left a comment

Choose a reason for hiding this comment

Uh oh!

sssd-bot commented Jan 23, 2026

Uh oh!

Uh oh!

sumit-bose commented Jan 24, 2026

Uh oh!

ikerexxe commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants