ospfd: add validation in several places before accessing message bodies - #21303
Conversation
Greptile SummaryThis PR adds bounds/size validation for LSA, TLV, and sub-TLV buffers in All five P0/P1 findings raised in earlier review rounds (missing early-exit on Confidence Score: 4/5Safe to merge once the developer confirms no further follow-up commits are outstanding; all five previously flagged critical issues are resolved in HEAD. All P0/P1 findings from prior review rounds — missing loop early-exit, unsigned underflow in two ext_pref functions, sizeof-vs-body-size false positives, and the NBO stack overflow — are verified fixed in the current commit. Remaining observations are P2-level. Score is 4 rather than 5 only as a prompt to confirm no additional fixes are pending before merge. No files require special attention — all previously flagged critical paths have been addressed.
|
| Filename | Overview |
|---|---|
| ospfd/ospf_sr.c | Adds error_p guards and body-only size constants for sub-TLV validation in get_ext_link_sid; replaces raw algo pointer with a local copy using host-byte-order length in ospf_sr_ri_lsa_update; all previously flagged issues (early-exit loop, sizeof mismatch, NBO overflow) are resolved. |
| ospfd/ospf_te.c | Adds LSA/TLV size guards in ospf_te_parse_te, ospf_te_parse_ri, ospf_te_parse_ext_pref, ospf_te_delete_ext_pref, ospf_te_parse_ext_link, and ospf_te_delete_ext_link; previously flagged unsigned-underflow issues are now guarded with < OSPF_LSA_HEADER_SIZE pre-checks. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Receive OSPF LSA] --> B{lsa->size <= OSPF_LSA_HEADER_SIZE?}
B -- yes --> C[Warn & return / -1]
B -- no --> D[Compute body length]
D --> E{length <= TLV_HDR_SIZE?}
E -- yes --> C
E -- no --> F[Get TLV header pointer]
F --> G{TLV_BODY_SIZE < minimum struct size?}
G -- yes --> C
G -- no --> H[Cast pointer to struct]
H --> I[Iterate sub-TLVs]
I --> J{tlv_size > remaining?}
J -- yes --> K[Set error_p, break]
J -- no --> L{Type switch}
L --> M{Sub-TLV body size valid?}
M -- no --> K
M -- yes --> N[Access struct fields safely]
N --> O{error_p set?}
O -- yes --> K
O -- no --> P{More sub-TLVs?}
P -- yes --> I
P -- no --> Q[Process accumulated data]
K --> R[Warn & abort / free]
Reviews (9): Last reviewed commit: "ospfd: add validation in several places ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| sum += tlv_size; | ||
| if (sum < length) | ||
| tlvh = TLV_HDR_NEXT(tlvh); | ||
| if (sum >= length || ((length - sum) < TLV_HDR_SIZE)) | ||
| break; | ||
|
|
||
| tlvh = TLV_HDR_NEXT(tlvh); | ||
| } |
There was a problem hiding this comment.
After error_p is set in one of the case blocks (e.g. for RI_SR_TLV_SRGB_LABEL_RANGE), the loop does not break immediately. It falls through to sum += tlv_size and may call TLV_HDR_NEXT(tlvh), continuing to process subsequent TLVs even though an error was already detected. Subsequent iterations can even overwrite algo, ri_srgb, etc. with pointers from later (possibly valid) TLVs.
This is inconsistent with every other modified loop in this same PR. Both get_ext_link_sid (same file, line ~1080) and ospf_te_parse_te (ospf_te.c, line ~2375) correctly add if (error_p) break; immediately after the switch statement. The missing guard should be added here too.
| } | |
| sum += tlv_size; | |
| if (sum < length) | |
| tlvh = TLV_HDR_NEXT(tlvh); | |
| if (sum >= length || ((length - sum) < TLV_HDR_SIZE)) | |
| break; | |
| tlvh = TLV_HDR_NEXT(tlvh); | |
| } | |
| } | |
| if (error_p) | |
| break; | |
| sum += tlv_size; | |
| if (sum >= length || ((length - sum) < TLV_HDR_SIZE)) | |
| break; | |
| tlvh = TLV_HDR_NEXT(tlvh); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ospfd/ospf_sr.c
Line: 1514-1521
Comment:
**Missing early-exit on error inside loop**
After `error_p` is set in one of the `case` blocks (e.g. for `RI_SR_TLV_SRGB_LABEL_RANGE`), the loop does not break immediately. It falls through to `sum += tlv_size` and may call `TLV_HDR_NEXT(tlvh)`, continuing to process subsequent TLVs even though an error was already detected. Subsequent iterations can even overwrite `algo`, `ri_srgb`, etc. with pointers from later (possibly valid) TLVs.
This is inconsistent with every other modified loop in this same PR. Both `get_ext_link_sid` (same file, line ~1080) and `ospf_te_parse_te` (`ospf_te.c`, line ~2375) correctly add `if (error_p) break;` immediately after the `switch` statement. The missing guard should be added here too.
```suggestion
}
if (error_p)
break;
sum += tlv_size;
if (sum >= length || ((length - sum) < TLV_HDR_SIZE))
break;
tlvh = TLV_HDR_NEXT(tlvh);
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
yes, that's a valid comment; I will fix it
| /* Validate TLV buffer */ | ||
| if ((lsa->size - OSPF_LSA_HEADER_SIZE) <= TLV_HDR_SIZE) { | ||
| zlog_warn("Malformed EXT Prefix LSA"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Same class of bug as in ospf_te_parse_ext_pref above. lsa->size - OSPF_LSA_HEADER_SIZE is an unsigned size_t subtraction. If lsa->size < 20, the result wraps to a value far larger than TLV_HDR_SIZE, the check is always false, and execution falls through to TLV_BODY_SIZE(tlvh) on a potentially invalid pointer.
The guard should follow the same pattern used in ospf_te_parse_te and ospf_te_parse_ri:
| /* Validate TLV buffer */ | |
| if ((lsa->size - OSPF_LSA_HEADER_SIZE) <= TLV_HDR_SIZE) { | |
| zlog_warn("Malformed EXT Prefix LSA"); | |
| return -1; | |
| } | |
| /* Validate TLV buffer */ | |
| if (lsa->size <= OSPF_LSA_HEADER_SIZE || | |
| (lsa->size - OSPF_LSA_HEADER_SIZE) <= TLV_HDR_SIZE) { | |
| zlog_warn("Malformed EXT Prefix LSA"); | |
| return -1; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ospfd/ospf_te.c
Line: 2902-2906
Comment:
**Unsigned underflow bypasses size validation**
Same class of bug as in `ospf_te_parse_ext_pref` above. `lsa->size - OSPF_LSA_HEADER_SIZE` is an unsigned `size_t` subtraction. If `lsa->size < 20`, the result wraps to a value far larger than `TLV_HDR_SIZE`, the check is always false, and execution falls through to `TLV_BODY_SIZE(tlvh)` on a potentially invalid pointer.
The guard should follow the same pattern used in `ospf_te_parse_te` and `ospf_te_parse_ri`:
```suggestion
/* Validate TLV buffer */
if (lsa->size <= OSPF_LSA_HEADER_SIZE ||
(lsa->size - OSPF_LSA_HEADER_SIZE) <= TLV_HDR_SIZE) {
zlog_warn("Malformed EXT Prefix LSA");
return -1;
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
yes, that's valid, I will fix those
7576b00 to
8d87e76
Compare
|
Pushed an update to fix a couple of greptile comments |
|
@greptileai review |
| @@ -1031,6 +1039,13 @@ static struct sr_link *get_ext_link_sid(struct tlv_header *tlvh, size_t size) | |||
| IPV4_ADDR_COPY(&srl->nhlfe[i].nexthop, &link->link_id); | |||
| break; | |||
| case EXT_SUBTLV_LAN_ADJ_SID: | |||
| /* Validate sub-TLV length */ | |||
| if (TLV_BODY_SIZE(sub_tlvh) < | |||
| sizeof(struct ext_subtlv_lan_adj_sid)) { | |||
| error_p = true; | |||
| break; | |||
| } | |||
|
|
|||
| lan_sid = (struct ext_subtlv_lan_adj_sid *)sub_tlvh; | |||
| srl->type = LAN_ADJ_SID; | |||
| i = CHECK_FLAG(lan_sid->flags, | |||
| @@ -1047,18 +1062,38 @@ static struct sr_link *get_ext_link_sid(struct tlv_header *tlvh, size_t size) | |||
| &lan_sid->neighbor_id); | |||
| break; | |||
| case EXT_SUBTLV_RMT_ITF_ADDR: | |||
| /* Validate sub-TLV length */ | |||
| if (TLV_BODY_SIZE(sub_tlvh) < | |||
| sizeof(struct ext_subtlv_rmt_itf_addr)) { | |||
| error_p = true; | |||
| break; | |||
| } | |||
|
|
|||
| rmt_itf = (struct ext_subtlv_rmt_itf_addr *)sub_tlvh; | |||
| IPV4_ADDR_COPY(&srl->nhlfe[0].nexthop, &rmt_itf->value); | |||
| IPV4_ADDR_COPY(&srl->nhlfe[1].nexthop, &rmt_itf->value); | |||
There was a problem hiding this comment.
sizeof(struct) includes TLV header, causing all valid TLVs to fail validation
Every new size check in ospf_sr.c compares TLV_BODY_SIZE(tlvh) — which is ROUNDUP(ntohs(tlvh->length), 4), i.e. the body only — against sizeof(struct …), which includes the embedded struct tlv_header (4 bytes). Since the header fields are part of every struct, sizeof is always 4 bytes larger than the actual body, so the check is always true for a valid TLV, causing error_p = true for every legitimate sub-TLV.
Concrete examples:
struct ext_subtlv_adj_sidbody = 8 bytes →TLV_BODY_SIZE = 8,sizeof = 12→8 < 12✓ (false positive error)struct ext_subtlv_lan_adj_sidbody = 12 bytes →TLV_BODY_SIZE = 12,sizeof = 16→12 < 16✓ (false positive error)struct ext_subtlv_rmt_itf_addrbody = 4 bytes →TLV_BODY_SIZE = 4,sizeof = 8→4 < 8✓ (false positive error)
The existing ospf_te.c code uses the body-only size constants for the same TLVs (e.g. TLV_BODY_SIZE(tlvh) != EXT_SUBTLV_ADJ_SID_SIZE where EXT_SUBTLV_ADJ_SID_SIZE = 8). The fixes below should use those same constants or subtract TLV_HDR_SIZE:
// get_ext_link_sid fixes
case EXT_SUBTLV_ADJ_SID:
if (TLV_BODY_SIZE(sub_tlvh) < EXT_SUBTLV_ADJ_SID_SIZE) { ...
case EXT_SUBTLV_LAN_ADJ_SID:
if (TLV_BODY_SIZE(sub_tlvh) < EXT_SUBTLV_LAN_ADJ_SID_SIZE) { ...
case EXT_SUBTLV_RMT_ITF_ADDR:
if (TLV_BODY_SIZE(sub_tlvh) < EXT_SUBTLV_RMT_ITF_ADDR_SIZE) { ...The same pattern applies to all four cases in ospf_sr_ri_lsa_update (lines ~1474–1506). The correct body-only sizes are:
RI_SR_TLV_SR_ALGORITHM:TLV_BODY_SIZE < 1(at least one algorithm byte)RI_SR_TLV_SRGB_LABEL_RANGE/RI_SR_TLV_SRLB_LABEL_RANGE:TLV_BODY_SIZE < RI_SR_TLV_LABEL_RANGE_SIZE(= 12)RI_SR_TLV_NODE_MSD:TLV_BODY_SIZE < RI_SR_TLV_NODE_MSD_SIZE(= 4)
Prompt To Fix With AI
This is a comment left during a code review.
Path: ospfd/ospf_sr.c
Line: 1017-1074
Comment:
**`sizeof(struct)` includes TLV header, causing all valid TLVs to fail validation**
Every new size check in `ospf_sr.c` compares `TLV_BODY_SIZE(tlvh)` — which is `ROUNDUP(ntohs(tlvh->length), 4)`, i.e. the **body only** — against `sizeof(struct …)`, which includes the embedded `struct tlv_header` (4 bytes). Since the header fields are part of every struct, `sizeof` is always 4 bytes larger than the actual body, so the check is always true for a valid TLV, causing `error_p = true` for every legitimate sub-TLV.
Concrete examples:
- `struct ext_subtlv_adj_sid` body = 8 bytes → `TLV_BODY_SIZE = 8`, `sizeof = 12` → `8 < 12` ✓ (false positive error)
- `struct ext_subtlv_lan_adj_sid` body = 12 bytes → `TLV_BODY_SIZE = 12`, `sizeof = 16` → `12 < 16` ✓ (false positive error)
- `struct ext_subtlv_rmt_itf_addr` body = 4 bytes → `TLV_BODY_SIZE = 4`, `sizeof = 8` → `4 < 8` ✓ (false positive error)
The existing `ospf_te.c` code uses the body-only size constants for the same TLVs (e.g. `TLV_BODY_SIZE(tlvh) != EXT_SUBTLV_ADJ_SID_SIZE` where `EXT_SUBTLV_ADJ_SID_SIZE = 8`). The fixes below should use those same constants or subtract `TLV_HDR_SIZE`:
```c
// get_ext_link_sid fixes
case EXT_SUBTLV_ADJ_SID:
if (TLV_BODY_SIZE(sub_tlvh) < EXT_SUBTLV_ADJ_SID_SIZE) { ...
case EXT_SUBTLV_LAN_ADJ_SID:
if (TLV_BODY_SIZE(sub_tlvh) < EXT_SUBTLV_LAN_ADJ_SID_SIZE) { ...
case EXT_SUBTLV_RMT_ITF_ADDR:
if (TLV_BODY_SIZE(sub_tlvh) < EXT_SUBTLV_RMT_ITF_ADDR_SIZE) { ...
```
The same pattern applies to all four cases in `ospf_sr_ri_lsa_update` (lines ~1474–1506). The correct body-only sizes are:
- `RI_SR_TLV_SR_ALGORITHM`: `TLV_BODY_SIZE < 1` (at least one algorithm byte)
- `RI_SR_TLV_SRGB_LABEL_RANGE` / `RI_SR_TLV_SRLB_LABEL_RANGE`: `TLV_BODY_SIZE < RI_SR_TLV_LABEL_RANGE_SIZE` (= 12)
- `RI_SR_TLV_NODE_MSD`: `TLV_BODY_SIZE < RI_SR_TLV_NODE_MSD_SIZE` (= 4)
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
yes, that's a valid comment; I will fix it
8d87e76 to
228e8de
Compare
|
Pushed more fixes, caught a bug in another file too |
|
@greptileai review |
| for (i = 0; i < tlvh->length; i++) | ||
| algo.value[i] = *(p + i); |
There was a problem hiding this comment.
The loop bound tlvh->length is the raw network-byte-order uint16_t field, not the host-order value. On little-endian systems (x86/x86_64), a valid length of 1 stored in network order is the bytes 0x00 0x01, which when read as a little-endian uint16_t yields 256. For the maximum validated value of ALGORITHM_COUNT = 4, the raw field reads as 1024. The loop therefore tries to write up to 1024 bytes into algo.value[ALGORITHM_COUNT], which is only 4 bytes — this is a stack buffer overflow.
Notably, algo.length was set to ntohs(tlvh->length) on the line immediately before, so the fix is simply to use it:
| for (i = 0; i < tlvh->length; i++) | |
| algo.value[i] = *(p + i); | |
| for (i = 0; i < algo.length; i++) | |
| algo.value[i] = *(p + i); |
The analogous code in ospf_te_parse_ri (line 2641) correctly uses ntohs(algo->header.length) in its loop bound.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ospfd/ospf_sr.c
Line: 1500-1501
Comment:
**Network-byte-order loop bound causes stack buffer overflow**
The loop bound `tlvh->length` is the raw network-byte-order `uint16_t` field, not the host-order value. On little-endian systems (x86/x86_64), a valid length of `1` stored in network order is the bytes `0x00 0x01`, which when read as a little-endian `uint16_t` yields `256`. For the maximum validated value of `ALGORITHM_COUNT = 4`, the raw field reads as `1024`. The loop therefore tries to write up to 1024 bytes into `algo.value[ALGORITHM_COUNT]`, which is only 4 bytes — this is a stack buffer overflow.
Notably, `algo.length` was set to `ntohs(tlvh->length)` on the line immediately before, so the fix is simply to use it:
```suggestion
for (i = 0; i < algo.length; i++)
algo.value[i] = *(p + i);
```
The analogous code in `ospf_te_parse_ri` (line 2641) correctly uses `ntohs(algo->header.length)` in its loop bound.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
yes, that's a valid comment, I will fix it
228e8de to
f772aef
Compare
|
Pushed yet-another byte-ordering fix |
|
@greptileai review |
f772aef to
e2f46bb
Compare
|
... and pushed another set of fixes |
|
@greptileai review |
|
@Mergifyio backport stable/10.6 stable/10.5 stable/10.4 stable/10.3 stable/10.2 |
✅ Backports have been createdDetails
|
|
@greptileai review |
|
ci failures don't look related ... rerunning just the failed tests to see if we can clear those |
|
rebased, and cleaned-up a greptile comment |
|
@greptileai review |
|
there's a repeated failure in a relevant SR/TE topotest on one platform. I've added temporary logging in some of the changed code to see if I can learn more about why that's failing. |
edef14b to
c2ac523
Compare
|
Adding some more debugging info, including in the failing topotest |
|
version with debugs passed CI - try again without the debugs |
|
failed again on ubu 24 - trying again with some topotest debugs enabled on r1 |
|
adding some more ospf debugs, still failing one test on ubu 24 |
f24ff62 to
d2ee3dd
Compare
Add validation of LSA, TLV, and sub-TLV sizes before accessing bytes within the message bodies. Reported-by: Haruto Kimura (Stella) <harutokimura0608@gmail.com> Signed-off-by: Mark Stapp <mjs@cisco.com>
|
think I found the problem - pushed an update |
|
@greptile review |
|
Tip: Greploop — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
|
Target branch is not in the allowed branches list. |
ospfd: add validation in several places before accessing message bodies (backport #21303)
ospfd: add validation in several places before accessing message bodies (backport #21303)
ospfd: add validation in several places before accessing message bodies (backport #21303)
ospfd: add validation in several places before accessing message bodies (backport #21303)
ospfd: add validation in several places before accessing message bodies (backport #21303)
3 participants
Add validation of LSA, TLV, and sub-TLV sizes before accessing bytes within packet/message bodies.