fix: improve socket security and refactor hook execution#1673
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves security by moving the package manager socket to a dedicated /tmp/linglong directory with restricted permissions and refactors hook command execution to use a Cmd wrapper instead of std::system. The review feedback identifies a critical security vulnerability regarding insecure directory creation in /tmp (susceptible to TOCTOU/symlink attacks) and recommends verifying directory ownership and type using lstat and getuid(), as well as including <unistd.h>.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!std::filesystem::exists(socketDir, ec)) { | ||
| std::filesystem::create_directory(socketDir, ec); | ||
| if (ec) { | ||
| LogE("Failed to create socket directory /tmp/linglong: {}", ec.message()); | ||
| return -1; | ||
| } | ||
| std::filesystem::permissions( | ||
| socketDir, | ||
| std::filesystem::perms::owner_all | std::filesystem::perms::group_read | ||
| | std::filesystem::perms::group_exec | std::filesystem::perms::others_read | ||
| | std::filesystem::perms::others_exec, | ||
| ec); | ||
| if (ec) { | ||
| LogE("Failed to set permissions on /tmp/linglong: {}", ec.message()); | ||
| return -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Creating a directory in a world-writable location like /tmp without verifying its ownership and type introduces a critical security vulnerability (symlink/TOCTOU attack). If an attacker pre-creates /tmp/linglong with loose permissions (e.g., 0777), they can delete or replace the socket file inside it because directory write permissions allow file deletion/renaming.
To prevent this, you should:
- Use
lstatto verify that if/tmp/linglongalready exists, it is a directory (not a symlink) and is owned by the current user (getuid()). - Set the directory permissions to
0700(owner-all) to prevent other users from accessing or manipulating the directory contents.
struct stat st;
if (lstat(socketDir.c_str(), &st) == 0) {
if (!S_ISDIR(st.st_mode)) {
LogE("Path /tmp/linglong exists but is not a directory");
return -1;
}
if (st.st_uid != getuid()) {
LogE("Directory /tmp/linglong is owned by UID {}, expected {}", st.st_uid, getuid());
return -1;
}
// Ensure secure permissions (0700) on the existing directory
if (chmod(socketDir.c_str(), S_IRWXU) != 0) {
LogE("Failed to set secure permissions on /tmp/linglong: {}", strerror(errno));
return -1;
}
} else {
if (errno != ENOENT) {
LogE("Failed to stat /tmp/linglong: {}", strerror(errno));
return -1;
}
std::filesystem::create_directory(socketDir, ec);
if (ec) {
LogE("Failed to create socket directory /tmp/linglong: {}", ec.message());
return -1;
}
std::filesystem::permissions(socketDir, std::filesystem::perms::owner_all, ec);
if (ec) {
LogE("Failed to set permissions on /tmp/linglong: {}", ec.message());
return -1;
}
}
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11 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 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances security by creating a dedicated, restricted directory (/tmp/linglong) for the package manager's UNIX socket and setting strict permissions on both the directory and the socket file. It also refactors hook command execution to use the Cmd utility instead of std::system. A security review identified a TOCTOU vulnerability and insecure shared directory ownership when creating /tmp/linglong, recommending using mkdir atomically and verifying directory ownership.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
dengbo11
force-pushed
the
fix-security
branch
2 times, most recently
from
ba7b6a8 to
bd8ad4b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves security by creating a dedicated, restricted socket directory under /tmp/linglong and replacing manual shell command escaping and std::system calls with a safer Cmd wrapper. The review feedback identifies a potential Time-of-Check to Time-of-Use (TOCTOU) race condition during the directory's creation and permission checks, recommending the use of file descriptors (open, fstat, and fchmod) to perform these operations safely, as well as including the <fcntl.h> header.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (mkdir(socketDir.c_str(), S_IRWXU) == 0) { | ||
| } else if (errno == EEXIST) { | ||
| struct stat st; | ||
| if (lstat(socketDir.c_str(), &st) != 0) { | ||
| LogE("Failed to stat /tmp/linglong: {}", strerror(errno)); | ||
| return -1; | ||
| } | ||
| if (S_ISLNK(st.st_mode)) { | ||
| LogE("Failed to create /tmp/linglong: it is a symbolic link. Please remove it."); | ||
| return -1; | ||
| } | ||
| if (!S_ISDIR(st.st_mode)) { | ||
| LogE("Failed to create /tmp/linglong: it exists but is not a directory. Please " | ||
| "remove it."); | ||
| return -1; | ||
| } | ||
| if (st.st_uid != getuid()) { | ||
| LogE("Failed to create /tmp/linglong: owner mismatch (expected uid {}, got {}). " | ||
| "Please remove it.", | ||
| getuid(), | ||
| st.st_uid); | ||
| return -1; | ||
| } | ||
| if (chmod(socketDir.c_str(), S_IRWXU) != 0) { | ||
| LogE("Failed to set permissions on /tmp/linglong: {}", strerror(errno)); | ||
| return -1; | ||
| } | ||
| } else { | ||
| LogE("Failed to create /tmp/linglong: {}", strerror(errno)); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
The directory check and permission setting logic has a Time-of-Check to Time-of-Use (TOCTOU) race condition. If an attacker replaces /tmp/linglong with a symbolic link after the lstat check but before the chmod call, chmod will follow the symbolic link and modify the permissions of the target file/directory (potentially a sensitive system file). To prevent this, open the directory safely using open with O_DIRECTORY | O_NOFOLLOW to obtain a file descriptor, and then perform fstat and fchmod on that descriptor. This also eliminates the empty if block style issue.
if (mkdir(socketDir.c_str(), S_IRWXU) != 0) {
if (errno == EEXIST) {
int fd = open(socketDir.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC);
if (fd < 0) {
LogE("Failed to open /tmp/linglong safely: {}", strerror(errno));
return -1;
}
struct stat st;
if (fstat(fd, &st) != 0) {
LogE("Failed to fstat /tmp/linglong: {}", strerror(errno));
close(fd);
return -1;
}
if (st.st_uid != getuid()) {
LogE("Failed to create /tmp/linglong: owner mismatch (expected uid {}, got {}).",
getuid(),
st.st_uid);
close(fd);
return -1;
}
if (fchmod(fd, S_IRWXU) != 0) {
LogE("Failed to set permissions on /tmp/linglong: {}", strerror(errno));
close(fd);
return -1;
}
close(fd);
} else {
LogE("Failed to create /tmp/linglong: {}", strerror(errno));
return -1;
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves security and robustness by moving the package manager's Unix socket to a dedicated /tmp/linglong directory with strict ownership and permission checks, and by replacing the use of std::system with a safer Cmd wrapper for executing hook commands. A review comment identifies a potential file descriptor leak in the socket directory setup where fd is not guarded by finally immediately after being opened, which could lead to leaks if subsequent checks fail.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| int fd = open(socketDir.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); | ||
| if (fd < 0) { | ||
| LogE("Failed to open /tmp/linglong safely: {}", strerror(errno)); | ||
| return -1; | ||
| } | ||
| struct stat st; | ||
| if (fstat(fd, &st) != 0) { | ||
| LogE("Failed to fstat /tmp/linglong: {}", strerror(errno)); | ||
| return -1; | ||
| } | ||
| if (st.st_uid != getuid()) { | ||
| LogE( | ||
| "Failed to create /tmp/linglong: owner mismatch (expected uid {}, got {}).", | ||
| getuid(), | ||
| st.st_uid); | ||
| return -1; | ||
| } | ||
| if (fchmod(fd, S_IRWXU) != 0) { | ||
| LogE("Failed to set permissions on /tmp/linglong: {}", strerror(errno)); | ||
| return -1; | ||
| } | ||
|
|
||
| auto close_fd = linglong::utils::finally::finally([fd] { | ||
| close(fd); | ||
| }); |
There was a problem hiding this comment.
The file descriptor fd is leaked if any of the subsequent checks (fstat, st_uid check, or fchmod) fail and return -1 before the finally guard close_fd is constructed. Moving the construction of close_fd immediately after the successful open call ensures that the file descriptor is always safely closed on any exit path.
int fd = open(socketDir.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC);
if (fd < 0) {
LogE("Failed to open /tmp/linglong safely: {}", strerror(errno));
return -1;
}
auto close_fd = linglong::utils::finally::finally([fd] {
close(fd);
});
struct stat st;
if (fstat(fd, &st) != 0) {
LogE("Failed to fstat /tmp/linglong: {}", strerror(errno));
return -1;
}
if (st.st_uid != getuid()) {
LogE(
"Failed to create /tmp/linglong: owner mismatch (expected uid {}, got {}).",
getuid(),
st.st_uid);
return -1;
}
if (fchmod(fd, S_IRWXU) != 0) {
LogE("Failed to set permissions on /tmp/linglong: {}", strerror(errno));
return -1;
}
|
/gemini review |
deepin pr auto review代码审查报告总体评价这段代码主要实现了 linglong 包管理器的核心功能，包括守护进程模式、D-Bus 通信和钩子执行等功能。代码整体质量较高，但有一些可以改进的地方。 主要改进建议1. 安全性增强main.cpp 改进：
hooks.cpp 改进：
2. 错误处理改进main.cpp 改进：
hooks.cpp 改进：
3. 代码结构改进main.cpp 改进：
hooks.cpp 改进：
4. 性能优化main.cpp 改进：
hooks.cpp 改进：
5. 日志记录改进main.cpp 改进：
hooks.cpp 改进：
具体代码建议
auto createAndSecureSocketPath(const std::filesystem::path &socketDir) -> std::optional<std::filesystem::path>
{
// 实现细节...
}
auto executeHookWithTimeout(const std::string &command,
const std::vector<std::pair<std::string, std::string>> &envVars,
std::chrono::seconds timeout) -> utils::error::Result<void>
{
// 实现细节...
}
总结代码整体质量良好，安全性考虑较为周全。主要的改进空间在于代码结构的模块化、错误处理的完善和日志记录的增强。建议在后续开发中逐步实施这些改进，以提高代码的可维护性和可靠性。 |
There was a problem hiding this comment.
Code Review
This pull request improves security and robustness by moving the package manager's UNIX domain socket to a dedicated, securely-created directory (/tmp/linglong) and refactoring hook command execution to use a helper class instead of std::system. A security review comment correctly identifies a race condition between socket creation and the subsequent chmod call, recommending the use of umask to securely initialize the socket file permissions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| auto *server = new QDBusServer(QString("unix:path=%1").arg(socketPath.c_str()), | ||
| QCoreApplication::instance()); | ||
| if (!server->isConnected()) { | ||
| LogE("listen on socket: {}", server->lastError().message().toStdString()); | ||
| LogE("listen on socket {}: {}", | ||
| socketPath.c_str(), | ||
| server->lastError().message().toStdString()); | ||
| return -1; | ||
| } | ||
| QObject::connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, []() { | ||
| if (QDir::root().remove("/tmp/linglong-package-manager.socket")) { | ||
| return; | ||
| } | ||
| LogE("failed to remove /tmp/linglong-package-manager.socket."); | ||
| }); | ||
|
|
||
| // Set socket file permissions: only owner can read/write (0600) | ||
| if (chmod(socketPath.c_str(), S_IRUSR | S_IWUSR) != 0) { | ||
| LogE("Failed to set socket permissions: {}", strerror(errno)); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
There is a security race condition between the creation of the UNIX domain socket by QDBusServer and the subsequent chmod call. During this window, the socket file is created with default permissions (governed by the system umask, typically 0755 or 0777), which may allow other local users to connect to the socket before its permissions are restricted to 0600.
To prevent this race condition securely, set the process umask to 0077 before creating the QDBusServer and restore it afterward. This ensures the socket file is created with secure permissions (0600/0700) from the very beginning, and eliminates the need for a separate chmod call.
// Set umask to 0077 to ensure the socket is created with secure permissions (0600/0700)
// and avoid a race condition between socket creation and chmod.
mode_t oldUmask = ::umask(0077);
auto *server = new QDBusServer(QString("unix:path=%1").arg(socketPath.c_str()),
QCoreApplication::instance());
::umask(oldUmask);
if (!server->isConnected()) {
LogE("listen on socket {}: {}",
socketPath.c_str(),
server->lastError().message().toStdString());
return -1;
}2 participants
Influence: