fix: fix TOCTOU race in wallpaper save function#1145
Open
xionglinlin wants to merge 1 commit into
Open
Conversation
1. Use unix.Open with O_NOFOLLOW to atomically reject symlinks, eliminating TOCTOU race condition 2. Reuse the file descriptor for stat, MD5 checksum calculation, and content reading 3. Compute MD5 from already-read content instead of re-opening the file 4. Replace runuser/cat shell command with direct file read via the file descriptor 5. Close file descriptor properly with defer Log: Fixed security vulnerability in wallpaper saving functionality Influence: 1. Test saving wallpaper with a regular file to verify functionality 2. Test with a symlink target to confirm symlinks are rejected 3. Verify MD5 checksum is computed correctly 4. Test file permission handling and error cases 5. Verify the fix works for both regular and solid color wallpapers 6. Test concurrent operations to ensure race condition is mitigated fix: 修复壁纸保存函数中的TOCTOU竞态漏洞 1. 使用unix.Open函数配合O_NOFOLLOW标志原子性地拒绝符号链接，消除TOCTOU竞 态条件 2. 复用文件描述符进行stat、MD5校验和计算及内容读取 3. 从已读取的内容计算MD5，避免重新打开文件 4. 使用文件描述符直接读取替换runuser/cat shell命令 5. 使用defer正确关闭文件描述符 Log: 修复壁纸保存功能的安全漏洞 Influence: 1. 使用常规文件测试保存壁纸功能，验证功能正常 2. 使用符号链接目标测试，确认符号链接被拒绝 3. 验证MD5校验和计算是否正确 4. 测试文件权限处理和错误情况 5. 验证修复对常规壁纸和纯色壁纸均有效 6. 测试并发操作以确保竞态条件已被解决 PMS: BUG-364751 Change-Id: I0ed2d3474f0a869e4f61731f53730f6103720785
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xionglinlin The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the wallpaper-saving path to safely open the source file using unix.Open with O_NOFOLLOW, reuse the resulting file descriptor for stat, content read, and MD5 hashing, and remove the runuser/cat shell pipeline, thereby fixing a TOCTOU race and tightening permissions handling. Sequence diagram for updated SaveCustomWallPaper file handlingsequenceDiagram
participant Daemon
participant SaveCustomWallPaper
participant unix
participant osFile
participant io
participant md5
participant os
Daemon->>SaveCustomWallPaper: SaveCustomWallPaper(sender, username, file)
SaveCustomWallPaper->>unix: Open(file, O_RDONLY|O_NOFOLLOW|O_CLOEXEC, 0)
unix-->>SaveCustomWallPaper: fd
SaveCustomWallPaper->>osFile: NewFile(uintptr(fd), file)
SaveCustomWallPaper->>osFile: Stat()
osFile-->>SaveCustomWallPaper: FileInfo
SaveCustomWallPaper->>io: ReadAll(osFile)
io-->>SaveCustomWallPaper: src
SaveCustomWallPaper->>md5: Sum(src)
md5-->>SaveCustomWallPaper: md5sum
SaveCustomWallPaper->>os: WriteFile(destFile, src, 0644)
os-->>SaveCustomWallPaper: err
SaveCustomWallPaper-->>Daemon: destFile / error
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review
<style>
.ai-report h1 { color: #2c3e50; border-bottom: 2px solid #2c3e50; padding-bottom: 10px; font-size: 24px; }
.ai-report h2 { color: #2980b9; margin-top: 25px; font-size: 20px; }
.ai-report h3 { color: #34495e; font-size: 16px; }
.ai-report .solution-box { background-color: #f8f9fa; padding: 15px; border-left: 4px solid #28a745; margin-top: 10px; }
.ai-report .info-needed { background-color: #fff3cd; padding: 15px; border-left: 4px solid #ffc107; margin-top: 10px; }
.ai-report .note { background-color: #e7f4ff; padding: 15px; border-left: 4px solid #17a2b8; margin-top: 10px; }
.ai-report code { background-color: #f1f2f3; padding: 2px 4px; border-radius: 3px; color: #d63384; font-family: monospace; }
</style>
问题分析报告问题分析在 dde-system-daemon 项目的 SaveCustomWallPaper 函数中存在严重的 TOCTOU 竞态条件 漏洞与符号链接跟随安全风险。原代码先通过 解决方案
补充说明在系统级守护进程开发中，应严格遵循“打开即校验”原则，避免将文件路径在不同系统调用或外部命令之间传递。对于需要权限控制的文件读取操作，应优先考虑基于已打开的文件描述符进行权限降级或校验，而非依赖不安全的衍生进程命令。 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- By replacing the
runuser/catcall with a direct read from the daemon process, the effective permissions check has changed; consider whether the daemon should still enforce thatusernamehas read access to the source file (e.g., via a dedicated helper or explicit ACL check) to avoid unintentionally broadening access to files. - The new approach reads the entire wallpaper file into memory to compute MD5 and then writes it out; if wallpapers can be large, consider switching to a streaming hash and copy (e.g., using
io.Copywithmd5.New) to avoid holding the whole file in memory at once.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By replacing the `runuser`/`cat` call with a direct read from the daemon process, the effective permissions check has changed; consider whether the daemon should still enforce that `username` has read access to the source file (e.g., via a dedicated helper or explicit ACL check) to avoid unintentionally broadening access to files.
- The new approach reads the entire wallpaper file into memory to compute MD5 and then writes it out; if wallpapers can be large, consider switching to a streaming hash and copy (e.g., using `io.Copy` with `md5.New`) to avoid holding the whole file in memory at once.
## Individual Comments
### Comment 1
<location path="bin/dde-system-daemon/wallpaper.go" line_range="225-228" />
<code_context>
}
- info, err := os.Stat(file)
+ // Use O_NOFOLLOW to atomically reject symlinks, eliminating TOCTOU race.
+ fd, err := unix.Open(file, unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
+ if err != nil {
+ logger.Warning(err)
+ return "", dbusutil.ToError(err)
+ }
+ f := os.NewFile(uintptr(fd), file)
</code_context>
<issue_to_address>
**🚨 issue (security):** Opening the file as the daemon user changes the security model compared to the previous `runuser`-based read.
With the prior `runuser` approach, the kernel enforced the target user's filesystem permissions and returned "permission denied" when appropriate. Now the daemon opens the file itself, likely with broader privileges, so per-user filesystem checks are no longer applied at this layer. Please confirm this change is deliberate and that higher-level authorization fully replaces the lost OS‑level permission check for this path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+225
to
+228
| fd, err := unix.Open(file, unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) | ||
| if err != nil { | ||
| logger.Warning(err) | ||
| return "", dbusutil.ToError(err) |
There was a problem hiding this comment.
🚨 issue (security): Opening the file as the daemon user changes the security model compared to the previous runuser-based read.
With the prior runuser approach, the kernel enforced the target user's filesystem permissions and returned "permission denied" when appropriate. Now the daemon opens the file itself, likely with broader privileges, so per-user filesystem checks are no longer applied at this layer. Please confirm this change is deliberate and that higher-level authorization fully replaces the lost OS‑level permission check for this path.
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
2 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.
Log: Fixed security vulnerability in wallpaper saving functionality
Influence:
fix: 修复壁纸保存函数中的TOCTOU竞态漏洞
Log: 修复壁纸保存功能的安全漏洞
Influence:
PMS: BUG-364751
Change-Id: I0ed2d3474f0a869e4f61731f53730f6103720785
Summary by Sourcery
Mitigate a TOCTOU race and improve security in the custom wallpaper saving path by hardening file handling and checksum computation.
Bug Fixes:
Enhancements: