elfunwindinfo: bounds check gopclntab offsets before use - #1844
Conversation
getFunc and getPcval take offsets that come straight from the pclntab of the profiled executable, and use them to slice into it. Neither was fully checked, so an out of range value panics instead of being rejected: - getFunc computed funcOff+funSize, which wraps around for a large funcOff. For pre-Go1.18 pclntab the offset is read verbatim from the file as a 64-bit value, so the whole uintptr range is reachable. Do the comparison without overflowing instead. The resulting bound is tight: funSize is the start PC width plus the size of pclntabFunc, so the descriptor returned always ends within the table. - getPcval did not validate its offset. Only one of its four call sites compared it against the length of pctab, and that comparison misses negative values, which the int32 offset allows. Check both ends in getPcval itself and return an empty pcval, which the callers already handle: it steps to a stop immediately. Signed-off-by: Alban Crequy <albancrequy@microsoft.com> Assisted-by: Claude Opus 5
| func (g *Gopclntab) getPcval(offs int32, startPc uint) pcval { | ||
| // offs comes from the function descriptor. Check for invalid out of | ||
| // bound values. | ||
| if offs < 0 || int(offs) > len(g.pctab) { | ||
| return newPcval(nil, startPc, g.quantum) | ||
| } | ||
| return newPcval(g.pctab[int(offs):], startPc, g.quantum) | ||
| } |
There was a problem hiding this comment.
In
p.val is initialized to -1. When len(p.ptr) == 0returns early without mutating
p.val, which remains -1. newPcval discards the result of that first step(), so callers cannot distinguish between "first entry loaded" and "empty table".
Two call sites then consume this -1 value:
opentelemetry-ebpf-profiler/nativeunwind/elfunwindinfo/elfgopclntab.go
Lines 871 to 872 in 3fdcc20
and:
opentelemetry-ebpf-profiler/nativeunwind/elfunwindinfo/elfgopclntab.go
Lines 895 to 896 in 3fdcc20
In parseX86pclntabFunc and parseArm64pclntabFunc, the loop body executes once before the p.step(). With the -1 value intact, it emits a bogus delta on x86, Param == 7 (p.val + 8). On arm64, the p.val != 0 branch produces Param == -1.
Because no error is returned, the malformed entry silently yields invalid unwind metadata instead of being rejected.
We could add something to distinguish between those scenarios.
Pull request dashboard statusWaiting on the author · refreshed 2026-09-11 09:00 UTC Respond to 2 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
| // offs comes from the function descriptor. Check for invalid out of | ||
| // bound values. | ||
| if offs < 0 || int(offs) > len(g.pctab) { | ||
| return newPcval(nil, startPc, g.quantum) |
There was a problem hiding this comment.
Some of the callers do check for this, but not all. But to handle this, let's change the signature of this helper to return an error instead. I think we should abort instead of silently accept malformed data. Please check all call sites and remove the redundant checks from them.
3 participants
getFuncandgetPcvalslice into the pclntab using offsets read from the profiled executable. Neither was fully checked, so a malformed pclntab panics instead of being skipped.getFunccomputedfuncOff+funSize, which wraps around. For pre-Go1.18 pclntabfuncOffis auintptrread verbatim from the file, so the whole 64-bit range is reachable:index out of range [18446744073709551615] with length 128. Compare without overflowing instead.getPcvaldid not validate its offset, and only one of its four call sites did. The offsets areint32, so a negative one givesslice bounds out of range [-1:]. Check both ends ingetPcvaland return an emptypcval, which callers already handle by stopping immediately.Both panics are covered by new tests, which fail on main.
Not filed as a security report: SECURITY.md excludes denial of service by a crafted profiled executable, and asks for a normal PR.
Assisted-by: Claude Opus 5
AIL:3