isisd: improve validation of flex-algo decoder

[mjstapp]

Validate flex-algo sub-TLV before accessing data; validate embedded sub-sub-TLVs before accessing them; avoid length wrap-around.

[greptile-apps]

Greptile Summary

This PR hardens the flex-algo sub-TLV decoder in isisd/isis_tlvs.c against malformed or malicious IS-IS packets by adding three distinct validation guards:

• FAD minimum-size check (line 5131): skips the entire sub-TLV and breaks early if length < ISIS_SUBTLV_FAD_MIN_SIZE (4), preventing out-of-bounds reads of the four fixed fields (algorithm, metric_type, calc_type, priority).
• Sub-sub-TLV length overrun check (line 5168): before parsing each embedded sub-sub-TLV, ensures subsubtlv_len ≤ subsubtlvs_len − 2. If the length field would overrun the remaining buffer, the code correctly forwards subsubtlvs_len − 2 bytes (the bytes still unread after consuming the 2-byte type/length header) and breaks the while loop, consuming exactly length bytes in total.
• Per-type alignment/minimum-size checks (lines 5178–5215): validates that EXCAG, INCANYAG, and INCALLAG sub-sub-TLVs are at least 4 bytes and a multiple of 4, fixing a pre-existing silent bug where odd small lengths caused the stream to become misaligned (e.g. subsubtlv_len == 1 previously read 0 bytes but decremented the counter by 3).

The stream accounting and subsubtlvs_len tracking remain consistent across all error paths. The break statements in the inner switch cases correctly exit only the switch and let subsubtlvs_len -= 2 + subsubtlv_len (line 5245) handle the per-iteration accounting, allowing parsing to resume at the next sub-sub-TLV — this is the intended graceful-degradation behaviour.

• One minor typo in the INCANYAG error message ("INCANYG") should be corrected for log clarity.

Confidence Score: 4/5

• Safe to merge after fixing the one-character log-message typo; all validation logic and stream accounting are correct.
• The three new validation guards are logically sound, stream byte-consumption is balanced on every code path (exactly length bytes consumed in all cases), and the uint8_t arithmetic carries no overflow risk given the while-loop entry precondition. The only issue is a trivial typo in one error log string ("INCANYG" → "INCANYAG"). Prior review concerns about the stream_forward_getp off-by-two and the break-exits-switch behaviour are correctly resolved in this revision.
• No files require special attention beyond the one-line typo fix in isisd/isis_tlvs.c.

Important Files Changed

Filename	Overview
isisd/isis_tlvs.c	Adds three layers of validation to the Flex-Algo sub-TLV decoder: minimum size check for the FAD sub-TLV itself, an overrun check for embedded sub-sub-TLVs before reading their type/length headers, and per-type minimum-size/alignment guards for EXCAG/INCANYAG/INCALLAG sub-sub-TLVs. Logic and stream accounting are correct; one minor log-message typo ("INCANYG" → "INCANYAG").

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Receive ISIS_SUBTLV_FAD] --> B{length < FAD_MIN_SIZE?}
    B -- Yes --> C[stream_forward_getp length\nbreak case]
    B -- No --> D[Read 4 fixed bytes\nalgorithm/metric/calc/priority\nAlloc & init fad struct]
    D --> E[subsubtlvs_len = length - 4]
    E --> F{subsubtlvs_len > 2?}
    F -- No --> Z[break case]
    F -- Yes --> G[Read subsubtlv_type + subsubtlv_len\n2 bytes]
    G --> H{subsubtlv_len >\nsubsubtlvs_len - 2?}
    H -- Yes\noverrun --> I[stream_forward_getp\nsubsubtlvs_len - 2\nbreak while loop]
    I --> Z
    H -- No --> J{subsubtlv_type?}
    J -- EXCAG/INCANYAG/INCALLAG --> K{len < 4 or\nnot multiple of 4?}
    K -- Yes --> L[stream_forward_getp subsubtlv_len\nbreak switch]
    K -- No --> M[Read n_ag × uint32_t values\ninto admin_group]
    M --> N[break switch]
    L --> N
    J -- FLAGS --> O[Read flags byte +\nskip remaining]
    O --> N
    J -- ESRLG --> P[stream_forward_getp subsubtlv_len]
    P --> N
    J -- default --> Q[Log unsupported\nstream_forward_getp subsubtlv_len]
    Q --> N
    N --> R[subsubtlvs_len -= 2 + subsubtlv_len]
    R --> F

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: isisd/isis_tlvs.c
Line: 5197

