Repository navigation
Conversation
Log: replace system() call with fork+execl in analyse.c ipwatchd Influence: fix CWE-78 OS command injection vulnerability in conflict notification script execution
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602 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 GuideThe conflict-notification script is now launched with fork()+execl() and its arguments passed directly, preventing shell metacharacters in device, IP, or MAC values from being interpreted as commands while preserving synchronous execution. Sequence diagram for safe conflict-notification script executionsequenceDiagram
participant Analyse as ipwd_analyse
participant Parent as Parent process
participant Child as Child process
participant Script as config.script
Analyse->>Parent: fork()
alt fork failed
Parent->>Analyse: ipwd_message(... fork failed)
else child process
Parent->>Child: execl(config.script, device, ip, rcv_smac)
Child->>Script: Execute directly without shell
alt execl failed
Child->>Child: _exit(127)
end
else parent process
Parent->>Parent: waitpid(pid, &status, 0)
alt waitpid failed
Parent->>Analyse: ipwd_message(... config.script)
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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/plugin-sdbus/plugin-ipwatchd/upstream/analyse.c" line_range="216" />
<code_context>
+ {
+ /* Parent process - wait for child */
+ int status;
+ rv = waitpid (pid, &status, 0);
+ if (rv == -1)
+ {
</code_context>
<issue_to_address>
**Scripts outlive daemon shutdown**
When a caught signal, especially SIGTERM, interrupts `waitpid` while a notification script runs, `waitpid` returns `-1` with `EINTR`, and the callback in `ipwd_analyse` returns without retrying or reaping the child. The daemon can exit while the script is still running, or leave an unreaped child that becomes a zombie.
Retry `waitpid` when it fails with `EINTR` until the child is reaped.
</issue_to_address>| { | ||
| /* Parent process - wait for child */ | ||
| int status; | ||
| rv = waitpid (pid, &status, 0); |
There was a problem hiding this comment.
🟡 Medium · Scripts outlive daemon shutdown
When a caught signal, especially SIGTERM, interrupts waitpid while a notification script runs, waitpid returns -1 with EINTR, and the callback in ipwd_analyse returns without retrying or reaping the child. The daemon can exit while the script is still running, or leave an unreaped child that becomes a zombie.
Retry waitpid when it fails with EINTR until the child is reaped.
Prompt for AI agents
In `src/plugin-sdbus/plugin-ipwatchd/upstream/analyse.c` at line 216:
**Scripts outlive daemon shutdown**
When a caught signal, especially SIGTERM, interrupts `waitpid` while a notification script runs, `waitpid` returns `-1` with `EINTR`, and the callback in `ipwd_analyse` returns without retrying or reaping the child. The daemon can exit while the script is still running, or leave an unreaped child that becomes a zombie.
Retry `waitpid` when it fails with `EINTR` until the child is reaped.
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 语法正确，逻辑清晰 ✅ 通过 潜在问题： 建议： ['删除第 43-44 行已无用的 command 与 command_len 声明，消除编译器警告', "可选：父进程 waitpid 成功后通过 WIFEXITED/WEXITSTATUS 检查子脚本退出码，以区分'脚本执行失败'与'fork/waitpid 失败'，提升可诊断性（原 system() 同样未检查退出码，故非回归）"] 2. 代码质量 ✅评价： 代码结构清晰，注释完整 ✅ 通过 潜在问题： 建议： ['fork 失败分支仅打印日志后继续循环（原 malloc 失败为 break），行为略有差异但可接受；如需保持一致可考虑 break'] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： [] 4. 代码安全 🔒评价： 存在0个安全漏洞 ✅ 通过
安全漏洞详情： 建议： ['建议确认 ipwatchd.h 已包含 <unistd.h>、<sys/wait.h>、<sys/types.h> 以提供 fork/execl/waitpid/pid_t 声明（当前未见显式 include，依赖头文件传递）'] 💡 改进建议代码示例/* 清理无用声明后的推荐写法（仅展示关键片段） */
void ipwd_analyse(u_char *args, const struct pcap_pkthdr *header, const u_char *packet)
{
int rv = 0;
int i = 0;
/* ... 删除 char *command 与 int command_len 声明 ... */
if (config.script != NULL)
{
/* Run user-defined script: script "dev" "ip" "mac" */
pid_t pid = fork();
if (pid == -1)
{
ipwd_message(IPWD_MSG_TYPE_ERROR, "Unable to execute user-defined script - fork failed");
}
else if (pid == 0)
{
/* Child process */
execl(config.script, config.script,
devices.dev[i].device, devices.dev[i].ip, rcv_smac,
(char *)NULL);
_exit(127); /* execl only returns on failure */
}
else
{
/* Parent process - wait for child */
int status;
rv = waitpid(pid, &status, 0);
if (rv == -1)
{
ipwd_message(IPWD_MSG_TYPE_ERROR,
"Unable to execute user-defined script: %s", config.script);
}
else if (WIFEXITED(status) && WEXITSTATUS(status) != 0)
{
ipwd_message(IPWD_MSG_TYPE_ERROR,
"User-defined script exited with code %d", WEXITSTATUS(status));
}
}
}
/* ... */
}本报告由 AI 代码审查工具自动生成 |
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
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.
修复内容
CWE-78 OS命令注入
将
analyse.c中system()调用替换为fork()+execl()，避免通过 shell 执行拼接的命令字符串。原代码
修复后
参数直接传递给
execl，不经过 shell 解释，消除命令注入风险。Log: replace system() call with fork+execl in analyse.c ipwatchd
Influence: fix CWE-78 OS command injection vulnerability in conflict notification script execution
Summary by Sourcery
Harden user-defined script execution against command injection vulnerabilities.
Bug Fixes:
Enhancements: