ospfd: Fix NULL Pointer Deference when dumping opaque lsa #19480
+174
−77
Conversation
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
Jafaral
requested changes
Aug 24, 2025
There was a problem hiding this comment.
Please refer to this section for more info on how to organize your commits and how to format your commits/PR messages. In particular, please:
- Squash all commit with the same title to one commit
- Use a short title for the new squashed commit, and copy the description you have for the PR to the commit message. You know your title is too long when it looks like the following (wrapped lines and dots) when you browse the commit on github, example
Fix the Null Pointer Deference Vulnerability in show_vty_ext_link_lan…
…_adj_sid. Add security check for vty_out. Specifically, Check NULL for vty. If vty is not available, dump info via zlog.
Signed-off-by: s1awwhy <seawwhy@163.com>
- Fix the commit messages of the other commits to adhere to the guidelines referenced above.
- Fix code formatting, see frrbot
Thank you very much for your contributions!
ospfd/ospf_opaque.c
Outdated
| void show_opaque_info_detail(struct vty *vty, struct ospf_lsa *lsa, | ||
| json_object *json) | ||
| { | ||
| { |
There was a problem hiding this comment.
This is clearly not resolved.
ospfd/ospf_ext.c
Outdated
| zlog_debug(" Wrong %s TLV size: %d(%d). Abort!", msg, ntohs(tlvh->length), size); \ | ||
| return size + TLV_HDR_SIZE; \ | ||
| } \ | ||
| } \ |
| json_object_string_addf(json, "remoteInterfaceAddress", "%pI4", | ||
| &top->value); | ||
|
|
||
| } |
When the command debug ospf packet all send/recv detail is enabled in the OSPF configuration, ospfd will dump detailed information of any received or sent OSPF packets, either via VTY or through the zlog. However, the original show_opaque_info_detail failed to check lsa.data. The patch adds security check for lsa.data to prevent null pointer dereferencing in show_opaque_info_detail when parsing the LSA. Signed-off-by: s1awwhy <seawwhy@163.com>
When the command debug ospf packet all send/recv detail is enabled in the OSPF configuration, ospfd will dump detailed information of any received or sent OSPF packets, either via VTY or through the zlog. However, the check for functab->show_opaque_info is not valid in original show_opaque_info_detail.The patch adds proper null check for functab and functab->show_opaque_info. Signed-off-by: s1awwhy <seawwhy@163.com>
Add security check for vty_out. Specifically, Check NULL for vty. If vty is not available, dump info via zlog. Signed-off-by: s1awwhy <seawwhy@163.com>
dc27b83 to
1f17653
Compare
When the command debug ospf packet all send/recv detail is enabled in the OSPF configuration, ospfd will dump detailed information of any received or sent OSPF packets, either via VTY or through the zlog. However, the original Opaque LSA handling code failed to check whether the VTY context and show_opaque_info were available, resulting in NULL pointer dereference and crashes in ospfd. The patch fixes the Null Pointer Deference Vulnerability in show_vty_ext_link_rmt_itf_addr, show_vty_ext_link_adj_sid, show_vty_ext_link_lan_adj_sid, show_vty_unknown_tlv, show_vty_link_info, show_vty_ext_pref_pref_sid, show_vtY_pref_info. Specifically, add NULL check for vty. If vty is not available, dump details via zlog. Signed-off-by: s1awwhy <seawwhy@163.com>
1f17653 to
fdd9574
Compare
Thank you very much for your guidance! I've revised the commits and code style. |
mjstapp
reviewed
Aug 25, 2025
|
@s1awwhy Kindly remove the whitespace changes. |
|
@s1awwhy Normally you have to fix the styling of each commit separately. You cannot fix that in the last commit. |
louis-6wind
suggested changes
Oct 30, 2025
| if (TLV_SIZE(tlvh) > buf_size) { | ||
| vty_out(vty, " TLV size %d exceeds buffer size. Abort!", | ||
| TLV_SIZE(tlvh)); | ||
| return buf_size; |
There was a problem hiding this comment.
Why are you removing this line ? It seems to be a mistake.
| vty_out(vty, | ||
| " Extended Link TLV size %d exceeds buffer size. Abort!\n", | ||
| length); | ||
| return buf_size; |
There was a problem hiding this comment.
Why are you removing this line ? It seems to be a mistake.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Solve issue #19471
When the command debug ospf packet all send/recv detail is enabled in the OSPF configuration, ospfd will dump detailed information of any received or sent OSPF packets, either via VTY or through the zlog. However, The original Opaque LSA handling code failed to check whether the VTY context and show_opaque_info were available, resulting in NULL pointer dereference and crashes in ospfd. This patch addresses the issue by adding security checks for the vty and pointers related to Opaque LSAs dumping. If the vty context is unavailable, the patch ensures that the packet details are logged using zlog instead.