Comment:
**Typo in log message: "INCANYG" should be "INCANYAG"**

The error message is missing the letter `A`, making it inconsistent with the sub-TLV name `ISIS_SUBTLV_FAD_SUBSUBTLV_INCANYAG` and the surrounding messages which correctly spell out "EXCAG" and "INCALLAG".

```suggestion
						  "Received an invalid Flex-Algo INCANYAG sub-TLV\n");
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (3): Last reviewed commit: "isisd: improve validation of flex-algo d..." | Re-trigger Greptile}

[greptile-apps]

Off-by-two overread in stream_forward_getp

At this point in the loop, 2 bytes have already been consumed from the stream (subsubtlv_type on line 5164 and subsubtlv_len on line 5165). The number of bytes remaining in the subsubtlvs region is subsubtlvs_len - 2, not subsubtlvs_len. Forwarding subsubtlvs_len bytes will overread 2 bytes past the subsubtlvs boundary and corrupt the outer sub-TLV parser's stream position.

The exact same issue exists in the inner switch-case error paths:

• isisd/isis_tlvs.c:5182 (EXCAG invalid length)
• isisd/isis_tlvs.c:5198 (INCANYAG invalid length)
• isisd/isis_tlvs.c:5214 (INCALLAG invalid length)

All four should use subsubtlvs_len - 2:

Suggested change

	stream_forward_getp(s, subsubtlvs_len);
	stream_forward_getp(s, subsubtlvs_len - 2);

Prompt To Fix With AI

This is a comment left during a code review.
Path: isisd/isis_tlvs.c
Line: 5172

Comment:
**Off-by-two overread in `stream_forward_getp`**

At this point in the loop, 2 bytes have already been consumed from the stream (`subsubtlv_type` on line 5164 and `subsubtlv_len` on line 5165). The number of bytes remaining in the subsubtlvs region is `subsubtlvs_len - 2`, not `subsubtlvs_len`. Forwarding `subsubtlvs_len` bytes will overread 2 bytes past the subsubtlvs boundary and corrupt the outer sub-TLV parser's stream position.

The exact same issue exists in the inner switch-case error paths:
- `isisd/isis_tlvs.c:5182` (EXCAG invalid length)
- `isisd/isis_tlvs.c:5198` (INCANYAG invalid length)
- `isisd/isis_tlvs.c:5214` (INCALLAG invalid length)

All four should use `subsubtlvs_len - 2`:

```suggestion
				stream_forward_getp(s, subsubtlvs_len - 2);
```

How can I resolve this? If you propose a fix, please make it concise.

[mjstapp]

yes, this is valid - confused "subsubtlv" and "subsubtlvs" variables...

[greptile-apps]

break exits switch, not the enclosing while loop

This break (and the identical ones at isisd/isis_tlvs.c:5198-5199 and isisd/isis_tlvs.c:5214-5215) only exits the switch statement. After the break, execution continues at line 5245:

subsubtlvs_len -= 2 + subsubtlv_len;

This decrements subsubtlvs_len further, and if the new value is still > 2, the while loop continues to read from the stream—past the region that was already skipped by stream_forward_getp. This causes reads from beyond the subsubtlvs region (or potentially past the entire sub-TLV).

A goto is the idiomatic fix in this style of C code:

/* at the error site: */
stream_forward_getp(s, subsubtlvs_len - 2);
goto done_subsubtlvs;

/* after the while loop (line 5246): */
done_subsubtlvs:
    break;  /* breaks the outer switch (case ISIS_SUBTLV_FAD) */

Alternatively, set a flag before the inner break and check it after the switch to exit the while.

Prompt To Fix With AI

This is a comment left during a code review.
Path: isisd/isis_tlvs.c
Line: 5182-5183

Comment:
**`break` exits `switch`, not the enclosing `while` loop**

This `break` (and the identical ones at `isisd/isis_tlvs.c:5198-5199` and `isisd/isis_tlvs.c:5214-5215`) only exits the `switch` statement. After the `break`, execution continues at line 5245:

```c
subsubtlvs_len -= 2 + subsubtlv_len;
```

This decrements `subsubtlvs_len` further, and if the new value is still `> 2`, the `while` loop continues to read from the stream—past the region that was already skipped by `stream_forward_getp`. This causes reads from beyond the subsubtlvs region (or potentially past the entire sub-TLV).

A `goto` is the idiomatic fix in this style of C code:

```c
/* at the error site: */
stream_forward_getp(s, subsubtlvs_len - 2);
goto done_subsubtlvs;
```

```c
/* after the while loop (line 5246): */
done_subsubtlvs:
    break;  /* breaks the outer switch (case ISIS_SUBTLV_FAD) */
```

Alternatively, set a flag before the inner `break` and check it after the `switch` to exit the `while`.

How can I resolve this? If you propose a fix, please make it concise.

[mjstapp]

I think this will be fixed when I fix the preceding stream_forward_getp() calls.

[mjstapp]

Pushed a fix for greptile comments

[mjstapp]

@greptileai review

[mjstapp]

... and pushed one more time to include reported-by commit comment

[Jafaral]

@Mergifyio backport stable/10.6 stable/10.5 stable/10.4 stable/10.3 stable/10.2

[mergify]

backport stable/10.6 stable/10.5 stable/10.4 stable/10.3 stable/10.2

✅ Backports have been created

Details

• #21401 isisd: improve validation of flex-algo decoder (backport #21314) has been created for branch stable/10.6
• #21402 isisd: improve validation of flex-algo decoder (backport #21314) has been created for branch stable/10.5 but encountered conflicts

Cherry-pick of 42c6dd7 has failed:

On branch mergify/bp/stable/10.5/pr-21314
Your branch is up to date with 'origin/stable/10.5'.

You are currently cherry-picking commit 42c6dd7af.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   isisd/isis_tlvs.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

• #21403 isisd: improve validation of flex-algo decoder (backport #21314) has been created for branch stable/10.4 but encountered conflicts

Cherry-pick of 42c6dd7 has failed:

On branch mergify/bp/stable/10.4/pr-21314
Your branch is up to date with 'origin/stable/10.4'.

You are currently cherry-picking commit 42c6dd7af.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   isisd/isis_tlvs.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

• #21404 isisd: improve validation of flex-algo decoder (backport #21314) has been created for branch stable/10.3 but encountered conflicts

Cherry-pick of 42c6dd7 has failed:

On branch mergify/bp/stable/10.3/pr-21314
Your branch is up to date with 'origin/stable/10.3'.

You are currently cherry-picking commit 42c6dd7af.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   isisd/isis_tlvs.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

• #21405 isisd: improve validation of flex-algo decoder (backport #21314) has been created for branch stable/10.2 but encountered conflicts

Cherry-pick of 42c6dd7 has failed:

On branch mergify/bp/stable/10.2/pr-21314
Your branch is up to date with 'origin/stable/10.2'.

You are currently cherry-picking commit 42c6dd7af.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   isisd/isis_tlvs.c

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

[Jafaral]

@greptile review

[mjstapp]

CI:rerun

[riw777]

looks good