tools/ax25_replay.cpp: heap overflow in loadWav() when the WAV data chunk length is not a multiple of 4

Open
#4,907 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
64/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
cpp
Domain
security, tooling

Research direction

Start in tools/ax25_replay.cpp:60-82 and review the existing WAV bounds and float32 validation, then inspect CMakeLists.txt:4768 and try building the ax25_replay target. Choose and document whether partial samples are rejected or truncated, ensure allocation and copying use consistent sizes, and handle the noted bounds concern. Done means the malformed WAV no longer causes an overflow and the relevant test or verification passes.

Written by the indexing model from the issue text.

Description

bug good first issue maintainer-review

loadWav() in tools/ax25_replay.cpp sizes its destination buffer by rounding
the WAV data chunk length down to whole samples, then memcpys the
full declared length into it. A data chunk whose length is not a multiple
of 4 overflows the heap allocation by up to 3 bytes.

// tools/ax25_replay.cpp:81-82
samples.resize(static_cast<size_t>(dataBytes / 4));          // rounds DOWN
std::memcpy(samples.data(), data, static_cast<size_t>(dataBytes));  // copies FULL length

dataBytes == 6 allocates one float (4 bytes) and copies 6.

Reproduced under ASan

The data length is fully attacker-controlled, and this needs no malformed
framing to reach: the file is otherwise well-formed, every byte it declares is
really present, and the existing bounds check at :60 passes cleanly.

==68511==ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 6 at 0x7b87f7de0074 thread T0
    #0 memcpy
    #1 loadWav(...) ax25_replay.cpp:82

0x7b87f7de0074 is located 0 bytes after 4-byte region [...0070,...0074)
allocated by thread T0 here:
    #6 std::vector<float>::resize(unsigned long)
    #7 loadWav(...) ax25_replay.cpp:81

The generator for the input file — a valid mono float32 WAV whose data chunk
declares 6 bytes:

import struct
fmt  = struct.pack('<HHIIHH', 3, 1, 48000, 48000*4, 4, 32) + b'\x00\x00'
data = b'\x01\x02\x03\x04\x05\x06'          # 6 bytes = 1.5 floats
body = b'WAVE' + b'fmt ' + struct.pack('<I', len(fmt)) + fmt \
     + b'data' + struct.pack('<I', len(data)) + data
wav  = b'RIFF' + struct.pack('<I', len(body)) + body
open('evil.wav','wb').write(wav + b'\x00' * max(0, 44 - len(wav)))

Caveat on how this was run: ax25_replay does not currently link (see
"Second, unrelated" below), so I ran loadWav() and its two readLe* helpers
extracted verbatim from the file, compiled with -fsanitize=address against
Qt6Core. The extracted code is byte-identical to tools/ax25_replay.cpp:26-84;
only main() was replaced. Worth re-confirming against the real binary once it
builds again.

Note on the Antares advisory that surfaced this

This was surfaced by the Antares detector on PR #4905 (which does not touch this
file). Its stated mechanism is not correct — it reports that the code "does
not verify that pos + sz <= bytes.size()", but :60-61 does exactly that and
breaks, so an oversized data length cannot reach the memcpy. The real
defect is the resize/memcpy size mismatch above, which is adjacent to but
distinct from what was reported. Flagging so the advisory isn't closed as a
false positive along with its mechanism.

Fix is a judgment call — hence an issue, not a PR

Two defensible behaviours, and the choice belongs to whoever owns the AX.25
tooling:

  1. Reject a data length that isn't a multiple of 4 — it is a malformed
    float32 WAV, and the tool already refuses non-float32/non-mono input with a
    clear error, so this is consistent with how it treats the fmt chunk.
  2. Truncate to whole samples — memcpy dataBytes & ~3 and ignore the
    trailing partial sample.

Either way the two sizes must be derived from one expression rather than
two. There is also a latent 32-bit-only concern: pos + static_cast<qsizetype>(sz)
with sz near UINT32_MAX can wrap where qsizetype is 32-bit, which would
defeat the :60 check — not reachable on the 64-bit builds anyone uses, but
worth handling in the same pass.

Scope and severity

Deliberately not fixed in #4905 — unrelated to that PR, and folding a
memory-safety fix into a feature branch makes both harder to review.

Mitigating: ax25_replay is an EXCLUDE_FROM_ALL developer tool
(CMakeLists.txt:4768), not built by default and not shipped, so exposure is a
developer pointing it at a malformed WAV — not a path reachable from the running
app or from the radio. Constitution Principle VII (untrusted input validated at
the boundary) still applies: a contributor-supplied file is exactly the input
class it names.

Pre-existing since #3381 (June); not a regression.

Second, unrelated: the tool does not currently build

cmake --build build --target ax25_replay fails to link on main
(f6f56865), on both a Debug and a clean RelWithDebInfo tree:

undefined reference to `AetherSDR::HdlcCodec::processBit(unsigned char)'
undefined reference to `AetherSDR::HdlcCodec::bitstreamSize() const'

from src/core/tnc/AetherAx25LibmodemShim.cpp:836 / :1022. Being
EXCLUDE_FROM_ALL, CI never builds it, so the breakage is invisible to the
gate. Happy to split this into its own issue if you'd rather keep them separate.

Dominant language
C++
Stars
221
Forks
117
Avg merge
2d 7h
Merged PRs (30d)
299

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from aethersdr/AetherSDR

All issues in aethersdr/AetherSDR

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.