Repository navigation
fix(security): move root temp files from /tmp to /run/lastore - #503
zhaohuiw42 merged 1 commit into
Conversation
Root temp files used predictable or world-writable /tmp paths. Move them to the root-only runtime dir, verify its ownership and mode before each write, reject symlinks when reading the checkpolicy cache, and drop the legacy /tmp cache fallback. Tests isolate the runtime dir and no longer call UpdateSource. Log: harden temp file paths PMS: BUG-378545 Influence: Temp files for update checks, repo config, downloads, platform reports and log masking now live in /run/lastore. Legacy /tmp caches are ignored, so the first check after upgrade re-triggers UpdateSource once. Writes are rejected when the dir is not owned by the current user or is writable by others.
Reviewer's GuideThe PR backports temp-file hardening by replacing root-owned predictable paths under Sequence diagram for hardened checkpolicy cache handlingsequenceDiagram
participant Tool as lastore-tools checkpolicy
participant Cache as checkpolicy.cache
participant Runtime as /run/lastore
participant System as system bus
Tool->>Cache: readCheckPolicyCache
Cache-->>Tool: O_NOFOLLOW and O_NONBLOCK read
Tool->>Tool: genVersionResponse
alt policy changed
Tool->>Runtime: EnsureSecureDir
Tool->>Cache: writeCheckPolicyCache
Cache-->>Tool: atomic 0600 update
Tool->>System: notify policy change
end
Flow diagram for root temporary-file hardeningflowchart TD
Start[Root operation needs temporary storage]
Validate[EnsureRuntimeDir]
Check{Directory is owned by current user and not group or world writable?}
Secure["/run/lastore"]
Fail[Abort operation]
Cleanup[Remove temporary artifact]
Start --> Validate
Validate --> Check
Check -- No --> Fail
Check -- Yes --> Secure
Secure --> Cleanup
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
CLA Assistant Lite bot: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/lastore-daemon/manager_ifc.go" line_range="637" />
<code_context>
+ logger.Warning(err)
+ return dbusutil.ToError(err)
+ }
+ tmpFile, err := os.CreateTemp(utils.RuntimeDirPath, "custom_repo_*.list")
if err != nil {
logger.Warning(err)
</code_context>
<issue_to_address>
**Upgrade options remain in /tmp**
When a fullscreen upgrade is prepared while an unprivileged local user can modify `/tmp`, `PrepareFullScreenUpgrade` still renames the option file to `/tmp/deepin_update_option.json` (`manager_check.go:195`), where an unprivileged user can replace the fixed pathname before the upgrade consumer reads it. Root can then consume attacker-controlled upgrade options, despite the new custom-repository temp file using the protected runtime directory.
Move the fullscreen-upgrade option file and its consumer to a path in the secured runtime directory, validating that directory before use.
</issue_to_address>
### Comment 2
<location path="src/internal/utils/utils.go" line_range="208" />
<code_context>
+ if !fi.IsDir() {
+ return fmt.Errorf("%s is not a directory", dir)
+ }
+ if fi.Mode().Perm()&0022 != 0 {
+ return fmt.Errorf("%s mode %#o is writable by other users", dir, fi.Mode().Perm())
+ }
</code_context>
<issue_to_address>
**Runtime files remain readable**
When the runtime directory exists with mode 0755, `EnsureSecureDir` rejects write permissions but accepts group/world read and execute permissions. Callers can then create files such as the log archive there, allowing other local users to list the directory and read those files.
Require the runtime directory to have no group or world permissions.
</issue_to_address>
### Comment 3
<location path="src/lastore-daemon/common.go" line_range="476" />
<code_context>
+ if err := utils.EnsureRuntimeDir(); err != nil {
+ return "", err
+ }
+ tmpDir, err := os.MkdirTemp(utils.RuntimeDirPath, coreListPkgName+".XXXXXX")
if err != nil {
return "", err
</code_context>
<issue_to_address>
**Core-list extractions fill runtime storage**
When core-list downloads or extractions succeed repeatedly, `downloadAndDecompressCoreList` creates an extraction directory and returns a file inside it, but `getCoreListOnline` does not remove the directory after reading or copying the file. The extracted trees accumulate in `/run/lastore` until runtime storage is consumed and other temporary-file operations fail.
Remove each extraction directory after its caller has finished using the returned file, including on error paths.
</issue_to_address>
### Comment 4
<location path="src/internal/system/apt/apt.go" line_range="288" />
<code_context>
+ if err := utils.EnsureRuntimeDir(); err != nil {
+ return "", err
+ }
+ tmpPath, err := os.MkdirTemp(utils.RuntimeDirPath, "apt-download-")
if err != nil {
return "", err
</code_context>
<issue_to_address>
**Large downloads exhaust runtime storage**
When an apt download exceeds the space available in `/run`, `DownloadPackages` places all downloaded packages under `/run/lastore`, which is typically a size-limited tmpfs. When a package set exceeds the space available there, `apt-get` fails with ENOSPC and the update or core-list download fails.
Keep security-sensitive staging in `/run/lastore`, but place potentially large package downloads in a protected disk-backed directory with safe creation and ownership checks.
</issue_to_address>
### Comment 5
<location path="src/lastore-daemon/manager_idle.go" line_range="24" />
<code_context>
+// 原路径为 /tmp/lastoreJobCache.json：/tmp 全局可写且文件名固定，本地用户可抢先
+// 创建同名符号链接/文件，诱导 root 守护进程读写其指定对象；/run/lastore 为 root 独占目录。
+// 保留为变量以便测试重定向。
+var lastoreJobCacheJson = filepath.Join(utils.RuntimeDirPath, "lastoreJobCache.json")
func (m *Manager) canAutoQuit() bool {
</code_context>
<issue_to_address>
**Saved jobs disappear after reboot**
When the daemon saves failed or paused jobs and the machine reboots before they are restored, `loadCacheJob` reads the relocated cache from `/run/lastore`, but `/run` is cleared on reboot and the old `/tmp` cache is not migrated or read, so saved failed or paused jobs are not restored after a reboot.
Store this restart/reboot recovery cache in persistent state, or migrate it to a persistent location and read it there on startup.
</issue_to_address>
### Comment 6
<location path="src/lastore-daemon/manager_logic_extra_test.go" line_range="157" />
<code_context>
func TestSaveCacheJob_EmptyJobList(t *testing.T) {
+ oldCache := lastoreJobCacheJson
+ lastoreJobCacheJson = filepath.Join(secureTempDir(t), "lastoreJobCache.json")
+ t.Cleanup(func() { lastoreJobCacheJson = oldCache })
+
</code_context>
<issue_to_address>
**Failed job omission passes**
When `saveCacheJob` writes a cache file but omits the failed job, `TestSaveCacheJob_WithFailedJob` only calls `assert.FileExists` at line 171, so an implementation that writes an empty job list instead of saving the failed job still passes.
Read and unmarshal the cache, then assert it contains the expected job ID and failed status.
</issue_to_address>
### Comment 7
<location path="src/internal/system/apt/download_extra_test.go" line_range="43" />
<code_context>
}
func TestDownloadPackagesSuccess(t *testing.T) {
+ redirectRuntimeDir(t)
prependFakeAptGet(t, 0, "")
</code_context>
<issue_to_address>
**Apt path regression passes**
When `DownloadPackages` returns a nonempty path outside `utils.RuntimeDirPath`, `TestDownloadPackagesSuccess` only checks `assert.NotEmpty(t, tmpPath)` at line 48, so a regression that creates the download directory outside `utils.RuntimeDirPath` still passes and the test misses the required relocation.
Assert that `tmpPath` exists and is a child of `utils.RuntimeDirPath`.
</issue_to_address>| logger.Warning(err) | ||
| return dbusutil.ToError(err) | ||
| } | ||
| tmpFile, err := os.CreateTemp(utils.RuntimeDirPath, "custom_repo_*.list") |
There was a problem hiding this comment.
🟠 High · Upgrade options remain in /tmp
When a fullscreen upgrade is prepared while an unprivileged local user can modify /tmp, PrepareFullScreenUpgrade still renames the option file to /tmp/deepin_update_option.json (manager_check.go:195), where an unprivileged user can replace the fixed pathname before the upgrade consumer reads it. Root can then consume attacker-controlled upgrade options, despite the new custom-repository temp file using the protected runtime directory.
Move the fullscreen-upgrade option file and its consumer to a path in the secured runtime directory, validating that directory before use.
Prompt for AI agents
In `src/lastore-daemon/manager_ifc.go` at line 637:
**Upgrade options remain in /tmp**
When a fullscreen upgrade is prepared while an unprivileged local user can modify `/tmp`, `PrepareFullScreenUpgrade` still renames the option file to `/tmp/deepin_update_option.json` (`manager_check.go:195`), where an unprivileged user can replace the fixed pathname before the upgrade consumer reads it. Root can then consume attacker-controlled upgrade options, despite the new custom-repository temp file using the protected runtime directory.
Move the fullscreen-upgrade option file and its consumer to a path in the secured runtime directory, validating that directory before use.| if !fi.IsDir() { | ||
| return fmt.Errorf("%s is not a directory", dir) | ||
| } | ||
| if fi.Mode().Perm()&0022 != 0 { |
There was a problem hiding this comment.
🟡 Medium · Runtime files remain readable
When the runtime directory exists with mode 0755, EnsureSecureDir rejects write permissions but accepts group/world read and execute permissions. Callers can then create files such as the log archive there, allowing other local users to list the directory and read those files.
Require the runtime directory to have no group or world permissions.
Prompt for AI agents
In `src/internal/utils/utils.go` at line 208:
**Runtime files remain readable**
When the runtime directory exists with mode 0755, `EnsureSecureDir` rejects write permissions but accepts group/world read and execute permissions. Callers can then create files such as the log archive there, allowing other local users to list the directory and read those files.
Require the runtime directory to have no group or world permissions.| if err := utils.EnsureRuntimeDir(); err != nil { | ||
| return "", err | ||
| } | ||
| tmpDir, err := os.MkdirTemp(utils.RuntimeDirPath, coreListPkgName+".XXXXXX") |
There was a problem hiding this comment.
🟡 Medium · Core-list extractions fill runtime storage
When core-list downloads or extractions succeed repeatedly, downloadAndDecompressCoreList creates an extraction directory and returns a file inside it, but getCoreListOnline does not remove the directory after reading or copying the file. The extracted trees accumulate in /run/lastore until runtime storage is consumed and other temporary-file operations fail.
Remove each extraction directory after its caller has finished using the returned file, including on error paths.
Prompt for AI agents
In `src/lastore-daemon/common.go` at line 476:
**Core-list extractions fill runtime storage**
When core-list downloads or extractions succeed repeatedly, `downloadAndDecompressCoreList` creates an extraction directory and returns a file inside it, but `getCoreListOnline` does not remove the directory after reading or copying the file. The extracted trees accumulate in `/run/lastore` until runtime storage is consumed and other temporary-file operations fail.
Remove each extraction directory after its caller has finished using the returned file, including on error paths.| if err := utils.EnsureRuntimeDir(); err != nil { | ||
| return "", err | ||
| } | ||
| tmpPath, err := os.MkdirTemp(utils.RuntimeDirPath, "apt-download-") |
There was a problem hiding this comment.
🟡 Medium · Large downloads exhaust runtime storage
When an apt download exceeds the space available in /run, DownloadPackages places all downloaded packages under /run/lastore, which is typically a size-limited tmpfs. When a package set exceeds the space available there, apt-get fails with ENOSPC and the update or core-list download fails.
Keep security-sensitive staging in /run/lastore, but place potentially large package downloads in a protected disk-backed directory with safe creation and ownership checks.
Prompt for AI agents
In `src/internal/system/apt/apt.go` at line 288:
**Large downloads exhaust runtime storage**
When an apt download exceeds the space available in `/run`, `DownloadPackages` places all downloaded packages under `/run/lastore`, which is typically a size-limited tmpfs. When a package set exceeds the space available there, `apt-get` fails with ENOSPC and the update or core-list download fails.
Keep security-sensitive staging in `/run/lastore`, but place potentially large package downloads in a protected disk-backed directory with safe creation and ownership checks.| // 原路径为 /tmp/lastoreJobCache.json：/tmp 全局可写且文件名固定，本地用户可抢先 | ||
| // 创建同名符号链接/文件，诱导 root 守护进程读写其指定对象；/run/lastore 为 root 独占目录。 | ||
| // 保留为变量以便测试重定向。 | ||
| var lastoreJobCacheJson = filepath.Join(utils.RuntimeDirPath, "lastoreJobCache.json") |
There was a problem hiding this comment.
🟡 Medium · Saved jobs disappear after reboot
When the daemon saves failed or paused jobs and the machine reboots before they are restored, loadCacheJob reads the relocated cache from /run/lastore, but /run is cleared on reboot and the old /tmp cache is not migrated or read, so saved failed or paused jobs are not restored after a reboot.
Store this restart/reboot recovery cache in persistent state, or migrate it to a persistent location and read it there on startup.
Prompt for AI agents
In `src/lastore-daemon/manager_idle.go` at line 24:
**Saved jobs disappear after reboot**
When the daemon saves failed or paused jobs and the machine reboots before they are restored, `loadCacheJob` reads the relocated cache from `/run/lastore`, but `/run` is cleared on reboot and the old `/tmp` cache is not migrated or read, so saved failed or paused jobs are not restored after a reboot.
Store this restart/reboot recovery cache in persistent state, or migrate it to a persistent location and read it there on startup.|
|
||
| func TestSaveCacheJob_WithFailedJob(t *testing.T) { | ||
| oldCache := lastoreJobCacheJson | ||
| lastoreJobCacheJson = filepath.Join(secureTempDir(t), "lastoreJobCache.json") |
There was a problem hiding this comment.
🟡 Medium · Failed job omission passes
When saveCacheJob writes a cache file but omits the failed job, TestSaveCacheJob_WithFailedJob only calls assert.FileExists at line 171, so an implementation that writes an empty job list instead of saving the failed job still passes.
Read and unmarshal the cache, then assert it contains the expected job ID and failed status.
Prompt for AI agents
In `src/lastore-daemon/manager_logic_extra_test.go` at line 157:
**Failed job omission passes**
When `saveCacheJob` writes a cache file but omits the failed job, `TestSaveCacheJob_WithFailedJob` only calls `assert.FileExists` at line 171, so an implementation that writes an empty job list instead of saving the failed job still passes.
Read and unmarshal the cache, then assert it contains the expected job ID and failed status.| } | ||
|
|
||
| func TestDownloadPackagesSuccess(t *testing.T) { | ||
| redirectRuntimeDir(t) |
There was a problem hiding this comment.
🟡 Medium · Apt path regression passes
When DownloadPackages returns a nonempty path outside utils.RuntimeDirPath, TestDownloadPackagesSuccess only checks assert.NotEmpty(t, tmpPath) at line 48, so a regression that creates the download directory outside utils.RuntimeDirPath still passes and the test misses the required relocation.
Assert that tmpPath exists and is a child of utils.RuntimeDirPath.
Prompt for AI agents
In `src/internal/system/apt/download_extra_test.go` at line 43:
**Apt path regression passes**
When `DownloadPackages` returns a nonempty path outside `utils.RuntimeDirPath`, `TestDownloadPackagesSuccess` only checks `assert.NotEmpty(t, tmpPath)` at line 48, so a regression that creates the download directory outside `utils.RuntimeDirPath` still passes and the test misses the required relocation.
Assert that `tmpPath` exists and is a child of `utils.RuntimeDirPath`.
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 语法正确，逻辑清晰 ✅ 通过 潜在问题： 建议： [] 2. 代码质量 ✅评价： 代码结构清晰，注释完整 ✅ 通过 潜在问题： 建议： [] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： [] 4. 代码安全 🔒评价： 存在0个安全漏洞 ✅ 通过
安全漏洞详情： 建议： ['建议后续考虑将 WriteFileSecurely 内部的 MkdirAll 权限从 0755 收紧为 0700， 使其与 EnsureSecureDir 的安全策略一致，减少对调用方先调用 EnsureSecureDir 的隐式依赖。'] 💡 改进建议代码示例// 当前代码已正确实现安全修复，无需额外修改。
// 以下为 WriteFileSecurely 内部 MkdirAll 权限收紧的改进建议：
//
// func WriteFileSecurely(path string, content []byte, perm os.FileMode) error {
// dir := filepath.Dir(path)
// if err := os.MkdirAll(dir, 0700); err != nil { // 从 0755 收紧为 0700
// return err
// }
// // ... 其余逻辑不变
// }本报告由 AI 代码审查工具自动生成 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qiuzhiqian, zhaohuiw42 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary
Backport the 5.6.21 -> 5.6.22 temp-file hardening to master.
/tmpto the root-only/run/lastoreruntime dir: checkpolicy cache, job cache, apt download dir, combined source dirs, coreList extraction, custom repo list, InRelease staging, platform report xz/tar, and masked logs.utils.RuntimeDirPath,EnsureSecureDirandEnsureRuntimeDir; verify directory ownership and mode before writing.O_NOFOLLOW|O_NONBLOCKand write it atomically with mode 0600./tmp/platform_cache.jsonand/tmp/classified_cache.jsonfallbacks.readCacheWithLegacyFallbackhelper and its tests./run/lastore; the checkpolicy smoke test no longer callsUpdateSource.Verification
go build ./...passes.go test ./...passes for all packages.PMS: BUG-378545
Summary by Sourcery
Harden root temporary-file handling by replacing predictable
/tmppaths with validated, root-only runtime storage.Bug Fixes:
/tmpto the secure/run/lastoreruntime directory./tmpplatform and classified cache fallbacks.Enhancements:
Tests:
Chores: