Skip to content

fix: 调整 lastore 调用方鉴权逻辑以适配 deepin-security-loader#427

Closed
zhaohuiw42 wants to merge 1 commit into
linuxdeepin:masterfrom
zhaohuiw42:merge-v20-auth
Closed

fix: 调整 lastore 调用方鉴权逻辑以适配 deepin-security-loader#427
zhaohuiw42 wants to merge 1 commit into
linuxdeepin:masterfrom
zhaohuiw42:merge-v20-auth

Conversation

@zhaohuiw42
Copy link
Copy Markdown
Contributor

Description: 将安装、卸载和全屏升级等接口的二进制路径白名单鉴权改为允许调用者列表、lightdm 特殊 uid 与 polkit 回退校验,导出 SetAllowCaller 接口并将允许调用者保存到运行时目录,同时同步更新 D-Bus 访问规则、systemd RuntimeDirectory 和相关测试。

Log: 调整 lastore 的调用方鉴权逻辑以适配 deepin-security-loader
Task: https://pms.uniontech.com/task-view-389535.html
Influence: 控制中心、锁屏等经 deepin-security-loader 启动的服务可通过允许调用者列表访问安装、卸载和升级相关接口,lightdm greeter 可按特殊 uid 直接调用,未放行调用方仍需通过 polkit 鉴权。

Description: 将安装、卸载和全屏升级等接口的二进制路径白名单鉴权改为允许调用者列表、lightdm 特殊 uid 与 polkit 回退校验,导出 SetAllowCaller 接口并将允许调用者保存到运行时目录,同时同步更新 D-Bus 访问规则、systemd RuntimeDirectory 和相关测试。

Log: 调整 lastore 的调用方鉴权逻辑以适配 deepin-security-loader
Task: https://pms.uniontech.com/task-view-389535.html
Influence: 控制中心、锁屏等经 deepin-security-loader 启动的服务可通过允许调用者列表访问安装、卸载和升级相关接口,lightdm greeter 可按特殊 uid 直接调用,未放行调用方仍需通过 polkit 鉴权。
@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhaohuiw42

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 25, 2026

CLA Assistant Lite bot:
提交邮箱中包含我们的合作伙伴,但您似乎并非合作伙伴的成员或对接人,请联系相关对接人将您添加至组织之中,或由其重新发起 Pull Request。
The commit email domain belongs to one of our partners, but it seems you are not yet a member of the current organization, please contact the contact person to add you to the organization or let them submit the Pull Request.

You can retrigger this bot by commenting recheck in this Pull Request

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次代码变更的核心是将lastore-daemon的鉴权机制从**“基于可执行文件路径白名单”重构为“基于DBus唯一名白名单+ 特殊UID信任 + Polkit认证”**的组合鉴权模型。这是一个非常合理的安全架构演进,因为基于路径的白名单容易被替换或符号链接攻击,而基于连接的唯一名和UID更加可靠。

不过,在代码实现细节上,我发现了一些严重的安全漏洞、并发隐患以及代码质量问题。以下是详细的审查意见和改进建议:

一、 代码安全

1. 严重:SetAllowCaller 接口缺乏鉴权,可被任意进程利用提权

  • 问题:在 manager_ifc.go 中新增的 SetAllowCaller 方法没有任何权限校验。任何普通的用户进程都可以调用此接口,将自己的DBus唯一名加入白名单,从而绕过后续所有的Polkit认证检查,造成严重的权限提升漏洞。
  • 改进:必须在该方法入口处增加权限校验,至少要求调用者是root(uid == 0)或者是受信任的daemon(如deepin-app-store-daemon本身)。
    func (m *Manager) SetAllowCaller(sender dbus.Sender, uniqueName string) *dbus.Error {
        // 必须增加鉴权!只有受信任的进程才能注册白名单
        uid, err := m.service.GetConnUID(string(sender))
        if err != nil || uid != 0 {
            return dbusutil.ToError(fmt.Errorf("permission denied, only root can add allow caller"))
        }
        // ... 原有逻辑
    }
    注意:exported_methods_auto.go 中的定义也需要同步修改,将 sender 加上。

2. 严重:allowCallerStateFile 存在路径遍历与越权写入风险

  • 问题allowCallerStateFile 被硬编码为 /run/lastore/allow-callers.ini。如果该目录不存在,persistAllowCallerState 会用 0700 权限创建它。但 /run/lastore 通常属于root,如果lastore-daemon以非root运行(虽然不太可能),这会失败。更危险的是,如果该目录权限配置不当被恶意篡改,恶意程序可以提前在此路径下创建符号链接指向敏感文件(如 /etc/shadow),导致daemon覆盖写入系统文件。
  • 改进
    1. os.MkdirAll 之后,应检查 /run/lastore 的属主和权限是否符合预期。
    2. 写入文件时,建议先写入同目录下的临时文件,再通过 os.Rename 原子重命名,防止写入中途断电导致文件损坏,同时 Rename 会遵循符号链接的替换规则,相对更安全。

3. 高危:GetNameOwner 校验逻辑存在TOCTOU(检查时间到使用时间)竞态

  • 问题:在 addAllowCallerloadAllowCaller 中,使用 GetNameOwner 检查连接是否真实存在。但在检查通过后、真正将name加入内存白名单前,该DBus连接可能已经断开。此时该Unique Name会被DBus守护进程回收,并可能分配给新连接的恶意进程,导致恶意进程自动获得白名单权限。
  • 改进:这是DBus Unique Name机制的固有问题,很难完全避免。建议:
    1. 监听 NameOwnerChanged 信号,一旦白名单内的 Unique Name失去Owner,立即从 allowCallServiceList 中移除(目前代码只在 NameLost 中清理,可能不够及时)。
    2. isTrustedSender 中,除了检查 name 是否在白名单,还应该再次校验当前连接的UID是否属于合法范围(比如非root但必须是普通用户进程),增加一层防御。

