fingerprint: fix stack overflow building the TLS blocks string#3190
Merged
Conversation
The tls-blocks loop advanced idx by the raw snprintf() return value, which on truncation is the would-be length rather than what was written. With tls.max_num_blocks_to_analyze=32 and application block tracking enabled the blocks string regularly exceeds the 256-byte buffer, idx walks past it and the unsigned fp_buf_len-idx-1 size underflows, so the next snprintf() writes far out of bounds and trips the stack protector (SIGSEGV via __stack_chk_fail in ndpi_compute_ndpi_flow_fingerprint on aarch64/musl). Bound each round by the space actually remaining and stop at the first truncation, keeping the chars that fit so the digest input stays deterministic. Also bound the ignore_sni_extension patch byte to the formatted length instead of writing at strlen(l4_fp)+4 unconditionally. Reproduced the overflow with an ASan harness of the old loop (32 blocks of ",23=-14600" overflow a 256-byte buffer); the fixed loop caps at idx=255. Signed-off-by: Chad Monroe <chad@monroe.io>
|
Contributor
Author
|
That seems reasonable, but I'm not entirely sure how to go about this. The entry point to the crash is via ndpi_detection_process_packet() from classifi.c:1724 In case it's useful, here's the full backtrace from gdb as well: |
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
2 participants
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.
Please sign (check) the below before submitting the Pull Request:
Link to the related issue:
Describe changes:
The tls-blocks loop advanced idx by the raw snprintf() return value, which on truncation is the would-be length rather than what was written. With tls.max_num_blocks_to_analyze=32 and application block tracking enabled the blocks string regularly exceeds the 256-byte buffer, idx walks past it and the unsigned fp_buf_len-idx-1 size underflows, so the next snprintf() writes far out of bounds and trips the stack protector (SIGSEGV via __stack_chk_fail in ndpi_compute_ndpi_flow_fingerprint on aarch64/musl).
Bound each round by the space actually remaining and stop at the first truncation, keeping the chars that fit so the digest input stays deterministic. Also bound the ignore_sni_extension patch byte to the formatted length instead of writing at strlen(l4_fp)+4 unconditionally.
Reproduced the overflow with an ASan harness of the old loop (32 blocks of ",23=-14600" overflow a 256-byte buffer); the fixed loop caps at idx=255.