gpo: reject path traversal in gPCFileSysPath

[alexey-tikhonov]

The gPCFileSysPath LDAP attribute from AD Group Policy Objects is parsed by ad_gpo_extract_smb_components() which converts backslashes to forward slashes but does not reject ".." path traversal sequences. The resulting smb_path is used directly in gpo_cache_store_file() to construct a local filesystem path under GPO_CACHE_PATH, allowing an attacker with GPO write access to write files outside the cache directory.

Due to differential path resolution between libsmbclient (which clamps ".." at the SMB share root) and the kernel (which resolves ".." fully), the SMB download succeeds while the local file write escapes the cache. On systems with SELinux enforcing, this enables Kerberos configuration injection via /var/lib/sss/pubconf/krb5.include.d/ (sssd_public_t, writable by sssd_t). On systems without SELinux, this enables arbitrary file writes including cron job injection for root code execution.

This patch adds two layers of defense:

1. Reject ".." as a path component in smb_path at parse time in ad_gpo_extract_smb_components(). Uses component-aware validation that checks for "/..", "../", and exact ".." — not substring matching which would false-positive on legitimate names containing "..".

2. Validate the resolved cache path stays within GPO_CACHE_PATH in gpo_cache_store_file() using realpath(), with a trailing-slash prefix check to prevent prefix-collision attacks (e.g., /var/lib/sss/gpo_cache_evil/ matching /var/lib/sss/gpo_cache).

Based on the patch by: Ian Murphy imurphy@redhat.com
Amended by: Alexey Tikhonov atikhono@redhat.com

Fixes: CVE-2026-14476

[gemini-code-assist]

Code Review

This pull request introduces path traversal checks to prevent GPO cache directory escape by adding a parser-level check and a defense-in-depth check using realpath in gpo_cache_store_file. However, a critical security issue was identified in the defense-in-depth implementation: because realpath is called on the file path itself, it will return NULL for new files that do not exist yet, completely bypassing the validation. To resolve this, the parent directory of the file should be resolved and validated instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[sumit-bose]

Hi,

this patch matches the version I already reviewed privately, ACK.

bye,
Sumit

[sssd-bot]

Review done using Claude Code / Opus 4.6 (1M context)

Functional Issues

1. Defense-in-depth realpath() check is inoperative for the primary attack vector (src/providers/ad/ad_gpo_child.c:325-371)

   The comment on line 325 claims this block catches "any bypass of the '..' check in the parser, including encoding tricks, symlink attacks, or future regressions." In practice, realpath(filename, NULL) returns NULL for non-existent files, so the entire validation block is skipped for new file creation — which is the primary attack scenario described in the CVE (cron job injection, Kerberos config injection via new files).

   The maintainer has clarified this is intentionally scoped to existing-file substitution (symlink-on-existing-file attacks). If that narrower scope is the intent, the comment should be updated to reflect it — the current text overstates the check's coverage and could give false confidence during future security audits.

   A stronger alternative: since prepare_gpo_cache() has already created the parent directory by line 310, resolving the parent directory instead of the file itself (i.e., realpath(dirname(filename))) would make this defense-in-depth effective for both new and existing files, catching directory-level symlink attacks and parser regressions alike — without changing the function's semantics.

2. Directory creation side effects precede the defense-in-depth check (src/providers/ad/ad_gpo_child.c:310-334)

   prepare_gpo_cache() on line 310 creates the full directory hierarchy under GPO_CACHE_PATH before the realpath() validation on line 334. The mkdir calls in prepare_gpo_cache() use kernel path resolution, so if a future parser regression allowed .. components through, directories would be created outside the cache as a side effect — and the realpath() check would not catch it (since the file is new, realpath() returns NULL).

   This means neither defense layer would prevent directory creation outside the cache in a parser-bypass scenario. Consider adding a component-level .. rejection to prepare_gpo_cache() itself, or validating the resolved directory path after creation.

Nits & Non-Functional Issues

1. Missing :relnote: tag in commit message: The project uses :relnote: trailers for user-visible changes (see e.g. the recent "build: require OpenSSL >= 3.0.8" commit). A CVE fix is user-visible and should have a release note entry.

2. Variable declarations mid-block (src/providers/ad/ad_gpo_child.c:349-350): size_t cache_len and bool inside are declared after executable code within the if (resolved != NULL) block. The SSSD coding style guide says "Always declare variables at the top of the function or a block." These should be moved to the top of the if (resolved != NULL) block (after line 335).

3. No test coverage for gpo_path_has_traversal() (src/providers/ad/ad_gpo.c:3848): This is a security-critical function with several boundary conditions (exact .., leading ../, embedded /../, trailing /.., non-matching substrings like my..file). A unit test exercising these cases would guard against regressions during future modifications or backports.

4. Missing Assisted-By trailer: The commit message says "Based on the patch by: Ian Murphy" but the project's AI contribution policy requires an Assisted-By: trailer when AI tools are used. If AI tooling was involved in developing or reviewing this patch, the trailer should be added per the coding style guide.

Review of Existing Review Comments

• gemini-code-assist[bot] flagged that realpath() is bypassed for new files and suggested resolving the parent directory instead. This is a valid concern — see functional issue listed above. The maintainer responded that new files are "out of scope of this specific check" and that its purpose is to prevent substitution of existing files. This is a defensible design choice, but the comment on lines 325-331 should then be narrowed to match this stated intent, since it currently claims broader coverage ("encoding tricks... future regressions") that the check cannot deliver for new file creation.

• sumit-bose approved, noting the patch matches a privately reviewed version. No issues to address.

[alexey-tikhonov]

new files are "out of scope of this specific check" and that its purpose is to prevent substitution of existing files. This is a defensible design choice, but the comment on lines 325-331 should then be narrowed to match this stated intent

Done.

Directory creation side effects precede the defense-in-depth check

Ignoring.

Missing :relnote: tag in commit message

Fixed.

Variable declarations mid-block

To minimize changes to the original patch.

[thalman]

LGTM, ACK

[sssd-bot]

The pull request was accepted by @thalman with the following PR CI status:

---

🟢 CodeQL (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-44-x86_64:upstream (success)
🔴 rpm-build:fedora-rawhide-x86_64:upstream (failure)
🟢 testing-farm:centos-stream-10-x86_64:upstream (success)
🟢 testing-farm:fedora-43-x86_64:upstream (success)
🟢 testing-farm:fedora-44-x86_64:upstream (success)
🔴 testing-farm:fedora-rawhide-x86_64:upstream (failure)
🔴 Analyze (target) / cppcheck (failure)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🔴 ci / intgcheck (fedora-45) (failure)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-43) (success)
🔴 ci / system (fedora-44) (failure)
🔴 ci / system (fedora-45) (failure)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

---

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