二、 代码逻辑与性能

1. 并发锁粒度问题导致死锁或性能低下

  • 问题:在 checkInvokePermission 中:
    m.PropsMu.RLock()
    if !m.isTrustedSender(uid, sender) { ... }
    m.PropsMu.RUnlock()
    isTrustedSender 内部又获取了 m.PropsMu.RLock()。虽然Go的读写锁支持递归读锁,但在 addAllowCaller 中,获取了写锁后调用 persistAllowCallerState,如果持久化发生IO阻塞,会长时间持有写锁,导致所有需要鉴权的DBus调用(请求读锁)被阻塞。
  • 改进
    1. isTrustedSender 的锁交由外层统一管理,避免嵌套加锁。
    2. addAllowCaller 中,应该在获取写锁修改内存后立即释放写锁,然后再去执行耗时的文件IO操作 persistAllowCallerState。如果持久化失败,再重新获取写锁回滚内存状态。

2. DistUpgradePartly 中变量 err 重复声明与赋值混乱

  • 问题
    uid, err := m.service.GetConnUID(string(sender))
    // ...
    if !m.isTrustedSender(uid, sender) {
        err = polkit.CheckAuth(...) // 这里使用了赋值,而不是 :=,因为err已经在外层声明
    }
    这段代码虽然能编译通过,但逻辑上如果 isTrustedSender 返回 true,err 依然保留了 GetConnUID 可能返回的错误,这可能会影响后续逻辑。
  • 改进:将鉴权逻辑封装得更清晰,不要复用外层的 err 变量:
    if err := m.checkInvokePermission(sender); err != nil {
        return "/", dbusutil.ToError(err)
    }
    实际上你已经在 manager.go 实现了 checkInvokePermission,建议在所有需要鉴权的地方(如 DistUpgradePartly, PrepareDistUpgradePartly, PrepareFullScreenUpgrade)统一调用该方法,而不是把鉴权代码再复制一遍。

3. removeAllowCaller 中的切片深拷贝冗余

  • 问题
    m.PropsMu.Lock()
    newList, removed := m.allowCallServiceList.Delete(uniqueName)
    // ...
    snapshot := append(strv.Strv(nil), m.allowCallServiceList...)
    strv.Strv.Delete 通常已经返回了一个新的切片,不需要再做深拷贝作为 snapshot。
  • 改进:确认 strv 库的 Delete 行为,如果它已经做了值拷贝,直接 snapshot := newList 即可。

三、 代码质量

1. 大量重复的鉴权样板代码

  • 问题:在 InstallPackage, DistUpgradePartly, PrepareFullScreenUpgrade, PrepareDistUpgradePartly 中,你复制粘贴了几乎相同的获取UID、检查 isTrustedSender、调用 polkit.CheckAuth 的代码。
  • 改进:正如前面提到的,直接复用 m.checkInvokePermission(sender) 方法。这不仅减少代码量,也避免了前面提到的 err 变量作用域混乱问题。

2. loadAllowCallerGetNameOwner 逻辑错误

  • 问题
    owner, err := m.sysDBusDaemon.GetNameOwner(dbus.Flags(0), name)
    if err != nil { continue }
    if owner == name { validCallers = append(validCallers, name) }
    对于DBus唯一名(如 :1.123),它的 Owner 就是它自己,这个逻辑是正确的。但 GetNameOwner 通常用于获取Well-known Name(如 com.deepin.AppStore)的 Owner。如果传入的是 Unique Name,DBus底层可能并不认为它是需要解析 Owner 的名称。
  • 改进:建议通过 m.service.GetConnPID(name) 来验证该 Unique Name 对应的连接是否依然存活,如果报错说明连接已断开,直接 discard。这比 GetNameOwner 语义更清晰。

3. 遗留的清理不彻底

  • 问题
    1. common.go 中删除了 checkSenderNsMntValid,但 _initProcNsMnt_once 变量似乎在diff里也被删了,请确保没有其他地方引用。
    2. manager_ifc.goRemovePackage 方法中,删除了之前的注释掉的鉴权代码和 TODO,但没有加上新的鉴权逻辑。这意味着目前任何用户都可以无条件卸载软件包,这是非常危险的!
  • 改进:必须为 RemovePackage 加上 m.checkInvokePermission(sender) 鉴权!

总结与核心修改建议

  1. 最高优先级:修复 SetAllowCaller 的越权漏洞,增加 Sender 鉴权;为 RemovePackage 补充鉴权。
  2. 高优先级:优化 addAllowCaller 的加锁逻辑,先修改内存并释放写锁,再进行文件IO;防范 /run/lastore 的符号链接攻击。
  3. 中优先级:消除重复的鉴权代码,统一调用 checkInvokePermission;优化 loadAllowCaller 中连接存活性的检查方式。

整体而言,这次重构的方向非常正确,摒弃了不可靠的可执行路径检测,改用连接状态和UID结合的方式,只要堵住上述的鉴权漏洞和并发隐患,代码的安全性和可维护性将大幅提升。

@zhaohuiw42 zhaohuiw42 closed this May 25, 2026
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.

2 participants