fix: fix TOCTOU race in wallpaper save function#1145
Merged
Conversation
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
|
![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.
wubw0508
approved these changes
Jun 15, 2026
xionglinlin
force-pushed
the
master
branch
2 times, most recently
from
9d1602b to
3ec4a8f
Compare
wubw0508
approved these changes
Jun 16, 2026
deepin pr auto review★ 总体评分：100分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 在 readWallpaperSourceAsUser 函数入口处增加对空路径的防御性检查，
// 避免因 D-Bus 接口传入空字符串而创建无意义的子进程，进一步提升健壮性。
func readWallpaperSourceAsUser(username string, file string) ([]byte, error) {
if file == "" {
return nil, fmt.Errorf("file path cannot be empty")
}
cmd := exec.Command("runuser", "-u", username, "--", wallpaperHelperPath, file)
var stderr bytes.Buffer
cmd.Stderr = &stderr
src, err := cmd.Output()
if err != nil {
return nil, fmt.Errorf("permission denied, %s is not allowed to read this file:%s: %s", username, file, strings.TrimSpace(stderr.String()))
}
return src, nil
} |
1. Add dedicated helper binary (dde-wallpaper-helper) to read files safely
with O_NOFOLLOW, eliminating TOCTOU race condition
2. Execute helper via runuser to drop privileges, so kernel enforces the
target user's file permissions instead of root bypassing them
3. All file operations (Open/Stat/Read) use the same file descriptor in
the helper, leaving zero TOCTOU window
4. Compute MD5 from already-read content instead of re-opening the file
5. Remove unused maxSize constant and dead checkFileReadPermission code
Log: Fixed security vulnerability in wallpaper saving functionality.
The original TOCTOU fix introduced a privilege-escalation regression
(root reading arbitrary user files). This is now resolved by a
dedicated helper binary that runs under the target user's identity.
Influence:
1. Test saving wallpaper with a regular file to verify functionality
2. Test with a symlink target to confirm symlinks are rejected
3. Test with a file not readable by the target user to confirm access denied
4. Verify MD5 checksum is computed correctly
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. 新增专用 helper 二进制 (dde-wallpaper-helper)，使用 O_NOFOLLOW 安全读取
文件，消除 TOCTOU 竞态条件
2. 通过 runuser 以目标用户身份执行 helper，由内核强制文件权限检查，解决
root 越权读取用户文件的问题
3. helper 内所有文件操作 (Open/Stat/Read) 基于同一文件描述符，TOCTOU 窗口为零
4. 从已读取的内存数据计算 MD5，避免重复打开文件
5. 移除不再使用的 maxSize 常量和 checkFileReadPermission 死代码
Log: 修复壁纸保存功能的安全漏洞。
原 TOCTOU 修复引入了 root 越权读用户文件的权限回退问题，
现通过专用 helper 二进制以目标用户身份运行来解决。
Influence:
1. 使用常规文件测试保存壁纸功能，验证功能正常
2. 使用符号链接目标测试，确认符号链接被拒绝
3. 使用目标用户无读权限的文件测试，确认返回权限拒绝
4. 验证 MD5 校验和计算是否正确
5. 验证修复对常规壁纸和纯色壁纸均有效
6. 测试并发操作以确保竞态条件已被解决
PMS: BUG-364751
Change-Id: I0ed2d3474f0a869e4f61731f53730f6103720785
wubw0508
approved these changes
Jun 16, 2026
fly602
approved these changes
Jun 16, 2026
mhduiy
approved these changes
Jun 16, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602, mhduiy, wubw0508, 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 |
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
5 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: