Skip to content

Fix samtools merge crash on BAM files with malformed headers#2128

Merged
jkbonfield merged 1 commit into
samtools:developfrom
daviesrob:bad-merge-hdrs
Oct 16, 2024
Merged

Fix samtools merge crash on BAM files with malformed headers#2128
jkbonfield merged 1 commit into
samtools:developfrom
daviesrob:bad-merge-hdrs

Conversation

@daviesrob

@daviesrob daviesrob commented Oct 14, 2024

Copy link
Copy Markdown
Member

When the binary and text reference names disagree in a BAM header, it's possible for the number of references apparently present to increase when the header text is parsed. This caused a crash when samtools merge built its reference translation table because the trans_tbl_t::tid_trans array was allocated in trans_tbl_init() with the smaller size, and then trans_tbl_add_sq() used the bigger one when trying to put data in it.

Fixed by moving the allocation to trans_tbl_add_sq(), so the space allocated will always match the number of items iterated over.

Thanks to @frostb1ten for the bug report.
Fixes #2127 (Heap Buffer Overflow in samtools merge within trans_tbl_add_sq function)

@daviesrob
Fix samtools merge crash on BAM files with malformed headers
8c80a2a 
When the binary and text reference names disagree in a BAM header,
it's possible for the number of references apparently present to
increase when the header text is parsed.  This caused a crash when
samtools merge built its reference translation table because the
trans_tbl_t::tid_trans array was allocated in trans_tbl_init()
with the smaller size, and then trans_tbl_add_sq() used the bigger
one when trying to put data in it.

Fixed by moving the allocation to trans_tbl_add_sq(), so the
space allocated will always match the number of items iterated
over.

Thanks to @frostb1ten for the bug report.
Fixes samtools#2127 (Heap Buffer Overflow in samtools merge within
trans_tbl_add_sq function)
@daviesrob daviesrob mentioned this pull request Oct 14, 2024
Closed
@jkbonfield jkbonfield self-assigned this Oct 16, 2024
@jkbonfield

jkbonfield commented Oct 16, 2024

Copy link
Copy Markdown
Contributor

Do you think we should be updating htslib too to warn when there are disparities between binary and text SAM headers? It's legal (maybe?) but it's also unusual and probably indicative of something going wrong.

I'm also wondering how many other places have a similar issue. (This fix does work though.)

@jkbonfield jkbonfield merged commit 3f900e1 into samtools:develop Oct 16, 2024
@jkbonfield

jkbonfield commented Oct 16, 2024

Copy link
Copy Markdown
Contributor

I couldn't find any other obvious uses of sam_hdr_nref that occur before parsing the header and then iteration over ntargets after parsing. So I think we're safe, but it's sufficiently cryptic this probably wants documenting somewhere in htslib's header parser.

@daviesrob daviesrob deleted the bad-merge-hdrs branch October 16, 2024 16:20
@daviesrob

daviesrob commented Oct 16, 2024

Copy link
Copy Markdown
Member Author

I did a search for other uses of sam_hdr_nref() in samtools, and apart from the one in merge, they all looked safe from this bug. It might be possible to add a check to HTSlib, but we'd need to take care as the header API includes code to fix BAM headers with missing @SQ lines, which indicates that there was code that generated such data. I expect that the most likely case it either the @SQ lines or the reference list was completely missing, so it may be safe to complain if they are both present and contain data that doesn't match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jkbonfield jkbonfield

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Heap Buffer Overflow in samtools merge within trans_tbl_add_sq function

2 participants

@daviesrob @jkbonfield