sync: from linuxdeepin/dde-session-shell

[deepin-ci-robot]

Synchronize source files from linuxdeepin/dde-session-shell.

Source-pull-request: linuxdeepin/dde-session-shell#68

[sourcery-ai]

Sorry @deepin-ci-robot, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[deepin-ci-robot]

deepin pr auto review

你好！我是CodeGeex。我已经仔细审查了你提供的Git Diff，该代码的主要目的是引入 deepin-security-loader 机制，通过包装脚本和文件描述符（FD）握手，让 dde-lock 进程获得特定D-Bus服务的免Polkit授权。

整体设计思路清晰，但代码在安全性、逻辑严谨性、资源管理和代码质量方面存在一些需要改进的地方。以下是详细的审查意见：

一、 安全性问题 🚨

1. 文件描述符（FD）缺乏校验，存在严重的本地提权风险
在 dde-lock.cpp 中，直接将命令行传入的 fd1 和 fd2 转换为整数并传入 SecurityLoaderHelper：

int fd1 = cmdParser.isSet(fd1Opt) ? cmdParser.value(fd1Opt).toInt() : -1;

风险：如果恶意用户通过命令行传入属于其他进程或敏感文件（如 /etc/shadow）的 FD，dde-lock 可能会向其写入 JSON 数据或从中读取数据，导致数据破坏或信息泄露。
改进建议：必须校验传入的 FD 是否合法且属于预期的 security-loader 管道。可以使用 fstat() 和 S_ISFIFO() 检查 FD 是否为管道，或者通过 fcntl 获取 FD 标志进行验证。

2. 包装脚本的降级回退逻辑削弱了安全性
在 dde-lock-loader-wrapper 中，如果 deepin-security-loader 不可用或没有 cap_setgid，脚本会降级直接运行 dde-lock：

# Fallback: direct launch without loader (no polkit-free authorization)
exec "$REAL_BINARY" "$@"

风险：恶意用户可以通过篡改环境变量（如修改 PATH 指向伪造的 getcap）、删除 loader 文件或移除 capability，迫使锁屏进程降级启动，从而绕过安全授权机制。
改进建议：对于安全敏感程序，降级回退应当非常谨慎。建议在降级时记录警告级别的系统日志，或者在安全要求严格的场景下，直接拒绝启动（退出码非0），而不是静默降级。

3. 版权年份异常
dde-lock.cpp 中的版权年份被修改为 2026，这显然是一个逻辑错误或笔误，应当修正为当前年份或保留原年份。

二、 语法与逻辑问题 🧩

1. FD 管道通信存在死锁风险
在 performHandshake 中，代码先向 fd1 写入数据并 close()，然后从 fd2 读取数据：

fd1File.write(jsonData);
fd1File.close();
// ...
QByteArray response = fd2File.readAll();

风险：如果 security-loader 端的实现是等待读取完 fd1 后才开始向 fd2 写入，且写入的数据量超过管道缓冲区，那么 loader 会在写 fd2 时阻塞，而 dde-lock 会在 readAll() 处阻塞，导致死锁。
改进建议：确保协议设计为非阻塞或半关闭模式。当前代码在写入后立即 close() 是正确的（触发了 EOF），但需确认 loader 端是在读取到 EOF 后才响应，且响应数据不会撑爆 fd2 的内核缓冲区。

2. 无用的 D-Bus 调用
在 performHandshake 中有一行毫无意义的代码：

systemBus.interface()->isServiceRegistered("org.freedesktop.DBus");

这不仅浪费了一次系统调用和IPC开销，且返回值被丢弃。应将其删除。

3. QFile::open(int fd) 的生命周期陷阱
QFile 接管了 fd1 和 fd2，当 QFile 析构时，底层的文件描述符会被自动 close()。
风险：如果未来有人在此函数后还需要使用这两个 FD，会导致 “Bad file descriptor” 错误。虽然当前逻辑下后续不需要使用，但建议显式说明所有权转移，或者在 QFile 打开时设置 DontCloseHandle（但这可能导致 FD 泄漏），当前写法可以接受，但需加注释说明 QFile 会关闭 FD。

三、 代码性能与质量 ⚡

1. appendDest 中的 O(N²) 去重逻辑

for (const auto &existing : m_destList) {
    const QJsonObject current = existing.toObject();
    // ... 比较
}
m_destList.append(dest);

问题：每次添加新条目都遍历整个 QJsonArray，时间复杂度为 O(N²)。且 QJsonArray 内部是隐式共享的深拷贝结构，频繁修改效率较低。
改进建议：在加载配置阶段，可以使用 QSet<QString> 或 QHash 来进行去重判断，构建完成后再转换为 QJsonArray。

2. parseJsonFile 一次性读取大文件

QJsonDocument doc = QJsonDocument::fromJson(file.readAll(), &error);

