Skip to content

Conversation

@wangrong1069
Copy link
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

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

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

Log: 改进了网络挂载中密码传输的安全性

Influence:

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

@wangrong1069 wangrong1069 force-pushed the cifs-mount branch 2 times, most recently from 0c13563 to 39089d0 Compare January 15, 2026 08:28
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

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

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

Log: 改进了网络挂载中密码传输的安全性

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

deepin pr auto review

这段,这段代码主要针对 Qt 6 版本进行了适配,特别是在处理 CIFS 挂载密码传输方式上做了重大改进:从直接传输 Base64 字符串改为使用 Unix 管道和文件描述符通过 D-Bus 传输。这是一个很好的安全改进。

以下是对该 diff 的详细审查意见:

1. 语法逻辑

  • Qt 版本判断宏:代码中大量使用了 #if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)。这是正确的做法,保证了向后兼容性,逻辑清晰。
  • 管道资源管理:在 preparePasswd 函数中,对 pipe 创建的文件描述符(FD)处理逻辑基本正确:
    • 写端 (write_fd) 在写入后立即关闭。
    • 读端 (read_fd) 在被 QDBusUnixFileDescriptor 复制后关闭。
    • 潜在问题:如果在 write 调用失败时,代码关闭了 read_fd 并返回。这是正确的。但如果 write 成功但 QDBusUnixFileDescriptor 构造抛出异常(虽然 Qt 很少这样做),read_fd 可能会泄漏。不过鉴于 Qt 的设计,这种情况风险较低。
  • Base64 编码逻辑:在 Qt 5 分支中,loginPasswd 函数将密码进行 Base64 编码后存入 QVariant。而在 mountWithUserInput 的保存密码逻辑中,又将 Base64 解码回明文保存。这个闭环逻辑是自洽的。

2. 代码质量

  • 错误处理preparePasswd 函数包含了 pipe 创建失败、写入长度不匹配的错误检查,并使用 qCritical 输出日志,这很好。
  • 代码重复preparePasswd 函数被封装为静态函数,提高了代码复用性,避免了在 mountWithUserInputmountWithSavedInfos 中重复编写管道逻辑。
  • 日志:增加了调试日志(如 qDebug() << "Created empty QVariant..."),有助于排查问题。
  • 硬编码字符串"cifs" 字符串在代码中直接出现,如果未来支持其他文件系统,建议定义为常量。

3. 代码性能

  • 内存与系统调用:引入 pipewrite 系统调用相比于纯内存操作(QString 传递)会有微小的性能开销,但考虑到挂载操作本身是重 IO 操作,且此操作仅在挂载瞬间触发,这点性能损耗完全可以忽略不计。
  • 数据拷贝passwd.toLocal8Bit() 会产生一次数据拷贝,随后写入管道。这是必要的转换过程。

4. 代码安全

  • 显著提升:这是本次修改最大的亮点。
    • Qt 5 (旧方式):密码被 Base64 编码后作为普通字符串参数通过 D-Bus 传递。Base64 只是编码,不是加密。任何拥有 D-Bus 监控权限(如 dbus-monitor)或能够访问系统总线消息的进程都能截获并解码出明文密码。
    • Qt 6 (新方式):使用 QDBusUnixFileDescriptor 通过 Unix 管道传递密码。这种方式利用了 Unix domain socket 的辅助数据机制,数据在内核空间传递,不会暴露在用户态的消息队列中,且只有接收进程(拥有权限的守护进程)能读取该 FD。这极大地提高了安全性,防止了密码在进程间通信时泄露。
  • 内存安全:密码字符串 passwd 在转换为 QByteArray 并写入管道后,原字符串可能仍存在于内存中。对于极高安全要求的场景,可能需要考虑在写入后对内存中的明文密码进行清零(memset_s 或类似操作),但在一般桌面应用场景下,这通常不是必须的。
  • 空密码处理preparePasswd 对空密码有特殊处理,返回空字符串。这意味着空密码可能不会走管道传输。需要确认接收端(Daemon)是否正确处理了空 QVariant 的情况,以免造成挂载失败或逻辑错误。

改进建议

  1. 文件描述符标志位 (FD_CLOEXEC)
    preparePasswd 函数中创建管道后,建议设置 FD_CLOEXEC 标志。防止在 fork + exec 子进程时意外泄露密码文件描述符。

    #include <fcntl.h>
    // ... 在 pipe(pipefds) 之后 ...
    ::fcntl(pipefds[0], F_SETFD, FD_CLOEXEC);
    ::fcntl(pipefds[1], F_SETFD, FD_CLOEXEC);
  2. 原子写入
    虽然密码通常很短,不太可能超过 PIPE_BUF(通常为 4096 字节),但为了代码的健壮性,可以检查写入是否原子。或者明确指出假定密码长度小于 PIPE_BUF

  3. 错误处理的统一性
    preparePasswd 中,如果 write 返回的字节数不匹配,直接关闭 read_fd 并返回空字符串。调用方 mountWithUserInput 会将空 QVariant 传给后端。建议确认后端对“空密码”和“错误密码”的区分逻辑是否清晰。

  4. 清理敏感数据
    虽然是 C++ 且难以保证,但在 passwdBytes 析构前(即函数作用域结束前),如果可以,尝试将缓冲区清零是一种良好的安全习惯,尽管 Qt 的 QByteArray 并不提供安全的 zeroOut 方法。

  5. 代码注释
    建议在 preparePasswd 函数上方添加详细注释,解释为什么 Qt 6 需要使用管道传递密码(安全性原因),方便后续维护者理解。

总结来说,这是一个高质量的改动,主要解决了 D-Bus 传输密码的安全隐患,逻辑清晰,兼容性处理得当。加上上述关于 FD_CLOEXEC 的微调后,代码将更加健壮和安全。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@wangrong1069
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Jan 15, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 760b864 into linuxdeepin:master Jan 15, 2026
18 of 20 checks passed
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