Skip to content

refactor: secure password transmission by QDBusUnixFileDescriptor#292

Open
GongHeng2017 wants to merge 1 commit into
linuxdeepin:develop/eaglefrom
GongHeng2017:202605201539-dev-eagle-fix
Open

refactor: secure password transmission by QDBusUnixFileDescriptor#292
GongHeng2017 wants to merge 1 commit into
linuxdeepin:develop/eaglefrom
GongHeng2017:202605201539-dev-eagle-fix

Conversation

@GongHeng2017
Copy link
Copy Markdown
Contributor

Changed from using base64-encoded password strings to using QDBusUnixFileDescriptor for transmitting passwords during network mount operations

Log: Enhanced security of password transmission in network mounting

Influence:

  1. Test network mount with password to ensure successful authentication
  2. Verify anonymous mount still works without password
  3. Validate password saving functionality when savePasswd is enabled
  4. Test mount failure scenarios (wrong password, network error) to ensure error handling
  5. Ensure compatibility with existing D-Bus service (MountControl) that expects file descriptor

feat: 在网络挂载中使用文件描述符安全传输密码

将密码传输从 base64 编码字符串改为使用 QDBusUnixFileDescriptor

Log: 改进了网络挂载中密码传输的安全性
Task: https://pms.uniontech.com/task-view-389921.html

Influence:

  1. 测试带密码的网络挂载,确保认证成功
  2. 验证匿名挂载在无密码时仍能正常工作
  3. 验证启用 savePasswd 时的密码保存功能
  4. 测试挂载失败场景(错误密码、网络错误)以确保错误处理正常
  5. 确保与现有 D-Bus 服务(MountControl)的兼容性,该服务期望接收文件描述符

@GongHeng2017 GongHeng2017 force-pushed the 202605201539-dev-eagle-fix branch 2 times, most recently from 5d13571 to ecae51d Compare May 20, 2026 09:45
Changed from using base64-encoded password strings to using QDBusUnixFileDescriptor for transmitting passwords during network mount operations

Log: Enhanced security of password transmission in network mounting

Influence:
1. Test network mount with password to ensure successful authentication
2. Verify anonymous mount still works without password
3. Validate password saving functionality when savePasswd is enabled
4. Test mount failure scenarios (wrong password, network error) to ensure error handling
5. Ensure compatibility with existing D-Bus service (MountControl) that expects file descriptor

feat: 在网络挂载中使用文件描述符安全传输密码

将密码传输从 base64 编码字符串改为使用 QDBusUnixFileDescriptor

Log: 改进了网络挂载中密码传输的安全性
Task: https://pms.uniontech.com/task-view-389921.html

Influence:
1. 测试带密码的网络挂载,确保认证成功
2. 验证匿名挂载在无密码时仍能正常工作
3. 验证启用 savePasswd 时的密码保存功能
4. 测试挂载失败场景(错误密码、网络错误)以确保错误处理正常
5. 确保与现有 D-Bus 服务(MountControl)的兼容性,该服务期望接收文件描述符
@GongHeng2017 GongHeng2017 force-pushed the 202605201539-dev-eagle-fix branch from ecae51d to 6d2b32f Compare May 21, 2026 01:25
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的 Git Diff。本次代码变更的核心是将网络挂载(CIFS)的密码传输方式从 Base64编码字符串 改为了通过 memfd_create 创建的匿名内存文件描述符(FD)传递(基于 QDBusUnixFileDescriptor。这是一个非常好的安全改进,避免了密码在 D-Bus 总线上以可逆编码(Base64非加密)明文传输的风险。

不过,在语法逻辑、代码质量、性能和安全性方面,这段代码仍有不少需要改进和优化的地方。以下是详细的审查意见:

1. 语法与逻辑

  • QDBusUnixFileDescriptor 的文件描述符所有权与生命周期隐患
    preparePasswd 函数中,你使用 dbusFd.giveFileDescriptor(fd)fd 的所有权转移给了 dbusFd。当 dbusFd 析构时,它会自动 close(fd)。但是,preparePasswd 返回的是 QVariant,如果这个 QVariant 在传递给 D-Bus 之前被复制或发生隐式共享分离,可能会导致底层 FD 提前关闭,从而引发 D-Bus 传输失败。
    建议:确保 QDBusUnixFileDescriptor 作为 QVariant 传递时,其引用计数正确。通常建议直接传递 QDBusUnixFileDescriptor 本身,而不是包装在 QVariant 中,除非底层的 D-Bus 调用层强制要求 QVariantMap。如果必须使用 QVariant,请确保 Q_DECLARE_METATYPE 已正确注册,且在 D-Bus 调用完成前 QVariant 始终有效。
  • loginPasswd 函数返回的密码未经过 preparePasswd 处理
    mountWithSavedInfos 中,你从历史记录中取出密码并调用 preparePasswd。但是在 loginPasswd 函数中(第154行),你移除了 Base64 编码,直接 passwd.insert(kLoginPasswd, pwd)。如果 loginPasswd 的返回值也被用于 D-Bus 通信,那么它仍然是不安全的明文。
    建议:确认 loginPasswd 的返回值流向。如果它也参与 D-Bus 挂载调用,也需要统一调用 preparePasswd
  • lseek 的返回值未严格校验
    lseek 返回的是偏移量,虽然判断 < 0 可以捕获错误,但严格来说应该检查返回值是否等于期望的偏移量(0)。

2. 代码质量

  • 错误处理方式不一致且信息不足
    preparePasswd 中,遇到错误时使用了 qCritical() 并返回 QVariant("")。这存在两个问题:
    1. 调用者无法通过返回值区分“密码为空”和“系统调用失败”,这可能导致后续逻辑出现不可预期的行为(比如 D-Bus 收到一个空字符串而不是 FD)。
    2. 缺乏系统级的错误信息,排查问题时无法知道是权限问题还是内存不足。
      建议:使用 strerror(errno) 打印具体的错误原因;考虑返回无效的 QVariant() (即 QVariant()) 而不是空字符串 QVariant("") 来表示失败,或者引入错误码机制。
  • 魔法字符串 "DBusFD"
    memfd_create("DBusFD", MFD_CLOEXEC) 中的名称是硬编码的。
    建议:将其提取为 static constexpr char kMemfdName[] = "DNetworkMounterPasswd";,提高可读性,并在系统 /proc/self/fd 中更容易辨识。

3. 代码性能

  • 内存映射(mmap)的缺失
    你使用 memfd_create 创建了内存文件,但写入数据时使用的是 ::write()。对于小数据(如密码),write 的开销可以接受,但既然使用了 memfd,更高效且符合惯用法的方式是直接通过 mmap 将内存映射进进程地址空间,直接 memcpy,省去系统调用的开销。
    建议:由于密码长度通常很短,保留 write 是可以接受的,但如果你追求极致性能或后续有扩展,应考虑 mmap + memcpy 的方式。

4. 代码安全

  • memfd 内存中的密码明文未被安全擦除
    QDBusUnixFileDescriptor 析构关闭 FD 时,内核会释放 memfd 对应的内存页。但是,在 preparePasswd 函数的 byteData 中,以及 write 系统调用期间,密码明文曾存在于用户态内存中。如果程序在此期间发生 Core Dump,或者被恶意程序读取内存,密码会泄露。
    建议:在 write 完成后,显式擦除 byteData 的内存。Qt 没有提供原生的安全擦除方法,可以使用类似 explicit_bzero 或 volatile 指针清洗。
  • MFD_CLOEXEC 的必要性审查
    你正确地使用了 MFD_CLOEXEC,这防止了在 fork 执行子进程时 FD 泄漏。这很好,请保持。
  • preparePasswd 中空密码的 FD 泄漏风险(边界情况)
    如果 passwd.isEmpty(),你直接返回了 QVariant("")。这是正确的,避免了创建无意义的 FD。但如果后续有人修改代码,在 isEmpty 判断之后才创建 FD,请务必注意 FD 的释放。

改进后的代码建议

#include <unistd.h>
#include <string.h>
#include <QDBusUnixFileDescriptor>

// 安全擦除 QByteArray 内存的辅助函数
static void secureClearByteArray(QByteArray &ba) {
    if (ba.capacity() > 0) {
        // 使用 volatile 防止编译器优化掉 memset
        volatile char *ptr = ba.data();
        size_t size = ba.size();
        while (size--) {
            *ptr++ = 0;
        }
    }
    ba.clear();
}

static QVariant preparePasswd(const QString &passwd)
{
    if (passwd.isEmpty()) {
        // 返回空 QVariant 表示无需密码,而不是空字符串,以区分错误状态
        return QVariant(); 
    }

    int fd = memfd_create("DNetworkMounterPasswd", MFD_CLOEXEC);
    if (fd < 0) {
        qCritical() << "Failed to create memfd for data transfer, error:" << strerror(errno);
        return QVariant();
    }

    QByteArray byteData = passwd.toUtf8();
    ssize_t written = ::write(fd, byteData.constData(), byteData.size());
    
    // 写入后立即安全擦除内存中的明文
    secureClearByteArray(byteData);
    
    if (written < 0 || static_cast<ssize_t>(passwd.toUtf8().size()) != written) {
        qCritical() << "Failed to write data to memfd, error:" << strerror(errno);
        ::close(fd);
        return QVariant();
    }

    off_t seek_pos = lseek(fd, 0, SEEK_SET);
    if (seek_pos < 0 || seek_pos != 0) {
        qCritical() << "Failed to seek memfd to beginning, error:" << strerror(errno);
        ::close(fd);
        return QVariant();
    }

    QDBusUnixFileDescriptor dbusFd;
    dbusFd.giveFileDescriptor(fd);
    // 注意:fd 的所有权已移交给 dbusFd,后续无需手动 close

    return QVariant::fromValue(dbusFd);
}

总结

你从 Base64 转向 FD 传输的思路非常正确,有效提升了 D-Bus 通信层面的安全性。主要需要关注的是错误处理机制的完善(不要用空字符串代表错误)、内存中明文密码的擦除,以及确认 QDBusUnixFileDescriptorQVariant 中的生命周期是否满足 D-Bus 异步/同步调用的需求

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GongHeng2017, Johnson-zs

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

Details Needs approval from an approver in each of these files:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants