Skip to content

fix(auth): unify caller authentication to isTrustedSender and remove binary path identification#428

Open
Fire-dtx wants to merge 1 commit into
linuxdeepin:masterfrom
Fire-dtx:master
Open

fix(auth): unify caller authentication to isTrustedSender and remove binary path identification#428
Fire-dtx wants to merge 1 commit into
linuxdeepin:masterfrom
Fire-dtx:master

Conversation

@Fire-dtx
Copy link
Copy Markdown
Contributor

Replace binary path-based caller identification (getExecutablePathAndCmdline, mapMethodCaller, checkInvokePermission) with isTrustedSender + polkit authentication for DistUpgradePartly and PrepareDistUpgradePartly interfaces. Remove caller authentication from RemovePackage interface. Add appstore_intranet.list to trusted source list. Remove deprecated deny-exec-whitelist and install-package-support-auth config items.

Introduce manager_auth.go with allow-caller registration, lightdm trusted UID support, and persistent runtime state under /run/lastore. Export SetAllowCaller D-Bus method for deepin-security-loader integration. Add D-Bus access rules for SetAllowCaller deny and deepin-daemon group policy. Configure RuntimeDirectory with 0700 mode and preserve semantics.

…binary path identification

Replace binary path-based caller identification (getExecutablePathAndCmdline,
mapMethodCaller, checkInvokePermission) with isTrustedSender + polkit
authentication for DistUpgradePartly and PrepareDistUpgradePartly interfaces.
Remove caller authentication from RemovePackage interface. Add
appstore_intranet.list to trusted source list. Remove deprecated
deny-exec-whitelist and install-package-support-auth config items.

Introduce manager_auth.go with allow-caller registration, lightdm trusted
UID support, and persistent runtime state under /run/lastore. Export
SetAllowCaller D-Bus method for deepin-security-loader integration.
Add D-Bus access rules for SetAllowCaller deny and deepin-daemon group
policy. Configure RuntimeDirectory with 0700 mode and preserve semantics.
@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Fire-dtx

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

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这份 Git Diff 对 lastore-daemon 的权限验证机制进行了重大重构,将原本基于硬编码可执行文件路径的白名单机制,替换为了基于 D-Bus 唯一名、特殊 UID 以及 Polkit 组合的动态鉴权机制。同时,对 systemd 服务配置和 D-Bus 权限策略也进行了相应的收紧。

整体来看,该重构显著提升了系统的安全性和架构的合理性,但也引入了一些潜在的并发、一致性和边界场景问题。以下是详细的审查意见:

一、 语法与逻辑

  1. 并发锁的粒度与死锁风险 (manager.go & manager_auth.go)

    • checkInvokePermission 中,使用了 m.PropsMu.RLock(),并在内部调用了 m.isTrustedSender。而 isTrustedSender 内部又获取了一次 m.PropsMu.RLock()。虽然 Go 的 sync.RWMutex 攻击读锁重入是安全的,但这种嵌套读锁是不必要的,增加了代码维护的心智负担,建议直接在外层加锁,内层不再重复加锁。
    • 严重问题:在 checkInvokePermission 中,如果 polkit.CheckAuth 返回错误,代码在 return 前执行了 m.PropsMu.RUnlock(),但如果 polkit.CheckAuth 阻塞等待用户输入,此时读锁会被长时间持有。这会导致所有需要写锁(如 addAllowCaller 中的 m.PropsMu.Lock())的 Goroutine 被永久阻塞。建议在调用 Polkit 等阻塞/耗时操作前,必须释放读锁。
  2. PrepareFullScreenUpgrade 逻辑重构的回退机制 (manager_ifc.go)

    • 原代码使用 for {}break,重构后使用了 terminate 闭包。逻辑更清晰,但原逻辑中,只要 for 循环内任何一步出错 break,都会执行 RestartUnit
    • 新逻辑中:err = terminate(),如果 err != nil 则执行 RestartUnit但是,原逻辑中 seat.Terminate(0) 成功时返回 nil,此时不执行 RestartUnit;如果 seat.Terminate(0) 失败,原逻辑会 break 并执行 RestartUnit。新逻辑与原逻辑保持了一致,这部分是正确的。
    • 注意:新逻辑中,如果 terminate() 返回 nil(即成功 Terminate 了 seat),代码会跳过 RestartUnit,但随后的 logger.Info("RestartUnit display-manager") 依然会被执行(虽然不会真 Restart,但日志会产生误导)。建议将此日志移入 if err != nil 分支内。
  3. DistUpgradePartly 等方法的重复鉴权代码

    • DistUpgradePartlyPrepareDistUpgradePartlyInstallPackage 中出现了完全相同的鉴权逻辑代码(获取 UID -> isTrustedSender -> Polkit)。
    • 建议:既然已经将 checkInvokePermission 重构为方法,建议在这些业务方法中直接调用 m.checkInvokePermission(sender),以减少冗余代码,保持鉴权逻辑的一致性。目前 InstallPackage 的鉴权错误信息还保留了硬编码的路径提示,与新的鉴权模型不符。

二、 代码质量

  1. 废弃代码与注释的清理

    • common.go 中删除了 allowInstallPackageExecPaths 等变量,但 main.go 中的注释 // 安装/卸载接口不再追加可执行路径白名单,改由 allow-caller、特殊 uid 和 polkit 共同鉴权。 显得有些多余,建议直接删除该行注释,保持代码整洁。
    • manager_ifc.goRemovePackage 方法里删除了被注释掉的大段旧代码,这是很好的实践,但请确保 TODO 提到的需求(鉴权或给 launcher 加 loader)在新机制下已经得到解决(目前看 RemovePackage 似乎完全没有任何鉴权了,这是一个高风险点,见下文安全部分)。
  2. 测试用例的版权声明 (manager_upgrade_test.go)

    • 新增的测试文件头注释为 // SPDX-FileCopyrightText: 2026 UnionTech Software Technology Co., Ltd.,年份为 2026,这显然是一个笔误,应改为当前年份(如 2023 或 2024)。
  3. ioutil.WriteFile 的弃用 (appinfo_test.go)

    • 修改中使用了 ioutil.WriteFile,在 Go 1.16 及以上版本中,ioutil 整个包已被标记为废弃,建议改为 os.WriteFile

三、 代码性能

  1. isTrustedSender 的查询复杂度

    • m.allowCallServiceList.Contains(string(sender)) 底层是 strv.Strv,本质是 slice,Contains 操作是 $O(N)$ 线性遍历。
    • 如果 allowCallServiceList 长度通常很小(例如几十个以内),这完全没问题。但如果预期会非常长,建议将 allowCallServiceList 的类型从 strv.Strv 改为 map[string]struct{},以实现 $O(1)$ 的查询性能。
  2. 文件 I/O 与锁的交互 (persistAllowCallerState)

    • addAllowCallerremoveAllowCaller 中,都是在持有 m.PropsMu 修改完内存状态后,再调用 m.persistAllowCallerState(snapshot) 写入磁盘。
    • 写文件是相对耗时的 I/O 操作,在此期间持有写锁(或如当前代码在 addAllowCaller 中释放了写锁后调用,但 removeAllowCaller 并未释放读/写锁直接调用),会阻塞其他并发 D-Bus 请求。建议将文件 I/O 操作完全移到锁外部执行。

四、 代码安全

  1. 高危:RemovePackage 接口鉴权缺失

    • manager_ifc.go 中,RemovePackage 方法删除了原有的鉴权代码,但并未InstallPackage 那样添加新的 isTrustedSender 或 Polkit 鉴权。这意味着任何能连上 System Bus 的用户都可以无条件卸载系统软件包,这是极其危险的。
    • 必须修复:为 RemovePackage 添加与 InstallPackage 同等的鉴权逻辑。
  2. SetAllowCaller 的提权风险与 D-Bus 策略配置

    • 新增的 SetAllowCaller 接口允许将某个 D-Bus 连接加入白名单,从而绕过 Polkit 鉴权。这是一个高权限接口。
    • org.deepin.dde.Lastore1.conf 中,默认策略 <policy context="default"> 拒绝了普通用户调用此接口,这是正确的。
    • 但是<policy group="deepin-daemon"> 被赋予了完全的 <allow> 权限。这意味着属于 deepin-daemon 组的进程可以调用 SetAllowCaller任何连接加入白名单。
    • addAllowCaller 中,只校验了 owner == uniqueName(即该连接真实存在),但没有校验请求者(sender)的身份。如果 deepin-daemon 组下的某个服务被劫持,攻击者可以通过它将恶意进程的 UniqueName 注册为 AllowCaller,从而实现提权。
    • 建议:在 SetAllowCaller 的实现中,除了验证 uniqueName 存在,还应验证调用 SetAllowCallersender 本身是否是受信任的(例如必须是 root 或特定的 service name),或者 D-Bus 配置进一步收紧,只允许特定的 well-known name 调用此接口。
  3. RuntimeDirectoryMode=0700 与文件权限

    • systemd 配置中将 RuntimeDirectoryMode0750 改为 0700,增强了安全性,防止同组用户读取 /run/lastore/ 下的内容。
    • 但在 persistAllowCallerState 中,创建目录使用的是 os.MkdirAll(filepath.Dir(allowCallerStateFile), 0700),这与 systemd 的 0700 保持了一致,很好。
    • 注意kf.SaveToFile 保存文件时的默认权限需要确认。如果 go-lib/keyfile 默认以 0644 保存,虽然父目录是 0700 阻止了其他用户进入目录,但最好确保文件本身的权限也是 0600,以防有其他途径访问。
  4. lightdm 用户的信任边界

    • 代码中将 lightdm 用户(UID 通过动态查找)加入了 trustedCallerUIDs,这意味着 lightdm 进程可以无条件调用高权限接口(如关机、更新等)。
    • 注释说明是为了兼容 greeter 场景。请确保 lightdm 进程及其加载的 greeter 插件是绝对安全的,因为它们现在拥有了等同于 root 的调用权限而无需 Polkit 弹窗。

改进建议代码示例

针对 checkInvokePermission 的锁问题、RemovePackage 鉴权缺失、以及冗余代码问题,提供以下修改参考:

1. 修复 checkInvokePermission 的锁与阻塞问题 (manager.go)

func (m *Manager) checkInvokePermission(sender dbus.Sender) error {
	uid, err := m.service.GetConnUID(string(sender))
	if err != nil {
		return fmt.Errorf("failed to get sender conn uid:%v", err)
	}
	
	// 必须在调用可能阻塞的 Polkit 前释放锁
	isTrusted := func() bool {
		m.PropsMu.RLock()
		defer m.PropsMu.RUnlock()
		return m.isTrustedSender(uid, sender)
	}()

	if !isTrusted {
		err = polkit.CheckAuth(polkitActionChangeOwnData, string(sender), nil)
		if err != nil {
			logger.Warning(err)
			return dbusutil.ToError(err)
		}
	}
	return nil
}

// isTrustedSender 内部不再加锁,由调用方控制
func (m *Manager) isTrustedSender(uid uint32, sender dbus.Sender) bool {
	if uid == 0 {
		return true
	}
	if _, ok := m.trustedCallerUIDs[uid]; ok {
		return true
	}
	return m.allowCallServiceList.Contains(string(sender))
}

2. 为 RemovePackage 补充鉴权 (manager_ifc.go)

func (m *Manager) RemovePackage(sender dbus.Sender, jobName string, packages string) (job dbus.ObjectPath, busErr *dbus.Error) {
	m.service.DelayAutoQuit()
	
	// 必须补充鉴权!
	if err := m.checkInvokePermission(sender); err != nil {
		return "/", dbusutil.ToError(err)
	}

	jobObj, err := m.removePackage(sender, jobName, packages)
	if err != nil {
		return "/", dbusutil.ToError(err)
	}
	return jobObj.getPath(), nil
}

3. 简化 InstallPackage 等方法的冗余鉴权代码 (manager_ifc.go)

func (m *Manager) InstallPackage(sender dbus.Sender, jobName string, packages string) (job dbus.ObjectPath, busErr *dbus.Error) {
	m.service.DelayAutoQuit()

	// 直接使用统一的鉴权入口
	if err := m.checkInvokePermission(sender); err != nil {
		return "/", dbusutil.ToError(err)
	}

	jobObj, err := m.installPackage(sender, jobName, packages)
	if err != nil {
		return "/", dbusutil.ToError(err)
	}
	return jobObj.getPath(), nil
}

4. 修复 PrepareFullScreenUpgrade 的误导性日志

    err = terminate()
    if err != nil {
        // 如果上述方法出错,需要采用重启lightdm方案,此时所有图形session也都会退出
        _, err = m.systemd.RestartUnit(0, "display-manager.service", "replace")
        if err != nil {
            logger.Warning(err)
            return dbusutil.ToError(err)
        }
        // 将日志移入条件分支内
        logger.Info("RestartUnit display-manager")
    }
    return nil

# StateDirectory=lastore is not set because it would conflict with the ownership of smartmirror-daemon and build-system-info services, which need to be owned by deepin-daemon, and enabling this would set the owner to root.

BusName=org.deepin.dde.Lastore1
CacheDirectory=lastore
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

无关改动


// execPath和cmdLine可以有一个为空,其中一个存在即可作为判断调用者的依据
// getExecutablePathAndCmdline 获取调用者进程的可执行路径和命令行,供 PowerOff 等接口鉴权使用。
func getExecutablePathAndCmdline(service *dbusutil.Service, sender dbus.Sender) (string, string, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

函数可以删除

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