问题：readAll() 会将整个文件内容一次性载入内存。如果配置文件被恶意替换为超大文件，可能导致内存瞬时飙升。
改进建议：对于配置文件而言通常很小，这里问题不大。但如果追求极致安全，可使用 QJsonStreamReader 进行流式解析，并限制读取的最大字节数。

3. Wrapper 脚本的路径硬编码

REAL_BINARY="/usr/libexec/deepin/dde-lock"
LOADER="/usr/bin/deepin-security-loader"

问题：路径硬编码不利于多发行版适配或自定义安装前缀。
改进建议：CMake 在安装 wrapper 脚本时，可以通过 configure_file 对脚本进行变量替换（如 @CMAKE_INSTALL_FULL_LIBEXECDIR@），而不是在源码中写死 /usr/libexec。

四、 改进后的代码示例 💻

针对以上核心问题，提供以下修改建议代码：

1. C++ 端增加 FD 校验（dde-lock.cpp / securityloaderhelper.cpp）

// securityloaderhelper.cpp 中添加 FD 校验函数
#include <sys/stat.h>

bool SecurityLoaderHelper::isValidFd(int fd) {
    if (fd < 0) return false;
    struct stat st;
    if (fstat(fd, &st) != 0) {
        return false;
    }
    // 必须是管道或套接字，拒绝普通文件
    return S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode);
}

void SecurityLoaderHelper::doSecurityLoader(int fd1, int fd2)
{
    if (fd1 < 0 || fd2 < 0) {
        qCWarning(secLoader) << "Not loaded by loader, skipping handshake";
        return;
    }

    // 安全校验
    if (!isValidFd(fd1) || !isValidFd(fd2)) {
        qCCritical(secLoader) << "Security: Invalid file descriptors received! Possible spoofing attack.";
        return; // 或者直接 exit(1)
    }

    qCInfo(secLoader) << "Detected loader injection: fd1=" << fd1 << "fd2=" << fd2;
    // ... 后续逻辑
}

2. 优化去重逻辑（securityloaderhelper.cpp）

void SecurityLoaderHelper::loadConfig(const QString &configPath)
{
    QString path = configPath.isEmpty() ? DEFAULT_CONFIG_PATH : configPath;
    qCInfo(secLoader) << "Loading permission config from:" << path;

    // 使用 QSet 进行高效去重
    QSet<QString> existingKeys;
    for (const auto &item : m_destList) {
        QJsonObject obj = item.toObject();
        existingKeys.insert(obj["DbusName"].toString() + obj["DbusPath"].toString() + obj["DbusInterface"].toString());
    }

    // 假设 parseJsonFile 返回解析出的新列表
    QJsonArray newDests = parseJsonFile(path); // 需重构 parseJsonFile 使其返回结果

    for (const auto &item : newDests) {
        QJsonObject obj = item.toObject();
        QString key = obj["DbusName"].toString() + obj["DbusPath"].toString() + obj["DbusInterface"].toString();
        if (!existingKeys.contains(key)) {
            m_destList.append(item);
            existingKeys.insert(key);
        }
    }
}

3. 删除无用代码并增加注释（securityloaderhelper.cpp）

bool SecurityLoaderHelper::performHandshake(int fd1, int fd2)
{
    // ... 前置校验 ...
    QDBusConnection systemBus = QDBusConnection::systemBus();
    if (!systemBus.isConnected()) {
        qCWarning(secLoader) << "Cannot connect to system bus";
        return false;
    }
    
    // 删除这行无用代码: systemBus.interface()->isServiceRegistered("org.freedesktop.DBus");
    QString uniqueName = systemBus.baseService();
    
    // ... 构建 JSON ...

    // 注意: QFile 接管 fd 后，析构时会自动 close(fd1)，后续不可再使用 fd1
    QFile fd1File;
    if (!fd1File.open(fd1, QIODevice::WriteOnly)) { /* ... */ }
    fd1File.write(jsonData);
    fd1File.close();

    // 注意: 同理，fd2 会被自动关闭
    QFile fd2File;
    if (!fd2File.open(fd2, QIODevice::ReadOnly)) { /* ... */ }
    // ... 读取逻辑 ...
}

4. CMake 动态配置 Wrapper 脚本
将 files/dde-lock-loader-wrapper 改为 files/dde-lock-loader-wrapper.in：

#!/bin/bash
REAL_BINARY="@CMAKE_INSTALL_FULL_LIBEXECDIR@/deepin/dde-lock"
LOADER="@CMAKE_INSTALL_FULL_BINDIR@/deepin-security-loader"
# ... 其他逻辑不变

在 CMakeLists.txt 中：

configure_file(
    ${CMAKE_CURRENT_SOURCE_DIR}/files/dde-lock-loader-wrapper.in
    ${CMAKE_CURRENT_BINARY_DIR}/dde-lock-loader-wrapper
    @ONLY
)

install(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/dde-lock-loader-wrapper DESTINATION ${CMAKE_INSTALL_BINDIR}
    RENAME dde-lock
    PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE)

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deepin-ci-robot, xionglinlin

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• debian/deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment