Improve resilience of ELF parser to malformed input#2026
Merged
Conversation
Struct_ops programs need BTF, but when an ELF is presented without BTF, the struct ops reloc logic would panic. Adding a nil check and throwing a clear error when BTF is missing when its required. Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
When ELF sections are compressed the debug/elf package does not populate the `ReaderAt` field of the `elf.Section` struct. When we do not check for its presence, we end up with a nil pointer dereference when trying to read the section data. So check for the presence of the `ReaderAt` field and return an error if it is nil. We do not support compressed ELF sections. Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
dylandreimerink
force-pushed
the
feature/handle-elf-malformed-input
branch
from
ec0877b to
6a9069c
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens ELF/BTF parsing against malformed inputs by replacing panic-prone reads and arithmetic with validation errors.
Changes:
- Adds bounds checks for symbol offsets/sizes and compressed sections in ELF loading paths.
- Adds missing guards for BTF-dependent struct ops and zero section alignment.
- Changes line-info parsing to avoid a single large allocation from untrusted record counts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
elf_reader.go |
Adds validation around ELF sections, symbols, BTF map values, data variables, and struct ops metadata. |
btf/ext_info.go |
Parses line-info records in bounded chunks instead of allocating the full record slice upfront. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Part of the .BTF.ext header contains the number of line info records. We were blindly taking this number and allocating x mount of records in go. If this number is malformed it causes an out of memory error. So instead of pre-allocating memory for all the records, lets do so for 1024 at a time. If we run out of section data, we stop and return an error, going from an OOM to a more reasonable error. Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
Fuzzing the ELF reader revealed that it can be made to read out of bounds data if a symbol's size extends beyond the end of its section. Adding a bounds check to throw a proper error instead of panicking. Limit symbol values and sizes to 32 bits to prevent 64 bit integer overflow. This limits the maximum size of a symbol to 4GB, which is more than enough. Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
A final round of fuzzing surfaced 3 more edge cases. First is an integer overflow, handled by limiting both integers to 32 bit so they do not overflow a 64 bit integer when compared. Second is a division by zero, handled by checking if the divisor is zero before performing the division. Third is a compressed section, which is not supported. After these the fuzzer surfaced nothing within an hour of fuzzing, so I consider this good enough for now. Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
dylandreimerink
force-pushed
the
feature/handle-elf-malformed-input
branch
from
6a9069c to
3b62881
Compare
Contributor
|
Great, thanks. ❤️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Inspired by #2024 I decided to run our existing
FuzzLoadCollectionSpecfuzz test until it didn't return panics for an hour.This surfaced quite a number of places where "user input" in the form of an ELF file would cause a panic. These were out of bounds reads from slices, nil pointer dereferenceing, and out of memory exceptions. The out of bounds reads were often caused by integer overflows in bounds checks.
All in all, this should make the library panic less often and throw normal errors instead when faced with bad or unexpected input.