fix: add FIFO validation to prevent arbitrary file writes#320
Merged
Conversation
|
Hi @xujin177. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideAdds FIFO type validation for file descriptors and paths used for interprocess communication to prevent arbitrary file writes via --notify-fifo and security loader helper arguments, logging and ignoring invalid FIFOs while preserving existing behavior for valid ones. Sequence diagram for FIFO validation in SecurityLoaderHelper::parseLoaderArgssequenceDiagram
actor LoaderProcess
participant SecurityLoaderHelper
participant Kernel
LoaderProcess->>SecurityLoaderHelper: parseLoaderArgs(argc, argv)
loop args
SecurityLoaderHelper-->>SecurityLoaderHelper: parse --fd1 / --fd2
alt fd1 provided
SecurityLoaderHelper->>Kernel: fstat(fd1, st)
alt S_ISFIFO(st.st_mode)
SecurityLoaderHelper-->>LoaderProcess: info.fd1 set to fd1
else not FIFO or fstat error
SecurityLoaderHelper-->>LoaderProcess: info.fd1 set to -1
end
else fd2 provided
SecurityLoaderHelper->>Kernel: fstat(fd2, st)
alt S_ISFIFO(st.st_mode)
SecurityLoaderHelper-->>LoaderProcess: info.fd2 set to fd2
else not FIFO or fstat error
SecurityLoaderHelper-->>LoaderProcess: info.fd2 set to -1
end
end
end
Sequence diagram for FIFO validation in FifoNotifier reportPid and notifysequenceDiagram
actor WrapperScript
participant FifoNotifier
participant Kernel
WrapperScript->>FifoNotifier: reportPid()
alt m_path is empty
FifoNotifier-->>WrapperScript: return
else m_path set via --notify-fifo
FifoNotifier->>Kernel: stat(m_path, st)
alt S_ISFIFO(st.st_mode)
FifoNotifier->>Kernel: open(m_path, O_WRONLY | O_NONBLOCK)
FifoNotifier-->>WrapperScript: write PID to FIFO
else not FIFO or stat error
FifoNotifier-->>WrapperScript: log warning and return
end
end
WrapperScript->>FifoNotifier: notify()
alt m_path is empty
FifoNotifier-->>WrapperScript: return
else m_path set via --notify-fifo
FifoNotifier->>Kernel: stat(m_path, st)
alt S_ISFIFO(st.st_mode)
FifoNotifier->>Kernel: open(m_path, O_WRONLY | O_NONBLOCK)
FifoNotifier-->>WrapperScript: write notification to FIFO
else not FIFO or stat error
FifoNotifier-->>WrapperScript: log warning and return
end
end
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 2 issues, and left some high level feedback:
- The FIFO type checks for
--fd1and--fd2are duplicated; consider extracting a small helper (e.g.,bool isFifoFd(int fd)) to reduce repetition and make the intent clearer. - Within
FifoNotifier, thestat/S_ISFIFOlogic is duplicated in bothreportPidandnotify; factoring this into a shared private method would simplify maintenance and keep behavior consistent. - The
stat+opensequence introduces a small TOCTOU window; if feasible, consider validating the FIFO property on an already-open descriptor (or immediately afteropen) to tighten the check, even if the practical risk is low here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The FIFO type checks for `--fd1` and `--fd2` are duplicated; consider extracting a small helper (e.g., `bool isFifoFd(int fd)`) to reduce repetition and make the intent clearer.
- Within `FifoNotifier`, the `stat`/`S_ISFIFO` logic is duplicated in both `reportPid` and `notify`; factoring this into a shared private method would simplify maintenance and keep behavior consistent.
- The `stat` + `open` sequence introduces a small TOCTOU window; if feasible, consider validating the FIFO property on an already-open descriptor (or immediately after `open`) to tighten the check, even if the practical risk is low here.
## Individual Comments
### Comment 1
<location path="src/dde-update/securityloaderhelper.cpp" line_range="68-76" />
<code_context>
}
info.fd1 = static_cast<int>(fd);
info.isLoadedByLoader = true;
+ // Verify the fd is actually a FIFO to prevent writing
+ // authorization data to arbitrary open files.
+ struct stat st;
+ if (fstat(info.fd1, &st) != 0 || !S_ISFIFO(st.st_mode)) {
+ qWarning() << "fd1 is not a FIFO, ignoring";
+ info.fd1 = -1;
+ continue;
+ }
} else if (strcmp(argv[i], "--fd2") == 0 && i + 1 < argc) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify behavior of isLoadedByLoader when all validated fds end up rejected
In the `fd1` branch, `info.isLoadedByLoader` is set before the FIFO validation. If `fstat` fails or the fd is not a FIFO, `info.fd1` is reset to `-1` and we `continue`, but `isLoadedByLoader` remains `true`. If both `fd1` and `fd2` are rejected, you can end up with `isLoadedByLoader == true` and both fds invalid. Please either set `isLoadedByLoader` only after successful FIFO validation, or make sure downstream code correctly handles the case where `isLoadedByLoader` is `true` but `fd1`/`fd2` are `-1`.
</issue_to_address>
### Comment 2
<location path="src/dde-update/main.cpp" line_range="90-98" />
<code_context>
if (m_path.isEmpty()) {
return;
}
+ // Verify the path is actually a FIFO before opening,
+ // preventing arbitrary file writes via --notify-fifo.
+ struct stat st;
+ if (stat(m_path.toUtf8().constData(), &st) != 0 || !S_ISFIFO(st.st_mode)) {
+ qCWarning(logUpdateModal) << "FifoNotifier: path is not a FIFO, cannot report PID";
+ return;
+ }
// Write our PID so the wrapper can track this exact process via kill -0.
int fd = open(m_path.toUtf8().constData(), O_WRONLY | O_NONBLOCK);
if (fd < 0) {
</code_context>
<issue_to_address>
**🚨 issue (security):** Address TOCTOU window between stat() and open() when validating FIFO paths
Calling `stat(m_path...)` before `open(m_path...)` creates a TOCTOU race: the path can be changed between the check and the open, defeating the protection against arbitrary file writes via `--notify-fifo`. Instead, `open()` first with `O_NONBLOCK`, then `fstat()` the fd to verify it is a FIFO, and close it if the check fails. Apply the same pattern in `notify()`, which currently has the same issue.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
xujin177
force-pushed
the
bugfix/PMS-370161
branch
from
July 23, 2026 06:20
dce91fe to
9ae50ec
Compare
Added `sys/stat.h` include and FIFO type verification for file descriptors and paths used in FIFO-based interprocess communication. The fix ensures only valid FIFO files are opened for writing, preventing security vulnerabilities where arbitrary file paths could be used to write to unintended locations. This addresses potential issues with `--notify-fifo` and `--fd1`/`--fd2` arguments where the provided path or file descriptor might not be a FIFO, mitigating the risk of unauthorized file writes. Influence: 1. Test FIFO notification system with valid FIFO paths 2. Verify FIFO validation with regular files instead of FIFOs 3. Test with non-existent paths to ensure proper error handling 4. Verify security loader helper arguments (`--fd1`, `--fd2`) with valid FIFOs 5. Test with invalid file types (sockets, regular files) passed as FIFO arguments 6. Verify error logging and graceful handling when invalid paths/fds are detected fix: 添加FIFO验证以防止任意文件写入 添加了`sys/stat.h`头文件，并对FIFO通信中使用的文件描述符和路径进行了FIFO 类型验证。此修复确保只打开有效的FIFO文件进行写入，防止因提供任意文件路径 而写入未预期位置的潜在安全漏洞。解决了`--notify-fifo`和`--fd1`/`--fd2`参 数可能指向非FIFO文件的问题，降低了未经授权文件写入的风险。 Influence: 1. 使用有效的FIFO路径测试FIFO通知系统 2. 使用普通文件替代FIFO验证FIFO类型检查 3. 测试不存在的路径以确保正确处理错误 4. 使用有效FIFO验证安全加载器助手参数(`--fd1`, `--fd2`) 5. 测试将无效文件类型（套接字、普通文件）作为FIFO参数传递的情况 6. 验证检测到无效路径/文件描述符时的错误记录和优雅处理 PMS: TASK-393165
xujin177
force-pushed
the
bugfix/PMS-370161
branch
from
July 23, 2026 06:23
9ae50ec to
c1e6bb6
Compare
waterlovemelon
approved these changes
Jul 23, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: waterlovemelon, xujin177 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 |
Contributor
Author
|
/merge |
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.
Added
sys/stat.hinclude and FIFO type verification for file descriptors and paths used in FIFO-based interprocess communication. The fix ensures only valid FIFO files are opened for writing, preventing security vulnerabilities where arbitrary file paths could be used to write to unintended locations. This addresses potential issues with--notify-fifoand--fd1/--fd2arguments where the provided path or file descriptor might not be a FIFO, mitigating the risk of unauthorized file writes.Influence:
--fd1,--fd2) with valid FIFOsfix: 添加FIFO验证以防止任意文件写入
添加了
sys/stat.h头文件，并对FIFO通信中使用的文件描述符和路径进行了FIFO类型验证。此修复确保只打开有效的FIFO文件进行写入，防止因提供任意文件路径
而写入未预期位置的潜在安全漏洞。解决了
--notify-fifo和--fd1/--fd2参 数可能指向非FIFO文件的问题，降低了未经授权文件写入的风险。Influence:
--fd1,--fd2)PMS: TASK-393165
Summary by Sourcery
Add FIFO type validation for security loader helper file descriptors and notify FIFO paths to prevent arbitrary file writes.
Bug Fixes:
Enhancements: