[dso] validate parsed TLV length in ProcessKeepAliveMessage()

[abtink]

This commit updates the Dso::Connection::ProcessKeepAliveMessage() method to use the OffsetRange class for parsing TLVs. This approach robustly validates the size of each parsed TLV against the remaining length of the received message. Utilizing OffsetRange::Contains() ensures that the reported TLV size via GetSize() does not exceed the available bytes in the message, preventing potential out-of-bounds reads or infinite loops when iterating over subsequent TLVs.

---

Should help address #12602.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the robustness and security of the DNS DSO connection by introducing critical size validation for TLV parsing within the ProcessKeepAliveMessage function. By verifying that each TLV's declared size fits within the available message buffer, the changes effectively prevent potential vulnerabilities such as out-of-bounds reads and infinite processing loops, contributing to a more stable and secure system.

Highlights

• TLV Length Validation: Added validation checks for the size of parsed TLVs (Type-Length-Value) within the Dso::Connection::ProcessKeepAliveMessage() method.
• Prevented Out-of-Bounds Reads: Ensured that the reported size of each TLV does not exceed the remaining length of the received message, mitigating potential out-of-bounds memory access.
• Improved Robustness: Addressed a potential issue that could lead to infinite loops when iterating over subsequent TLVs if their reported size was malformed.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• src/core/net/dns_dso.cpp
  • Implemented a VerifyOrExit check to ensure the initial KeepAliveTlv's size does not exceed the message's remaining length.
  • Introduced a similar VerifyOrExit check within the TLV processing loop to validate the size of all subsequent TLVs against the available message buffer.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request adds validation for the parsed TLV length in Dso::Connection::ProcessKeepAliveMessage() to prevent potential out-of-bounds reads. The changes are a good step towards improving security.

I've added one comment to further improve the parsing logic for the KeepAliveTlv. The current implementation reads a fixed size but advances the message offset by a variable size derived from the TLV's content, which can lead to parsing errors if the TLV is malformed. My suggestion is to enforce a length check that allows for future extensions, using a greater-than-or-equal comparison, and to use the known fixed size of the KeepAliveTlv structure for advancing the offset, making the parsing more robust and forward-compatible.

[github-actions]

Merging #12609 into main

name	branch	text	data	bss	total
ot-cli-ftd	49582b3	482136	860	66540	549536
	5c2a04b	482136	860	66540	549536
	+/-	+0	+0	+0	+0
ot-ncp-ftd	49582b3	448036	764	61776	510576
	5c2a04b	448036	764	61776	510576
	+/-	+0	+0	+0	+0
ot-cli-mtd	49582b3	374608	764	50972	426344
	5c2a04b	374608	764	50972	426344
	+/-	+0	+0	+0	+0
ot-ncp-mtd	49582b3	355756	764	46240	402760
	5c2a04b	355756	764	46240	402760
	+/-	+0	+0	+0	+0
ot-cli-ftd-br	49582b3	598408	868	136596	735872
	5c2a04b	598408	868	136596	735872
	+/-	+0	+0	+0	+0
ot-rcp	49582b3	63328	568	20804	84700
	5c2a04b	63328	568	20804	84700
	+/-	+0	+0	+0	+0

Library files

name	branch	text	data	bss	total
libopenthread-ftd.a	49582b3	249788	95	40319	290202
	5c2a04b	249788	95	40319	290202
	+/-	+0	+0	+0	+0
libopenthread-cli-ftd.a	49582b3	61727	0	8091	69818
	5c2a04b	61727	0	8091	69818
	+/-	+0	+0	+0	+0
libopenthread-ncp-ftd.a	49582b3	33581	0	5948	39529
	5c2a04b	33581	0	5948	39529
	+/-	+0	+0	+0	+0
libopenthread-mtd.a	49582b3	168223	0	24783	193006
	5c2a04b	168223	0	24783	193006
	+/-	+0	+0	+0	+0
libopenthread-cli-mtd.a	49582b3	41436	0	8059	49495
	5c2a04b	41436	0	8059	49495
	+/-	+0	+0	+0	+0
libopenthread-ncp-mtd.a	49582b3	25805	0	5948	31753
	5c2a04b	25805	0	5948	31753
	+/-	+0	+0	+0	+0
libopenthread-ftd-br.a	49582b3	366689	100	110335	477124
	5c2a04b	366733	100	110335	477168
	+/-	+44	+0	+0	+44
libopenthread-cli-ftd-br.a	49582b3	80934	0	8131	89065
	5c2a04b	80934	0	8131	89065
	+/-	+0	+0	+0	+0
libopenthread-rcp.a	49582b3	9944	0	5060	15004
	5c2a04b	9944	0	5060	15004
	+/-	+0	+0	+0	+0
libopenthread-radio.a	49582b3	19711	0	238	19949
	5c2a04b	19711	0	238	19949
	+/-	+0	+0	+0	+0

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.15%. Comparing base (01d75f7) to head (c5dba1b).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12609      +/-   ##
==========================================
- Coverage   75.64%   75.15%   -0.49%     
==========================================
  Files         698      696       -2     
  Lines       95490    94520     -970     
==========================================
- Hits        72235    71039    -1196     
- Misses      23255    23481     +226     

Files with missing lines	Coverage Δ
src/core/net/dns_dso.cpp	91.16% <100.00%> (+0.01%)	⬆️

... and 133 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jwhui]

/gemini review

[gemini-code-assist]

Code Review

This pull request effectively addresses a potential vulnerability in Dso::Connection::ProcessKeepAliveMessage() by replacing manual offset management with the OffsetRange class. This change enhances the robustness of TLV parsing, preventing out-of-bounds reads and potential infinite loops, and no new security vulnerabilities were identified. The implementation is well-done, with one suggestion to remove a redundant check for improved code clarity, aligning with best practices for avoiding unnecessary validations.