Commit c41e955
committed
tasks/task_patch: bound BPS patch action-loop and size-prelude against attacker-controlled inputs
bps_apply_patch (tasks/task_patch.c) implemented BPS patch
application without bounds checks on any of the per-command
reads or writes. The action loop:
while (bps.modify_offset < bps.modify_length - 12)
{
size_t _len = bps_decode(&bps); /* attacker-chosen */
unsigned mode = _len & 3;
_len = (_len >> 2) + 1;
switch (mode)
{
case TARGET_READ:
while (_len--) {
uint8_t data = bps_read(&bps);
bps.target_data[bps.output_offset++] = data; /* OOB write */
}
case SOURCE_READ:
while (_len--) {
uint8_t data = bps.source_data[bps.output_offset]; /* OOB read */
bps.target_data[bps.output_offset++] = data;
}
case SOURCE_COPY:
bps.source_offset += offset; /* unbounded signed offset */
while (_len--) {
uint8_t data = bps.source_data[bps.source_offset++]; /* OOB read */
bps.target_data[bps.output_offset++] = data;
}
case TARGET_COPY:
bps.target_offset += offset;
while (_len--) {
uint8_t data = bps.target_data[bps.target_offset++];
bps.target_data[bps.output_offset++] = data;
}
}
}
A malicious .bps could:
- Write attacker-chosen bytes far past the malloc'd
target_data buffer (heap-buffer-overflow WRITE) via any
of the four modes.
- Read past source_data via SOURCE_COPY's unbounded signed
offset. Negative offsets underflow source_offset to a
huge size_t; positive offsets push it past
source_length. The leaked bytes flow into target_data
and the patch checksum, exposing heap contents.
- Read past modify_data via TARGET_READ when _len exceeds
the remaining patch bytes. The action-loop guard
confirms only "at least one command's worth of bytes",
not the full read range.
The size prelude (modify_source_size / modify_target_size /
modify_markup_size, decoded by bps_decode immediately after
the "BPS1" header) was also unchecked:
- modify_target_size declared as size_t silently truncated
the uint64 bps_decode result on 32-bit hosts. A value of
0x100000000 wraps to 0; the subsequent malloc(0) returns
a tiny allocation while bps.target_length is set to the
raw uint64. The next target_data[output_offset++] write
OOB by the truncated delta.
- modify_markup_size was used as a loop bound consuming
bytes from modify_data via bps_read. A value above the
remaining patch bytes ran the read loop off the end of
modify_data.
Reachability: user has soft-patching enabled (the default
when a .bps file with the same basename as the loaded ROM is
present) and loads a ROM whose .bps was supplied by an
attacker. Same threat class as CDFS / CHD / BSV file-load
bugs.
Fix:
1. Capture the three size-prelude values as raw uint64,
reject anything above SIZE_MAX (so the size_t cast is
guaranteed lossless), and reject markup_size that would
drive bps_read past modify_length.
2. At the top of every action-loop iteration, bound _len
against (target_length - output_offset). This single
bound covers the target_data write in all four modes.
3. In each mode, bound the source-side read range:
- SOURCE_READ: output_offset + _len <= source_length
- TARGET_READ: modify_offset + _len <= modify_length - 12
- SOURCE_COPY: source_offset (post-offset-add) + _len
<= source_length
- TARGET_COPY: target_offset (post-offset-add) + _len
<= target_length
Adds samples/tasks/bps_patch/bps_patch_bounds_test as a
regression test, registered in
.github/workflows/Linux-samples-tasks.yml as an ASan-enabled
step. Five cases covering OOB-write rejection, OOB-read
rejection, legitimate operations, and the exact-capacity
boundary.
Test follows the existing samples/tasks pattern (verbatim
copy of the post-fix predicates with maintenance-contract
comment, ASan as the discriminator). Verified to fire
under ASan when the bounds are removed: heap-buffer-overflow
WRITE of size 1 at offset 4 of a 4-byte allocation in
apply_action_loop. With the bounds the test passes clean.1 parent 669468d commit c41e955
4 files changed
Lines changed: 541 additions & 3 deletions
File tree
- .github/workflows
- samples/tasks/bps_patch
- tasks
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
171 | 171 | | |
172 | 172 | | |
173 | 173 | | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
0 commit comments