Pull Request已成功合入, 合并人@linhan
(感谢 写代码啥时候能换包餐巾纸啊 的贡献)代码评审:3637136e fix(security): require trusted verified model downloads
| 项 | 内容 |
|---|---|
| Commit | 3637136e1109353d684b31b8209de3f331cd6a71 |
| 作者 | pl |
| 日期 | 2026-08-11 11:19:20 +0800 |
| 规模 | 14 个文件,+471 / -55 |
| 评审方式 | 源码、构建配置、模型配置静态核查(未执行构建与测试) |
变更概述
将模型下载策略从"可选校验"改为"强制校验":
- 下载 URL 必须为 HTTPS(含全部重定向),SHA-256 checksum 必须存在且为 64 位十六进制。
- 新增
model_util.cpp中的 URL / checksum 校验函数族,并在ModelDownloader、ModelFileManager、SDKModelLifecycleService三层分别设卡。 - 新增 CMake 选项
SMARTSERVE_ENABLE_TEST_FILE_URLS(默认OFF),仅在开启时允许file+test://本地下载,供隔离测试使用。 ModelDownloader构造函数移除expected_size/expected_checksum的默认参数。
结论
安全加固方向正确,纵深防御设计到位。存在 1 个构建阻断问题和 1 个测试覆盖问题需在合并前处理,其余为可读性与测试健壮性改进。
关键问题
1. model_util.cpp 新增 libcurl 依赖,OpenHarmony GN 目标缺少该依赖(构建阻断)
model_util.cpp 现在包含 <curl/curl.h> 并调用 curl_url()、curl_url_set()、curl_url_get()、curl_url_cleanup()。
该文件被编入 OpenHarmony 的 ohos_shared_library("lms")(services/src/server/BUILD.gn:67),而该目标的依赖声明为:
deps = [
]
external_deps = [
"c_utils:utils",
"hilog:libhilog",
"json:nlohmann_json_static",
]
没有任何 curl 依赖。全仓库搜索所有 BUILD.gn 与 .gni 均未发现 curl 声明。
model_downloader.cpp 不受影响,因为它未被列入 GN 的 sources,仅由 CMake 构建。
影响:OpenHarmony 系统服务构建会在编译 model_util.cpp 时因找不到 curl/curl.h 失败。违反 AGENTS.md「兼顾双构建系统」要求。
建议:给 lms 目标补充 curl 的 external_deps;或将 URL 校验逻辑拆分到仅由 CMake 编译的独立文件,保持 model_util.cpp 无第三方依赖。
验证限制:未实际执行 GN 构建(需完整 OpenHarmony 源码树),结论基于依赖声明的静态判断。
2. 默认构建下 19 处测试被静默跳过(测试覆盖丢失)
SMARTSERVE_ENABLE_TEST_FILE_URLS 默认为 OFF,且 scripts/build_and_test.sh 从不传递该选项(已核对脚本中全部 -D 参数)。
因此以下 19 处 GTEST_SKIP 在默认回归中全部生效:
| 文件 | 跳过处数 |
|---|---|
test/internal/model_downloader_test.cpp |
6 |
test/api/gewu_smartserve_model_lifecycle_api_test.cpp |
7 |
test/api/gewu_smartserve_model_files_api_test.cpp |
4 |
| 合计(含重复统计的定义处) | 19 |
丢失覆盖的核心场景包括:多文件下载、暂停与恢复、并发下载幂等性、删除与下载竞态、缓存文件重校验。
影响:与 AGENTS.md「前置条件不满足或检查失败时应返回非零状态,不得静默跳过」冲突。本次加固的主要行为在默认回归中未被验证。
建议:让 build_and_test.sh 默认开启该选项。它只影响测试构建,且 CMakeLists.txt:76 已强制它必须与 SMARTSERVE_BUILD_TESTS=ON 同时开启,风险可控。否则这些用例等同于被删除。
次要问题
3. IsAllowedDownloadScheme 的 https 分支是死代码
services/src/server/model/model_downloader.cpp:121 定义的函数,唯一调用点在同文件 200 行:
const bool testFileUrl = GetUrlScheme(url_) == "file" && IsAllowedDownloadScheme("file");
实参恒为 "file",scheme == "https" 永不成立。该函数实质只是 SMARTSERVE_ENABLE_TEST_FILE_URLS 宏的包装。
不影响正确性,但函数签名暗示它是通用 scheme 白名单,易造成误读。
4. 部分测试的 checksum 与被测语义不匹配
test/internal/model_downloader_test.cpp:406 在 ModelFileManagerEmitsFinalProgressForAlreadyCompleteFile 中传入 CalculateSHA256(dest.string()),即目标文件自身的哈希而非源文件哈希。这使"已完成文件"判定恒真,用例无论下载逻辑正确与否都会通过。
test/api/gewu_smartserve_model_files_api_test.cpp:293 给 URL 为 https://example.com/model.bin 的配置传入 Sha256(sourcePath),checksum 与 URL 指向内容无关,仅为通过新增的格式校验。
这些用例仍能验证各自目标(进度上报、路径穿越拒绝),但 checksum 参数已退化为占位符。建议补充注释说明,避免后续维护者误读为真实校验。
5. DownloadAcceptsCustomHttpsAndRejectsInsecureSources 的成功路径较脆弱
test/api/gewu_smartserve_model_files_api_test.cpp:187 期望 https://custom.example/model.bin 返回 GEWU_SMARTSERVE_OK。
它能通过是因为 modelDir/model.bin 已预置且 checksum 匹配,client.IsModelReady 在 sdk/src/model_lifecycle_service.cpp:506 短路返回,从不发起真实请求。
逻辑正确,但依赖 name 字段恰好解析到该预置文件路径。若 SingleFileStorageSubPath 实现变化,用例会转为尝试访问 custom.example 并超时,而非给出清晰失败信号。
6. IsModelReady 的空 checksum 语义未收紧
interfaces/innerkits/src/smart_serve_client_non_ohos.cpp:344 仍保留:
if (checksum.empty()) {
return true;
}
本次在 SDK 层 4 个调用点(model_lifecycle_service.cpp:322、:340、:436、:506)加了 IsValidSha256Checksum 前置校验来弥补。但 applications/chatbox_cli/chatbox.cpp:345 与 :352 直接传入 file.checksum / config.checksum,无此保护,chatbox 仍会把无 checksum 的模型判定为 ready。
不构成新漏洞(行为与变更前一致),但加固不完整,且防护逻辑分散在调用方而非集中于 IsModelReady 内部。
已核查并排除的疑点
config/model_config/models.json 的兼容性
逐模型核查全部可下载条目的 checksum 完整性,结果:多文件模型的每个 files[] 条目均带合法 64 位 checksum;单文件模型均带 checksum(部分含 sha256: 前缀,NormalizeChecksum 已正确剥离)。
唯一无 checksum 的 applefm 同时也没有 base_url,走 sdk/src/model_lifecycle_service.cpp:492 的 INVALID_ARGUMENT 分支,该路径在变更前后行为一致。
结论:内置模型配置不因此变更产生下载回归。
ModelDownloader 构造函数签名变更的调用方
移除默认参数后,全部调用点均已显式传参:
| 调用点 | 状态 |
|---|---|
interfaces/innerkits/src/smart_serve_client_non_ohos.cpp:386 |
已传 0, checksum |
services/src/server/model/model_file_manager.cpp:641 |
已传 expectedSize, checksum |
test/internal/model_downloader_test.cpp:189, 211, 242, 288 |
已传 |
结论:无遗漏调用点。
设计上做得好的部分
URL 校验采用纵深防御。IsSecureModelDownloadUrl(model_util.cpp:59)先做字符串层面前置检查(scheme、authority 非空、无控制字符与空白),再交由 curl_url() 规范化解析并显式拒绝 userinfo,避免了手写 URL parser 的常见陷阱。
协议限制与事后复核并行。在 curl 层用 CURLOPT_PROTOCOLS_STR / CURLOPT_REDIR_PROTOCOLS_STR 限制协议,并在 curl_easy_perform 后用 CURLINFO_EFFECTIVE_URL 复核最终 URL(model_downloader.cpp:441)。即使 curl 的重定向限制被绕过也有兜底,且校验失败时删除已落盘文件。
libcurl 版本分支正确。LIBCURL_VERSION_NUM >= 0x075500 对应 7.85.0,正是 CURLOPT_PROTOCOLS_STR 的引入版本,旧版本回退到 CURLPROTO_* 常量。
移除默认参数是好选择。model_downloader.h:69 强制所有调用点显式传 checksum,编译期即可发现遗漏,优于运行时校验。
处理优先级
| 优先级 | 问题 | 说明 |
|---|---|---|
| 合并前必须 | 问题 1(GN 构建) | OpenHarmony 目标无法编译 |
| 合并前建议 | 问题 2(测试跳过) | 决定加固是否有实际覆盖 |
| 后续跟进 | 问题 3–6 | 可读性与测试健壮性 |
未执行的验证
- CMake 构建与 CTest:需初始化 submodule 且耗时较长。
- OpenHarmony GN 构建:需完整 OpenHarmony 源码树。
- 真实网络下载路径:需模型环境。
以上结论均基于源码、构建配置与模型配置的静态核查。


