Skip to content

修复console任意文件读取漏洞#362

Merged
shigma merged 1 commit intokoishijs:mainfrom
Potatowo233:security-fix
Nov 17, 2025
Merged

修复console任意文件读取漏洞#362
shigma merged 1 commit intokoishijs:mainfrom
Potatowo233:security-fix

Conversation

@Potatowo233
Copy link
Contributor

原代码中如果路径名以@plugin-开头则不对路径做任何校验,不是js文件的话(如koishi.yml)能直接sendFile(filename)返回
image
koishi的后台账号密码完全明文写在koishi.yml中,因此一旦读取到能够直接进入后台进行更加高危的操作比如任意代码执行
image
服务端代码执行示例(无需额外安装任何插件):
f7e31ede932851404f8f02baec1067be
修复方案对插件文件先目录规范化后去除所有./和../,再判断目录位置是否合法

@Potatowo233 Potatowo233 reopened this Nov 16, 2025
@Potatowo233 Potatowo233 marked this pull request as ready for review November 16, 2025 17:45
@Potatowo233 Potatowo233 reopened this Nov 17, 2025
@ilharp
Copy link
Member

ilharp commented Nov 17, 2025

已在 Koishi v4.18.9 + plugin-console v5.30.10 上复现。目测此漏洞危害极大、影响极大,需要尽快修复。

ping @shigma

@ilharp
Copy link
Member

ilharp commented Nov 17, 2025

Closed by mistake. Reopening

@ilharp ilharp reopened this Nov 17, 2025
@shigma shigma merged commit 896882b into koishijs:main Nov 17, 2025
0 of 6 checks passed
@DGCK81LNN
Copy link

DGCK81LNN commented Nov 18, 2025

@shigma 我对此修复有疑问。这里的逻辑是 filename = resolve(this.root, files[0] + name.slice(8 + key.length)),其中 files 是插件开发者定的路径,name 是请求 URL 中出现的、相对于 files[0] 的路径。只要 name 不包含 .. 段不就是安全的吗?为什么还要用 !filename.startsWith(this.root) && !filename.includes('node_modules') 作判断标准?以下是我的修复:

const relativePath = name.slice(8 + key.length)
if (relativePath.split(/[/\\]/g).includes("..")) {
  return ctx.status = 403
}
const filename = files[0] + relativePath

控制台插件开发者可能引用绝对路径不包含“node_modules”的 JavaScript 文件,我认为这本身没有问题,而在本 PR 的修改后这些控制台插件加载时都会返回 403 错误。而本 PR 的修改并没有阻止访问者读取任何绝对路径包含“node_modules”的文件,我认为这是不妥的。

@shigma
Copy link
Member

shigma commented Nov 19, 2025

@DGCK81LNN

从结果来看,这个文件的代码确实可能并不是最优的,但这并不意味着这个 PR 有问题。此文件的另一处位置已经包含了类似的逻辑(判断 startsWith + includes),因此这个 PR 符合最小改动原则。此外,从安全性的角度,对路径拼接的结果进行判断总是比判断片段更安全。

关于 node_modules 的问题,确实可以另做讨论,欢迎新的 issue 和 PR。

ilharp added a commit to ilharp/littleil that referenced this pull request Feb 5, 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.

4 participants