fix: Prevent symlink following privilege escalation in nbde_server_tang module#240
Merged
Merged
Conversation
…ng module Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesSymlink protection and tests
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
library/nbde_server_tang.py (1)
313-318: ⚡ Quick winAdd test coverage for symlink attack prevention.
The symlink protection logic should be validated with dedicated tests to ensure the security fix is effective and prevent future regressions.
Consider adding tests that:
- Pre-stage a symlink pointing to a sensitive test file in the keydir
- Invoke the module's key deployment/rotation operations
- Assert that the sensitive file's ownership and permissions remain unchanged
- Verify that legitimate
.jwkfiles (non-symlinks) are processed correctlyThis would provide confidence that the symlink attack vector is properly mitigated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/nbde_server_tang.py` around lines 313 - 318, Add unit tests that validate the symlink protection around the file-processing loop: create a temporary keydir, place a sensitive target file outside the keydir and a symlink inside the keydir that points to that sensitive file, then invoke the module function that iterates files (the code path using os.path.islink(fname), os.lchown(fname, uid, gid) and os.chmod(fname, 0o400)) and assert the sensitive target's ownership and permissions are unchanged; also add a control test where a real non-symlink `.jwk` file in the keydir is processed and its ownership/permissions are updated as expected to ensure legitimate files are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@library/nbde_server_tang.py`:
- Around line 313-318: There's a TOCTOU race between os.path.islink(fname) and
os.chmod(fname) because chmod follows symlinks; fix by opening the file
atomically with O_NOFOLLOW and operating on the file descriptor: use
os.open(fname, flags | os.O_NOFOLLOW) (handle ELOOP/EINVAL by skipping), then
call os.fchown(fd, uid, gid) and os.fchmod(fd, 0o400) on that fd, finally close
the fd; keep the existing os.path.islink/os.lchown checks only as
defense-in-depth but perform the ownership and mode changes via os.open +
os.fchown + os.fchmod to eliminate the race.
- Around line 313-318: File nbde_server_tang.py fails flake8/black: fix long
lines (E501) and non-top imports (E402) and ensure formatting matches black.
Shorten lines around the reported locations by breaking long string literals,
chained calls, or long argument lists into multiple lines (respecting black
line-length defaults), move any imports currently inside functions or blocks up
to the module top (resolve E402) and remove/adjust any unused imports; for the
os.lchown/os.chmod block ensure lines are within length limits and keep the
symlink check logic unchanged, then run black and flake8 locally (or tox -e
black,flake8) to confirm all violations are resolved before committing.
---
Nitpick comments:
In `@library/nbde_server_tang.py`:
- Around line 313-318: Add unit tests that validate the symlink protection
around the file-processing loop: create a temporary keydir, place a sensitive
target file outside the keydir and a symlink inside the keydir that points to
that sensitive file, then invoke the module function that iterates files (the
code path using os.path.islink(fname), os.lchown(fname, uid, gid) and
os.chmod(fname, 0o400)) and assert the sensitive target's ownership and
permissions are unchanged; also add a control test where a real non-symlink
`.jwk` file in the keydir is processed and its ownership/permissions are updated
as expected to ensure legitimate files are handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d5b880d8-49c4-4b09-8e82-10d40b90b9ce
📒 Files selected for processing (1)
library/nbde_server_tang.py
Use O_NOFOLLOW with file descriptor operations (fchown/fchmod) to atomically prevent symlink following, eliminating the race condition between islink() check and chmod() operation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add comprehensive unit tests covering: - Normal file ownership changes with O_NOFOLLOW - Symlink detection and skipping via islink() check - TOCTOU race protection via O_NOFOLLOW - Error handling (OSError, IOError) - File descriptor cleanup on error - Integration test with real symlinks Tests verify both Python 2.7 and 3.x compatibility. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #240 +/- ##
=======================================
Coverage ? 22.95%
=======================================
Files ? 2
Lines ? 196
Branches ? 0
=======================================
Hits ? 45
Misses ? 151
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Remove shebang from unit test file (ansible-test requirement) - Remove unused 'call' import (pylint unused-import) - Replace insecure tempfile.mktemp with NamedTemporaryFile (ansible-bad-function) - Apply black formatting Fixes ansible-test pylint and shebang sanity checks. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Enable mock package installation for Python 2.7 test environment. Python 2.7 does not have unittest.mock in stdlib, requires external mock package. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_nbde_server_tang.py (1)
304-308: 💤 Low valueMove
import shutilto module top.Importing inside a method works but deviates from PEP 8 conventions which recommend placing all imports at the top of the file. This also avoids potential import overhead on each tearDown call.
♻️ Suggested change
Add at the top of the file with other imports:
import shutilThen simplify tearDown:
def tearDown(self): """Clean up test directory.""" - import shutil - shutil.rmtree(self.test_dir, ignore_errors=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_nbde_server_tang.py` around lines 304 - 308, The tearDown method currently imports shutil locally; move "import shutil" to the top-of-module imports and remove the in-method import so tearDown simply calls shutil.rmtree(self.test_dir, ignore_errors=True). Update references in the tearDown function (method name: tearDown) to use the module-level shutil import.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_nbde_server_tang.py`:
- Around line 304-308: The tearDown method currently imports shutil locally;
move "import shutil" to the top-of-module imports and remove the in-method
import so tearDown simply calls shutil.rmtree(self.test_dir,
ignore_errors=True). Update references in the tearDown function (method name:
tearDown) to use the module-level shutil import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c0c40b1-5668-4ea3-8eee-405d16346a64
📒 Files selected for processing (4)
library/nbde_server_tang.pypytest_extra_requirements.txttests/unit/__init__.pytests/unit/test_nbde_server_tang.py
✅ Files skipped from review due to trivial changes (2)
- pytest_extra_requirements.txt
- tests/unit/init.py
Move shutil import from tearDown method to top-level imports for consistency with Python best practices. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
[citest] |
richm
approved these changes
Jun 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enhancement: Added symlink protection to prevent privilege escalation vulnerability in the
set_file_ownership_and_perms()function.Reason: The module was using
os.chown()which follows symlinks. An unprivileged local user could pre-stage symlinks in tang keydir pointing to sensitive files. When the module runs as root, it would follow the symlinks and change ownership of arbitrary files to the tang user, enabling privilege escalation.Result: The module now:
os.lchown()instead ofos.chown()for defense in depth (lchown does not follow symlinks)This prevents the symlink-following attack vector while maintaining the intended functionality of setting ownership and permissions on legitimate tang key files.
Security Impact: Prevents local privilege escalation attack where unprivileged users could manipulate file ownership through symlink attacks.
Summary by CodeRabbit
Bug Fixes
.jwksymlink handling) and avoid following them, preventing unintended changes to link targets.Tests
.jwk/non-directory edge cases.Chores