fix: prevent local root exploit via directory hijacking

[ComixHe]

Rewrite newPwdChangerX to fix critical vulnerability where chown() on .Xauthority in unprivileged-controlled directory allowed local root exploit. Use random directories, file descriptors, O_EXCL, and O_NOFOLLOW to prevent TOCTOU and symlink attacks.

[sourcery-ai]

Sorry @ComixHe, 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

这份代码审查主要针对 user_chpwd_union_id.go 文件的修改部分。代码主要涉及在 Linux 环境下处理 X11 认证（Xauthority）和密码重置流程。

以下是关于语法逻辑、代码质量、代码性能和代码安全的详细审查意见：

1. 代码安全

这是本次修改中表现最突出的部分，改进非常显著。

• 使用 crypto/rand 替代伪随机数生成器：

  • 改进点：新增的 randomString 函数使用了 crypto/rand，这是一个密码学安全的随机数生成器（CSPRNG）。
  • 评价：这对于生成临时目录名（.pwdChanger_x- 前缀）至关重要。如果使用 math/rand 或时间戳，攻击者可能预测临时目录路径，从而利用 TOCTOU（Time-of-check to time-of-use）竞态条件攻击，在目录创建和文件写入之间进行符号链接攻击，导致写入任意位置文件。
  • 细节：代码中通过 maxByte 处理了模偏移偏差，这是很好的实践，确保了字符分布的均匀性。

• 防御性编程：文件描述符与路径操作：

  • 改进点：代码从使用 os 包的高层级 API（如 os.Mkdir, filepath.Join）转向了 golang.org/x/sys/unix 包的系统调用（如 unix.Open, unix.Openat, unix.Mkdirat）。
  • 评价：这是一个巨大的安全提升。
    • 避免符号链接攻击：通过使用 O_NOFOLLOW 标志，防止了符号链接攻击。
    • 目录文件描述符：通过 unix.Openat 基于父目录的文件描述符（run fd）进行操作，而不是拼接绝对路径字符串。这消除了路径拼接过程中可能存在的路径穿越风险。
    • 竞态条件：先创建目录，再使用 Openat 获取其 FD，后续操作基于 FD 进行，即使目录被其他进程删除或替换，FD 依然指向原来的 inode，大大提高了安全性。

• 权限控制：

  • 改进点：创建目录和文件时明确指定了权限 0o700 和 0o600，并立即进行了 chown。
  • 评价：确保了敏感的 Xauth 文件只有所有者可读写，防止了信息泄露。

• 命令注入风险缓解：

  • 改进点：在调用 xauth 时，改用了 StdinPipe 批量写入命令，而不是构造多个命令行参数。
  • 评价：虽然 exec.Command 本身对参数有转义保护，但直接通过管道写入标准输入（stdin）进一步减少了参数解析的复杂性和潜在的注入风险。同时，stderr 的捕获有助于调试和审计。

2. 代码质量

• 资源管理：

  • 改进点：newPwdChangerX 和 clean 函数中使用了 defer 和复杂的错误处理逻辑来确保文件描述符（run, xAuthDir, xauthFd）被关闭，临时文件被删除。
  • 建议：虽然逻辑是正确的，但 newPwdChangerX 中的 defer 清理逻辑比较复杂，嵌套了多个 if 判断。虽然为了安全这是必要的，但建议添加更详细的注释，解释为什么需要在每个错误分支手动调用 Unlinkat 和 Close，以及为什么最后要将 run 和 xAuthDir 置为 -1（所有权转移）。
  • 潜在问题：在 clean 函数中，使用了 defer 来清理资源。如果 clean 被多次调用（虽然逻辑上不太可能），或者 pcrx 结构体被复用（虽然看起来是一次性的），需要确保逻辑的幂等性。目前的实现通过检查 != -1 已经处理了大部分情况，代码质量较高。

• 代码风格：

  • 改进点：常量定义使用了 const (...) 块，变量声明使用了 var (...) 块，符合 Go 语言规范。
  • 改进点：使用了 0o500 这种八进制字面量写法，比 0500 更清晰。
  • 建议：randomString 函数中的 for 循环稍微有些复杂，虽然性能是好的，但可读性稍差。可以考虑将内部循环提取或注释说明其用途（即过滤掉会导致模偏移的字节）。

3. 代码性能

• 系统调用开销：

  • 观察：代码从 Go 标准库的 os 包切换到了 unix 包的直接系统调用。通常 os 包内部也会调用这些系统调用，但 unix 包减少了中间层的抽象和检查。
  • 评价：对于这种安全关键的代码路径，性能开销通常不是瓶颈（主要瓶颈是 IO 和进程启动），因此这种为了安全而牺牲一点点封装便利性的做法是完全值得的。

• 随机数生成：

  • 观察：crypto/rand 比 math/rand 慢。
  • 评价：考虑到这里只是生成一个 12 字符的随机字符串，性能损耗可以忽略不计，安全性收益远大于性能损耗。

• Xauth 操作优化：

  • 改进点：将原来的循环调用 xauth add（每次启动一个 runuser 进程）改为启动一个 xauth 进程并通过 Stdin 批量写入所有 add 命令。
  • 评价：这是一个显著的性能提升。启动进程（尤其是 runuser 涉及权限切换）是昂贵的操作。减少进程创建次数可以明显提高函数执行速度。

4. 语法逻辑

• 逻辑正确性：

  • 改进点：在 newPwdChangerX 中，使用了 for 循环（最多 64 次）来创建临时目录，处理了并发创建时的 os.IsExist 冲突。
  • 评价：逻辑严密。之前的代码如果目录存在会直接 RemoveAll，这在高并发或异常情况下可能误删其他进程正在使用的目录。现在的重试机制更加安全。

• 错误处理：

  • 改进点：增强了错误日志，例如在 xauth list 失败时打印 stderr。
  • 评价：这极大地有助于排查问题。

总结与建议

这段代码的修改质量非常高，主要目标是提升安全性（防止 TOCTOU 和符号链接攻击），同时也优化了性能（减少进程创建）。

主要改进建议：

1. 注释增强：虽然代码逻辑正确，但涉及到底层 Unix 文件描述符的操作比较复杂。建议在 newPwdChangerX 函数头部添加注释，解释使用 O_PATH、O_NOFOLLOW 以及基于 FD 操作的安全设计意图。
2. 清理逻辑封装：newPwdChangerX 中的错误处理 defer 块和 clean 方法中有重复的清理逻辑（Unlinkat 和 Close）。可以考虑提取一个私有的辅助函数 cleanupResources(parentFd, dirFd, dirName string) 来减少重复代码，但这可能会影响 defer 的局部变量捕获，需权衡。
3. 常量定义：randomString 中的 charset 和 maxByte 可以定义为包级常量，避免每次调用函数时重新计算（虽然编译器可能会优化）。
4. 错误检查：在 randomString 中，rand.Read 的错误处理是正确的，但在极低概率下 buf 大小不足（虽然逻辑上 n+(n/4) 应该足够），可以添加一个断言或注释说明缓冲区大小的计算依据。

总体而言，这是一次非常专业的代码重构，显著提升了系统的安全性和健壮性。

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ComixHe, fly602

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