新增选项 SMARTSERVE_ENABLE_TEST_FILE_URLS 默认关闭
原有下载、暂停恢复、并发、删除及元数据测试现在通过 GTEST_SKIP() 跳过
项目标准入口 build_and_test.sh 没有启用该选项,因此默认回归会成功退出,却不再覆盖核心下载成功路径。
此次提交共给 17 个测试增加了该跳过条件。
新测试传输开关没有接入标准测试入口,也没有记录启用方式,导致默认回归静默跳过下载成功、恢复、并发和删除路径。
建议提供专用的安全测试目标或测试 transport 注入,使标准 CI 能运行这些用例,同时不让生产库支持本地文件 URL。


model_lifecycle_service.cpp:354-358 分支不可达(前面 310/337 行已穷尽 base_url 非空情形),属死代码


model_downloader.cpp:445:重定向拒绝路径用裸 std::remove(忽略错误),同函数其余删除均走带错误处理的 RemoveExistingFile,建议统一。


changed this line on 4a970f34 view diff detail
同理,model_downloader.cpp:473:重定向拒绝路径用裸 std::remove(忽略错误),建议统一。


changed this line on 4a970f34 view diff detail
interfaces/innerkits/include/smart_serve_client.h:43-53、sdk/include/smartserve/smartserve.h:64
SmartServeClient::DownloadModel保留checksum = ""默认参数,但新策略下空 checksum 必然下载失败,默认参数成为「必然失败」陷阱;注释也未提 SHA-256 必填、仅 HTTPS。
老 APISmartServeDownloadModel与GewuSmartServeDownloadModel走同一实现,但只有gewu_smartserve/core.h更新了文档。
建议:同步三处公共头注释,并移除 innerkits DownloadModel 的 checksum 默认参数编译期强制调用方提供)。


sdk/src/model_lifecycle_service.cpp:419,498、services/src/server/model/model_file_manager.cpp:425、model_downloader.cpp:436
URL 被原样写入日志,可能泄露凭据。
新增的拒绝日志会将带 userinfo 的非法 URL(如 https://user:password@host/x)完整写入日志,直接泄露账号密码,违反项目日志规范。
此外既有下载失败日志也会记录合法 URL 中的签名 query。
建议提供统一的 URL 脱敏函数,仅保留 scheme、host、port 和必要的路径信息,并移除 userinfo、query、fragment。


VerifyModelFilesBeforeLoad (line 55) 遇到空 checksum 时只记录 WARN,然后继续加载文件;单文件模型没有 checksum 时也会在 services/src/server/model/manager.cpp:67 直接通过。
此外,SmartServeClient::IsModelReady (line 344) 对空 checksum 仅检查路径存在,不验证文件内容。
因此,本提交保护了远程下载入口,但没有在最终模型加载边界强制完整性校验。本地放置、预装或其他方式写入的无 checksum 模型仍可进入推理引擎。
是否必须修复取决于安全目标:
- 如果目标只是“所有远程下载必须验证”,应在文档中明确本地模型属于另一个可信边界;
- 如果目标是“所有进入推理引擎的模型必须可信并经过验证”,当前实现仍有安全缺口。


已经确认修改,达成“所有进入推理引擎的模型必须可信并经过验证”,并且把fix/cross_path分支的修改移植过来了
库侧宏 SMARTSERVE_ENABLE_TEST_FILE_URLS 与测试侧宏 SMARTSERVE_TEST_FILE_URLS_AVAILABLE 命名不对称(因 PRIVATE 编译定义不传播所致),且 IsAllowedDownloadUrl(接受任意 )与 IsAllowedModelDownloadSource(严格要求 file+test:/// 前缀)的门控严格度不同,容易误用。
更稳妥的是集中转换和校验逻辑,避免两套门控逐渐分叉。


已修复
生产下载仅接受 HTTPS。
限制 libcurl 首跳和重定向协议。
强制合法的 64 位 SHA-256。
已存在的同尺寸文件也会重新计算 checksum,不能绕过验证后登记安装。
file:// 仅能由私有测试入口开启,无法由 models.json 触发。
internal 84/84、API 35/35 通过。