SWSPLAT-30704 firmware_image fix security issues in xsabin parsing#9889
Open
stsoe wants to merge 1 commit into
Open
SWSPLAT-30704 firmware_image fix security issues in xsabin parsing#9889stsoe wants to merge 1 commit into
stsoe wants to merge 1 commit into
Conversation
Six security defects were found and fixed in the xsabin/dsabin parsing code that handles firmware images supplied by users or loaded from disk. 1. remove_xsabin_mirror: broken mirror-tag search The original code cast the buffer pointer to a stringstream and called find() on it, which searched the pointer's textual address rather than the buffer contents. Offsets derived this way were completely wrong and the subsequent memset/memcpy had no bounds checks. Fixed by using std::search over the raw byte range; offsets are now derived from pointer arithmetic guaranteed to be within [0, bufferSize). 2. remove_xsabin_section: integer underflow in header-shift memcpy bytesToShift was computed as m_length minus the byte distance to the next section header. If that distance exceeded m_length (e.g. from a crafted xsabin), the subtraction underflowed to a huge value, causing a wild memcpy. Fixed by checking fromOffset against m_length before subtracting. 3. Section offset/size not validated against buffer length m_sectionOffset and m_sectionSize from the axlf header were used directly to allocate buffers and seek/read from the file with no check that [offset, offset+size) fits within m_length. A crafted binary could trigger out-of-bounds reads or huge allocations. Fixed with a new check_section_bounds() helper (overflow-safe: guards size > buf_len - offset) applied at all five section-read sites (DTB, BMC x2, FLASH, MCS). 4. Embedded struct fields not validated against section bounds Fields bmc->m_offset/m_size, flashMeta.m_image_offset/m_image_size, and mcs_chunk::m_offset/m_size come from within the file and were used to compute seek offsets and allocation sizes with no range check against the enclosing section. Also, no check that the section is large enough to hold the header struct itself. Fixed by applying check_section_bounds() at each embedded-offset site and adding sizeof(struct) minimum-size guards. 5. matchId: out-of-bounds access due to inverted empty check The condition `if (uuids.empty())` guarded an access to uuids[0], meaning the access was reached only when the vector was empty, causing undefined behaviour. Fixed by inverting to !uuids.empty(). 6. SWSPLAT-30704: remove_xsabin_section() heap OOB write via attacker-controlled section offsets Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
| return; // Tags are in the wrong order | ||
|
|
||
| // Zero out memory (not really needed but done for completeness) | ||
| uint64_t start_offset = static_cast<uint64_t>(start_ptr - buf_begin); |
Contributor
There was a problem hiding this comment.
warning: use auto when initializing with a cast to avoid duplicating the type name [hicpp-use-auto]
Suggested change
| uint64_t start_offset = static_cast<uint64_t>(start_ptr - buf_begin); | |
| auto start_offset = static_cast<uint64_t>(start_ptr - buf_begin); |
|
|
||
| // Zero out memory (not really needed but done for completeness) | ||
| uint64_t start_offset = static_cast<uint64_t>(start_ptr - buf_begin); | ||
| uint64_t end_offset = static_cast<uint64_t>(end_ptr - buf_begin); |
Contributor
There was a problem hiding this comment.
warning: use auto when initializing with a cast to avoid duplicating the type name [hicpp-use-auto]
Suggested change
| uint64_t end_offset = static_cast<uint64_t>(end_ptr - buf_begin); | |
| auto end_offset = static_cast<uint64_t>(end_ptr - buf_begin); |
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.
Problem solved by the commit
Six security defects were found and fixed in the xsabin/dsabin parsing code that handles firmware images supplied by users or loaded from disk.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
SWSPLAT-30704
How problem was solved, alternative solutions (if any) and why they were rejected
remove_xsabin_mirror: broken mirror-tag search The original code cast the buffer pointer to a stringstream and called find() on it, which searched the pointer's textual address rather than the buffer contents. Offsets derived this way were completely wrong and the subsequent memset/memcpy had no bounds checks. Fixed by using std::search over the raw byte range; offsets are now derived from pointer arithmetic guaranteed to be within [0, bufferSize).
remove_xsabin_section: integer underflow in header-shift memcpy bytesToShift was computed as m_length minus the byte distance to the next section header. If that distance exceeded m_length (e.g. from a crafted xsabin), the subtraction underflowed to a huge value, causing a wild memcpy. Fixed by checking fromOffset against m_length before subtracting.
Section offset/size not validated against buffer length m_sectionOffset and m_sectionSize from the axlf header were used directly to allocate buffers and seek/read from the file with no check that [offset, offset+size) fits within m_length. A crafted binary could trigger out-of-bounds reads or huge allocations. Fixed with a new check_section_bounds() helper (overflow-safe: guards size > buf_len - offset) applied at all five section-read sites (DTB, BMC x2, FLASH, MCS).
Embedded struct fields not validated against section bounds Fields bmc->m_offset/m_size, flashMeta.m_image_offset/m_image_size, and mcs_chunk::m_offset/m_size come from within the file and were used to compute seek offsets and allocation sizes with no range check against the enclosing section. Also, no check that the section is large enough to hold the header struct itself. Fixed by applying check_section_bounds() at each embedded-offset site and adding sizeof(struct) minimum-size guards.
matchId: out-of-bounds access due to inverted empty check The condition
if (uuids.empty())guarded an access to uuids[0], meaning the access was reached only when the vector was empty, causing undefined behaviour. Fixed by inverting to !uuids.empty().SWSPLAT-30704: remove_xsabin_section() heap OOB write via attacker-controlled section offsets
What has been tested and how, request additional testing if necessary
Testing is difficult w/o hardware to program