isisd: add stream bounds checks in unpack_tlv_router_cap #20358

ravikumark815 · 2026-01-03T20:42:13Z

Summary

This PR fixes a critical crash in isisd when processing malformed Router Capability TLVs (Issue #20329).

Problem

The unpack_tlv_router_cap function in isisd/isis_tlvs.c called stream_getc() without validating that sufficient bytes were available in the stream. This triggered an assertion failure in lib/stream.c:353, causing the daemon to abort and creating a Remote Denial of Service (DoS) vulnerability.

Solution

Added four stream boundary checks using STREAM_READABLE() before all stream_getc() calls in the Router Capability TLV parsing logic:

Initial header validation (router ID + flags)
Sub-TLV loop header check
FAD sub-TLV length validation
FAD sub-sub-TLV loop header check
Also added a check to prevent memory leaks when duplicate FAD entries are received.

Testing

Compiled FRR with the fix
Sent malformed LSP packets with truncated Router Capability TLVs
Verified the daemon no longer crashes and handles the malformed packets gracefully
Confirmed normal ISIS operation continues after receiving malformed packets

Checklist

Code follows FRR coding standards
Commit message follows FRR guidelines
Signed-off-by present
Fix tested and verified
No new compiler warnings introduced

Fixes #20329

vjardin · 2026-01-03T22:42:31Z

typoe: do you mean the issue #20329 ? Update the commit log.

vjardin · 2026-01-03T23:16:18Z

The added code to handle duplicate FAD entries has a memory leak.

  if (rcap->fads[fad->fad.algorithm])
      XFREE(MTYPE_ISIS_TLV, rcap->fads[fad->fad.algorithm]);

The cleanup in isis_tlvs.c shows the correct approach:

  if (rcap->fads[fad->fad.algorithm]) {
      struct isis_router_cap_fad *old_fad = rcap->fads[fad->fad.algorithm];
      admin_group_term(&old_fad->fad.admin_group_exclude_any);
      admin_group_term(&old_fad->fad.admin_group_include_any);
      admin_group_term(&old_fad->fad.admin_group_include_all);
      XFREE(MTYPE_ISIS_TLV, old_fad);
  }

The four STREAM_READABLE() checks look correct and address the crash scenarios described in #20329. The FAD length validation (length < 4) is also appropriate.

Please address the two items above and this should be better. Consider running with ASan/valgrind/FRR memory leakage to verify no memory leaks occur when processing duplicate FAD sub-TLVs. Do you have a mean to add a test for it ?

ravikumark815 · 2026-01-04T00:49:14Z

The added code to handle duplicate FAD entries has a memory leak.
  if (rcap->fads[fad->fad.algorithm])
      XFREE(MTYPE_ISIS_TLV, rcap->fads[fad->fad.algorithm]);
The cleanup in isis_tlvs.c shows the correct approach:
  if (rcap->fads[fad->fad.algorithm]) {
      struct isis_router_cap_fad *old_fad = rcap->fads[fad->fad.algorithm];
      admin_group_term(&old_fad->fad.admin_group_exclude_any);
      admin_group_term(&old_fad->fad.admin_group_include_any);
      admin_group_term(&old_fad->fad.admin_group_include_all);
      XFREE(MTYPE_ISIS_TLV, old_fad);
  }
The four STREAM_READABLE() checks look correct and address the crash scenarios described in #20329. The FAD length validation (length < 4) is also appropriate.

Please address the two items above and this should be better. Consider running with ASan/valgrind/FRR memory leakage to verify no memory leaks occur when processing duplicate FAD sub-TLVs. Do you have a mean to add a test for it ?

Thank you for the review! I've addressed both issues:

Fixed issue number: Changed from Duplicate routes from local route protocol #20337 to isisd: crash in unpack_tlv_router_cap via stream_getc assertion failure #20329 in the commit message.
Fixed memory leak: Updated the duplicate FAD handling to properly cleanup admin_group structures before freeing memory, following the pattern used in the cleanup code at line 5105-5107.
The updated code now calls admin_group_term() for all three admin_group fields before XFREE().
Regarding testing:
I verified the fix using Valgrind with a custom test script that injects packets with duplicate FAD sub-TLVs.
Valgrind Output: The definitely lost: 0 bytes confirms that replacing duplicate FAD entries no longer leaks memory.
==191058== LEAK SUMMARY:
==191058== definitely lost: 0 bytes in 0 blocks
==191058== indirectly lost: 0 bytes in 0 blocks

isisd/isis_tlvs.c

mjstapp · 2026-01-05T14:57:14Z

isisd/isis_tlvs.c


 	sbuf_push(log, indent, "Unpacking Router Capability TLV...\n");
-	if (tlv_len < ISIS_ROUTER_CAP_SIZE) {
+	if (tlv_len < ISIS_ROUTER_CAP_SIZE ||


several of these changes seem to be oriented around "we can't trust the tlv_len value" that was passed in. it would be clearer to fix that: to ensure that tlv_len was valid before calling this function

I completely agree that validating tlv_len against the stream bounds before dispatching to the specific unpack function would be a better design for the entire TLV parsing.

However, unpack_tlv and the dispatch logic currently trust the tlv_len from the packet header. Changing that behavior would require refactoring how tlv_len is handled for all TLV types to ensure no regressions.

Given that this PR targets a specific crash/vulnerability, I'd prefer to keep the fix localized to unpack_tlv_router_cap to ensure it's safe and minimal.

well, but ... that's just wrong isn't it? "trust the value from the packet header" I mean - if that's true, that's something that has caused trouble in this specific path, and will continue to cause trouble until it's fixed.

I've refactored the code as suggested to ensure tlv_len validity before calling the handler.
Changes:

Added size_t min_len to struct tlv_ops.

Updated unpack_tlv() to check if (tlv_len < ops->min_len) before dispatching.

Defined TLV_OPS_MIN_LEN macro and used it for Router Capability (Type 242) with .min_len = 5.

Removed the manual length check from unpack_tlv_router_cap as it is now redundant.
This provides a central mechanism for enforcing minimum TLV lengths.

riw777

looks good ... just waiting on an answer to @mjstapp 's comments

Add validation to ensure sufficient stream data is available before calling stream_getc() in unpack_tlv_router_cap(). This prevents assertion failures when processing malformed Router Capability TLVs. The fix adds four boundary checks: 1. Verify STREAM_READABLE before reading the 5-byte Router Capability header (router ID + flags). 2. Check stream availability before reading sub-TLV type and length in the main sub-TLV processing loop. 3. Validate FAD sub-TLV length is at least 4 bytes before reading mandatory fields (algorithm, metric_type, calc_type, priority). 4. Verify stream has 2 bytes available before reading sub-sub-TLV headers in the FAD processing loop. Edit2: Additionally, add a check to prevent memory leak when duplicate FAD entries are received for the same algorithm. Edit3: Set `min_len` to 5 for Router Capability TLV (Type 242) to prevent processing malformed TLVs that are shorter than the header size, which previously caused assertion failures. Fixes: FRRouting#20329 Signed-off-by: ravikumark815 <[email protected]>

mjstapp

this has sort of wandered a bit.
the issue says that it's possible to get a stream with a readable size less than five through unpack_tlv_router_cap() - but I don't see how that's possible: the ROUTER_CAP_SIZE check appears to prevent that.
the issue also says that in general the tlv lengths aren't checked against the stream, but I do see checks in the incoming callers of isis_unpack_tlvs(), and in unpack_tlvs() and unpack_tlv(). can you show where it's possible to get past those existing checks?

the issue with the subtlvs is a different, and does appear to be real - so it would be better just to focus on that?

ravikumark815 · 2026-01-07T22:49:59Z

this has sort of wandered a bit. the issue says that it's possible to get a stream with a readable size less than five through unpack_tlv_router_cap() - but I don't see how that's possible: the ROUTER_CAP_SIZE check appears to prevent that. the issue also says that in general the tlv lengths aren't checked against the stream, but I do see checks in the incoming callers of isis_unpack_tlvs(), and in unpack_tlvs() and unpack_tlv(). can you show where it's possible to get past those existing checks?

the issue with the subtlvs is a different, and does appear to be real - so it would be better just to focus on that?

I've reverted the tlv_ops refactoring to keep the PR focused on the specific crash and memory leak in the Router Capability TLV, as suggested.
Regarding your question about the existing checks:

"can you show where it's possible to get past those existing checks?"

Yes, the crash occurs when a TLV advertises a length that is valid according to the stream (avail_len >= tlv_len), but smaller than the fixed header size expected by the handler.

Scenario:

Incoming Packet: [Type=242] [Len=2] [Byte1] [Byte2]
unpack_tlv() at line 7280 checks: avail_len < tlv_len + 2.
- 4 < 2 + 2 is FALSE. The check passes.
unpack_tlv_router_cap() is called.
It calls stream_get_ipv4(s) which attempts to read 4 bytes.
CRASH: The stream only has 2 bytes available.

That is why the check if (tlv_len < ISIS_ROUTER_CAP_SIZE) inside the handler is necessary—unpack_tlv validates the container size, but not the content requirements.

mjstapp · 2026-01-08T13:49:09Z

sorry: the point was that there are existing checks that appear to prevent several of the faults that the open issue complains about. I think that issue is largely bogus. you've now gone back and forth several times, without ever showing exactly what problem you were trying to solve - you were just pointing at the bogus open issue.

you had removed the content check that was already present, and was preventing the read of 5 octets from a too-small stream. I see you've now put that check back in place. so again, I'll ask this: please describe whether any of the complaints in the issue are actually valid, and then explain how to fix them. I think the complaint about sub-tlvs may be valid, for example.

this has sort of wandered a bit. the issue says that it's possible to get a stream with a readable size less than five through unpack_tlv_router_cap() - but I don't see how that's possible: the ROUTER_CAP_SIZE check appears to prevent that. the issue also says that in general the tlv lengths aren't checked against the stream, but I do see checks in the incoming callers of isis_unpack_tlvs(), and in unpack_tlvs() and unpack_tlv(). can you show where it's possible to get past those existing checks?
the issue with the subtlvs is a different, and does appear to be real - so it would be better just to focus on that?

I've reverted the tlv_ops refactoring to keep the PR focused on the specific crash and memory leak in the Router Capability TLV, as suggested. Regarding your question about the existing checks:

"can you show where it's possible to get past those existing checks?"

Yes, the crash occurs when a TLV advertises a length that is valid according to the stream (avail_len >= tlv_len), but smaller than the fixed header size expected by the handler.

Scenario:
1. Incoming Packet: `[Type=242] [Len=2] [Byte1] [Byte2]`

2. `unpack_tlv()` at line 7280 checks: `avail_len < tlv_len + 2`.
   
   * `4 < 2 + 2` is FALSE. The check **passes**.

3. `unpack_tlv_router_cap()` is called.

4. It calls `stream_get_ipv4(s)` which attempts to read **4 bytes**.

5. **CRASH**: The stream only has 2 bytes available.
That is why the check if (tlv_len < ISIS_ROUTER_CAP_SIZE) inside the handler is necessary—unpack_tlv validates the container size, but not the content requirements.

mjstapp · 2026-01-08T13:50:23Z

isisd/isis_tlvs.c

 	sbuf_push(log, indent, "Unpacking Router Capability TLV...\n");
-	if (tlv_len < ISIS_ROUTER_CAP_SIZE) {
+	if (tlv_len < ISIS_ROUTER_CAP_SIZE ||
+	    STREAM_READABLE(s) < ISIS_ROUTER_CAP_SIZE) {


I don't think this can be meaningful - please show how an invalid length value could be passed here.

mjstapp · 2026-01-08T13:51:48Z

isisd/isis_tlvs.c

 #endif /* ifndef FABRICD */
 		uint8_t msd_type;

+		if (STREAM_READABLE(s) < 2) {


again, please show how this could be needed, how the check above could be incorrect.

mjstapp · 2026-01-08T13:52:29Z

isisd/isis_tlvs.c

+		if (STREAM_READABLE(s) < 2) {
+			sbuf_push(log, indent,
+				  "WARNING: Unexpected end of stream\n");
+			return 0;


that's not the correct error return handling, is it?
won't matter if this block is removed, of course.

mjstapp · 2026-01-08T13:55:25Z

isisd/isis_tlvs.c

 				uint32_t v;
 				int n_ag, i;

+				if (STREAM_READABLE(s) < 2) {


and again: if the subtlv length was valid, then the subsubtlvs_len check that's already present should be valid. how could this occur?

github-actions · 2026-01-08T15:28:54Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

frrbot bot added bugfix isis labels Jan 3, 2026

github-actions bot added master size/S labels Jan 3, 2026

ravikumark815 mentioned this pull request Jan 3, 2026

isisd: crash in unpack_tlv_router_cap via stream_getc assertion failure #20329

Closed

2 tasks

ravikumark815 force-pushed the master branch from db4f580 to db57177 Compare January 4, 2026 00:43

github-actions bot added size/M and removed size/S labels Jan 4, 2026

donaldsharp reviewed Jan 5, 2026

View reviewed changes

isisd/isis_tlvs.c Show resolved Hide resolved

ravikumark815 requested a review from donaldsharp January 5, 2026 14:25

mjstapp reviewed Jan 5, 2026

View reviewed changes

riw777 approved these changes Jan 6, 2026

View reviewed changes

ravikumark815 requested a review from mjstapp January 6, 2026 18:21

ravikumark815 force-pushed the master branch from db57177 to 80eaf44 Compare January 7, 2026 00:31

github-actions bot added the rebase PR needs rebase label Jan 7, 2026

mjstapp reviewed Jan 7, 2026

View reviewed changes

Merge branch 'FRRouting:master' into master

b6d9381

ravikumark815 force-pushed the master branch from 90ab725 to b6d9381 Compare January 7, 2026 22:48

ravikumark815 requested a review from mjstapp January 7, 2026 22:52

mjstapp reviewed Jan 8, 2026

View reviewed changes

github-actions bot added the conflicts label Jan 8, 2026

isisd: add stream bounds checks in unpack_tlv_router_cap #20358

Are you sure you want to change the base?

isisd: add stream bounds checks in unpack_tlv_router_cap #20358

Conversation

ravikumark815 commented Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Testing

Checklist

Uh oh!

vjardin commented Jan 3, 2026

Uh oh!

vjardin commented Jan 3, 2026

Uh oh!

ravikumark815 commented Jan 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mjstapp Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravikumark815 Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjstapp Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

ravikumark815 Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

riw777 left a comment

Choose a reason for hiding this comment

Uh oh!

mjstapp left a comment

Choose a reason for hiding this comment

Uh oh!

ravikumark815 commented Jan 7, 2026

Uh oh!

mjstapp commented Jan 8, 2026

Uh oh!

mjstapp Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

mjstapp Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

mjstapp Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

mjstapp Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ravikumark815 commented Jan 3, 2026 •

edited

Loading

ravikumark815 commented Jan 4, 2026 •

edited

Loading

mjstapp Jan 5, 2026 •

edited

Loading

ravikumark815 Jan 6, 2026 •

edited

Loading