fix: validate LoD byte size in DenseTensor deserialization to prevent heap overflow (CWE-787)#79508
Conversation
DeserializeFromStream read an attacker-controlled uint64 size, allocated a std::vector<size_t> of size / sizeof(size_t) elements, then read the full size bytes into it. When size is not a multiple of sizeof(size_t) the destination buffer is smaller than the read length and the read writes past the heap allocation, causing a heap buffer overflow. Reject sizes that are not a multiple of sizeof(size_t) before allocating, and verify the full number of bytes was read via gcount(). Ref: PaddlePaddle#79492
|
你的PR提交成功,感谢你对开源项目的贡献! |
risemeup1111
left a comment
There was a problem hiding this comment.
已完成初始 review。当前还有需要修复后再合入的问题,具体代码细节已放在 inline review comments;CI 目前仍有任务在运行,后续修复后也建议一起确认。
优先级:P1
处理要求:请针对该评论修复并提交新的 commit。
非行级:当前 diff 没有新增回归测试。这个 PR 修复的是可由恶意序列化文件触发的安全问题,至少需要覆盖 size % sizeof(size_t) != 0 被拒绝,以及 lod_level/size 声称长度超过实际剩余数据时不会先分配超大内存。建议在 test/legacy_test/test_paddle_save_load_binary.py 或新增 C++ 单测中构造最小二进制流调用 base.core.load_dense_tensor_from_memory/load_dense_tensor,断言抛出 Paddle 异常。
| @@ -117,9 +117,35 @@ void DeserializeFromStream(std::istream &is, | |||
| for (uint64_t i = 0; i < lod_level; ++i) { | |||
There was a problem hiding this comment.
优先级:P1
处理要求:请针对该评论修复并提交新的 commit。
lod_level 来自输入流,但当前在检查是否读满、数量是否合理之前,就已经用于 lod.resize(lod_level)。畸形文件可以在 version 后只提供部分 lod_level 字节,is.read(...lod_level...) 失败但仍留下一个部分填充的很大值;完整但虚假的超大 lod_level 也会在读取每层 size 前先触发超大 LoD 分配。这样安全修复仍保留可由恶意权重文件触发的 OOM/异常退出面。
请在 lod.resize(lod_level) 前校验 lod_level 头部已读满,并把 level 数量限制到当前流剩余字节能容纳的范围内(每层至少还需要一个 uint64_t size 头)。修复形态可参考:
is.read(reinterpret_cast<char*>(&lod_level), sizeof(lod_level));
PADDLE_ENFORCE_EQ(
static_cast<uint64_t>(is.gcount()),
sizeof(lod_level),
common::errors::InvalidArgument(
"Deserialize LoD information failed, expected to read %zu bytes for "
"LoD level count but only %lld bytes were available.",
sizeof(lod_level),
static_cast<int64_t>(is.gcount())));
const auto cur = is.tellg();
if (cur != std::istream::pos_type(-1)) {
is.seekg(0, std::ios::end);
const auto end = is.tellg();
is.seekg(cur);
PADDLE_ENFORCE_LE(
lod_level,
static_cast<uint64_t>((end - cur) / sizeof(uint64_t)),
common::errors::InvalidArgument(...));
}| i, | ||
| size, | ||
| sizeof(size_t))); | ||
| std::vector<size_t> tmp(size / sizeof(size_t)); |
There was a problem hiding this comment.
优先级:P1
处理要求:请针对该评论修复并提交新的 commit。
这里虽然已经禁止了非 sizeof(size_t) 倍数的 size,但仍在校验该字段是否读满、是否超过剩余数据之前,就直接用它构造 std::vector<size_t>,随后还把它转换成 std::streamsize 传给 read。恶意文件可以把 size 设成很大的 8 字节倍数但不提供对应 payload;代码会先尝试分配 size 级别的内存,下面的 gcount() 检查来得太晚,无法防止 OOM/bad_alloc。如果 size 超过 std::streamsize::max(),后面的长度转换也不安全。
请在分配前校验:size 字段自身已读满,size <= std::streamsize::max(),并且对可 seek 的输入流确认 size 不超过当前剩余字节。可以将这段检查放在读取 size 后、构造 tmp 前:
PADDLE_ENFORCE_EQ(
static_cast<uint64_t>(is.gcount()),
sizeof(size),
common::errors::InvalidArgument(...));
PADDLE_ENFORCE_LE(
size,
static_cast<uint64_t>((std::numeric_limits<std::streamsize>::max)()),
common::errors::InvalidArgument(...));
const auto cur = is.tellg();
if (cur != std::istream::pos_type(-1)) {
is.seekg(0, std::ios::end);
const auto end = is.tellg();
is.seekg(cur);
PADDLE_ENFORCE_LE(
size,
static_cast<uint64_t>(end - cur),
common::errors::InvalidArgument(...));
}
std::vector<size_t> tmp(size / sizeof(size_t));同时需要为 std::numeric_limits 补充 <limits> include。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查当前 head,代码没有新提交;上一轮两条 P1 inline 意见仍处于 active 状态,仍需要通过新的 commit 修复。这里不重复展开旧问题。
优先级:P1
处理要求:请编辑 PR 描述修复模板检查失败;代码相关 P1 仍需提交新的 commit。
非行级:PR 描述/CI 模板检查。当前模板检查把 PR Category 解析成了 ry\n\nExecute Infrastructure.,导致 Check PR Template 失败。请把 PR body 开头的模板注释和前导空行移除,让第一行直接从 ### PR Category 开始,并保留分类值为精确的 Execute Infrastructure,例如:
### PR Category
Execute Infrastructure
### PR Types
Security
risemeup1111
left a comment
There was a problem hiding this comment.
已复查当前 PR 描述,模板字段现在从 ### PR Category 开始,上一条 PR 描述/模板检查建议看起来已经处理。
当前代码 head 仍没有新 commit,因此上一轮代码相关 P1 inline 意见以及缺少回归测试的 P1 仍需通过后续 commit 修复后再复查。
…ation Address review feedback on PaddlePaddle#79508: besides rejecting non-sizeof(size_t) multiple LoD sizes, validate that the lod_level and per-level size headers were fully read and bound them against the remaining stream bytes before allocating, so a crafted serialized tensor cannot force a huge allocation (OOM) or an unsafe std::streamsize cast. Applies to both DeserializeFromStream overloads. Adds <limits>.
Covers rejection of a non-sizeof(size_t) multiple LoD size (heap buffer overflow) and absurd lod_level/size values (OOM) via base.core.load_dense_tensor_from_memory.
|
Thanks for the detailed review — addressed both P1s and the template check. 1. 2. 3. Regression test — 4. Template — fixed so |
risemeup1111
left a comment
There was a problem hiding this comment.
已复查新提交。上一轮两条代码相关 P1 在当前实现中看起来已经处理,恶意 LoD 的回归测试也已补上。
当前还剩一个 Pre Commit 暴露的 clang-format 问题,已在 inline comment 里给出可直接应用的修改;修复并 push 后可以继续复查。
| // The destination buffer holds `size / sizeof(size_t)` elements, i.e. | ||
| // exactly `size` bytes only when `size` is a multiple of `sizeof(size_t)`. | ||
| // Otherwise the read below would write past the allocation (CWE-787 / | ||
| // heap buffer overflow), so reject such sizes. |
There was a problem hiding this comment.
优先级:P1
处理要求:请针对该评论修复并提交新的 commit。
当前 head 的 Pre Commit 已失败,clang-format 自动修改的正是这段注释换行;如果不提交格式化后的内容,CI 会继续阻塞。请按下面的 clang-format 结果更新并重新 push。
| // The destination buffer holds `size / sizeof(size_t)` elements, i.e. | |
| // exactly `size` bytes only when `size` is a multiple of `sizeof(size_t)`. | |
| // Otherwise the read below would write past the allocation (CWE-787 / | |
| // heap buffer overflow), so reject such sizes. | |
| // The destination buffer holds `size / sizeof(size_t)` elements, i.e. | |
| // exactly `size` bytes only when `size` is a multiple of | |
| // `sizeof(size_t)`. Otherwise the read below would write past the | |
| // allocation (CWE-787 / heap buffer overflow), so reject such sizes. |
|
Applied the clang-format reflow from the inline suggestion (the comment above the size % sizeof(size_t) check) and verified no remaining lines exceed the 80-column limit. Pushed — Pre-Commit should pass now. |
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。之前的 LoD level/size 分配前校验问题、恶意 LoD 回归测试缺口,以及 clang-format 问题在当前 head 都已处理;Pre Commit 也已经通过。当前代码改动看起来可以接受,剩余 CI 请继续关注最终结果。
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (82.60%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #79508 +/- ##
==========================================
Coverage ? 82.60%
==========================================
Files ? 1
Lines ? 23
Branches ? 0
==========================================
Hits ? 19
Misses ? 4
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PR Category
Execute Infrastructure
PR Types
Security
Description
Fixes #79492 — heap buffer overflow in
DeserializeFromStream(LoD length integer-division truncation).Problem
In
paddle/phi/core/framework/dense_tensor_serialize.cc, the LoD header is parsed by reading an attacker-controlled 64-bit byte countsize, allocating astd::vector<size_t>sized by integer divisionsize / sizeof(size_t), and then reading the fullsizebytes into that buffer:When
sizeis not a multiple ofsizeof(size_t)(8), the destination buffer is smaller than the read length, so the read writes up to 7 bytes past the heap allocation with attacker-supplied bytes — a heap buffer overflow (WRITE). The routine is reachable from Python viapaddle.base.core.load_dense_tensorand the tensor/weight loading paths.A crafted serialized tensor and an AddressSanitizer trace (
heap-buffer-overflow WRITE) are in issue #79492.Solution
sizethat is not a multiple ofsizeof(size_t)before allocating/reading.sizebytes were actually read (gcount()), rejecting truncated or corrupted streams.SerializeToStreamalways writeslevel.size() * sizeof(size_t)bytes per LoD level, so legitimately serialized tensors are always a multiple ofsizeof(size_t)and are never rejected by this check. Tensors with no LoD (the common case,lod_level == 0) do not enter this loop at all.This vulnerability has also been responsibly disclosed to the Baidu security team (international_bsrc@baidu.com).
cc @luotao1
是否引起精度变化
否