fix: prevent symlink attack in removeLoginKeyring

[ComixHe]

The removeLoginKeyring() function previously accessed files in user's home directory as root using full paths, which could be exploited for local DoS attacks or deleting arbitrary users' login.keyring files via symlink attacks.

These changes ensure that all file operations are performed on the verified directory, preventing symlink-based attacks.

Pms: BUG-349987
Log: prevent symlink attack in removeLoginKeyring

[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

Git Diff 代码审查报告

总体评价

这段代码修改主要将 removeLoginKeyring 函数中的文件操作从 Go 标准库函数替换为 Linux 系统调用，同时改进了路径拼接方式。修改的主要目的是提高安全性和性能。下面从多个维度进行详细分析。

语法逻辑分析

1. 路径拼接改进:

   • 使用 filepath.Join 替代 path.Join，更适合处理文件系统路径
   • 使用多个参数而非字符串拼接，提高了代码可读性和跨平台兼容性

2. 系统调用替换:

   • 使用 unix.Open 和 unix.Openat 替代 os.ReadFile 和 os.IsFileExist
   • 使用 unix.Fstat 检查文件类型
   • 使用 unix.Read 替代直接读取文件内容
   • 使用 unix.Unlinkat 替代 os.Remove

3. 错误处理:

   • 保留了原有的错误处理逻辑
   • 添加了更详细的日志记录

代码质量评价

1. 优点:

   • 使用了更安全的文件操作方式，如 O_NOFOLLOW 防止符号链接攻击
   • 使用 O_CLOEXEC 防止文件描述符泄漏到子进程
   • 添加了文件类型检查，确保只操作常规文件
   • 使用了更精确的文件描述符操作，减少竞态条件

2. 可改进点:

   • isUseWhiteboxFunc 内部函数可以提取为独立函数，提高可测试性
   • 魔法数字 '1' 和 '1' 可以定义为常量，提高可读性
   • 日志信息可以更加结构化，便于后续分析

代码性能分析

1. 性能提升:

   • 使用系统调用减少了不必要的中间步骤
   • 通过文件描述符操作减少了路径解析开销
   • 只读取文件的前两个字节，而不是整个文件

2. 潜在性能问题:

   • 每次调用都需要打开和关闭文件描述符，如果频繁调用可能造成性能问题
   • 文件描述符使用 defer 关闭，在高并发场景下可能导致资源占用

代码安全性评价

1. 安全性提升:

   • 使用 O_NOFOLLOW 防止符号链接攻击
   • 使用 O_CLOEXEC 防止文件描述符泄漏
   • 添加了文件类型检查，防止操作非预期文件
   • 使用 unix.Fstat 而非 unix.Stat，基于文件描述符而非路径，减少竞态条件

2. 潜在安全风险:

   • 虽然使用了 O_NOFOLLOW，但目录本身仍可能是符号链接
   • 文件权限检查不够明确，可能存在权限提升风险
   • 错误信息可能泄露系统路径等敏感信息

改进建议

1. 代码结构优化:

   const (
       whiteBoxStatusEnabled = "11"
       keyringDir            = ".local/share/deepin-keyrings-wb"
       statusFile            = "status"
       loginKeyringFile      = "login.keyring"
   )
   
   // 将 isUseWhiteboxFunc 提取为独立函数
   func isWhiteBoxKeyringEnabled(dirFd int) bool {
       statusFd, err := unix.Openat(dirFd, statusFile, unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
       if err != nil {
           logger.Warning("failed to open keyring status file:", err)
           return false
       }
       defer unix.Close(statusFd)
   
       var stat unix.Stat_t
       if err := unix.Fstat(statusFd, &stat); err != nil {
           logger.Warning("failed to stat keyring status file:", err)
           return false
       }
   
       if stat.Mode&unix.S_IFMT != unix.S_IFREG {
           logger.Warning("keyring status file is not a regular file.")
           return false
       }
   
       buf := make([]byte, len(whiteBoxStatusEnabled))
       n, err := unix.Read(statusFd, buf)
       if err != nil || n < len(whiteBoxStatusEnabled) {
           logger.Warning("failed to read keyring status file:", err)
           return false
       }
   
       if string(buf) == whiteBoxStatusEnabled {
           logger.Info("[removeLoginKeyring] The WhiteBox keyring password has taken effect.")
           return true
       }
   
       return false
   }

2. 安全性增强:

   // 添加目录权限检查
   func isDirectorySecure(dirFd int) bool {
       var stat unix.Stat_t
       if err := unix.Fstat(dirFd, &stat); err != nil {
           logger.Warning("failed to stat directory:", err)
           return false
       }
       
       // 检查目录权限，确保只有所有者可写
       if stat.Mode&0o022 != 0 {
           logger.Warning("directory has insecure permissions")
           return false
       }
       
       return true
   }
   
   // 在 removeLoginKeyring 中使用
   func removeLoginKeyring(user *User) (err error) {
       dir := filepath.Join(user.HomeDir, keyringDir)
       dirFd, err := unix.Open(dir, unix.O_DIRECTORY|unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
       if err != nil {
           if os.IsNotExist(err) {
               return nil
           }
           return err
       }
       defer unix.Close(dirFd)
       
       if !isDirectorySecure(dirFd) {
           return fmt.Errorf("directory security check failed")
       }
       
       if !isWhiteBoxKeyringEnabled(dirFd) {
           if err := unix.Unlinkat(dirFd, loginKeyringFile, 0); err != nil && !os.IsNotExist(err) {
               return err
           }
       }
       
       return nil
   }

3. 错误处理改进:

   // 使用结构化日志
   logger.Fields{
       "user": user.Name,
       "dir": dir,
       "error": err,
   }.Warning("failed to open keyring directory")

4. 文档增强:

   // removeLoginKeyring 删除用户的登录密钥环
   // 
   // 当重置密码时，由于没有输入原密码，恢复密钥环中的数据是不可能的，
   // 因此只能直接移除密钥环文件。
   // 
   // 如果白盒密钥已生效（通过检查 status 文件内容为 "11"），
   // 则保留密钥环文件。
   //
   // 参数:
   //   user: 要操作的用户对象
   //
   // 返回值:
   //   error: 操作过程中遇到的错误，如果没有错误则为 nil
   func removeLoginKeyring(user *User) error {
       // 实现代码...
   }

总结

这段代码修改通过使用系统调用替代标准库函数，提高了代码的安全性和性能。主要改进点包括防止符号链接攻击、减少竞态条件、精确控制文件访问等。但仍有优化空间，如提取常量、增强错误处理、添加更详细的安全检查等。建议采纳上述改进建议，进一步提高代码质量、安全性和可维护性。

[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