isisd: fix rcap tlv double-free crash #16809
Merged
Conversation
This file contains 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
|
ci:rerun |
|
@Mergifyio backport stable/10.1 stable/10.0 stable/9.1 stable/9.0 |
✅ Backports have been created
|
donaldsharp
requested changes
Sep 13, 2024
| @@ -6178,7 +6178,6 @@ static int unpack_tlv_router_cap(enum isis_tlv_context context, | |||
| log, indent, | |||
| "WARNING: Router Capability subTLV length too large compared to expected size\n"); | |||
| stream_forward_getp(s, STREAM_READABLE(s)); | |||
| XFREE(MTYPE_ISIS_TLV, rcap); | |||
There was a problem hiding this comment.
This is wrong. If rcap is allocated at line 6155, this memory will never be freed.
There was a problem hiding this comment.
See trace below. It is freed.
XFREE pattern is present nowhere else in unpack_XXX
49efc80 showed no traces to prove that was a memory leak
=================================================================
==3811717==ERROR: AddressSanitizer: heap-use-after-free on address 0x51d0000b23a8 at pc 0xaaaaaadcab0c bp 0xffffffffd300 sp 0xffffffffd2f8
READ of size 8 at 0x51d0000b23a8 thread T0
[Attaching after Thread 0xfffff7fee020 (LWP 3811717) fork to child process 3811735]
[New inferior 2 (process 3811735)]
[Detaching after fork from parent process 3811717]
[Inferior 1 (process 3811717) detached]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
process 3811735 is executing new program: /usr/lib/llvm-17/bin/llvm-symbolizer
Error in re-setting breakpoint 1: No source file named isis_tlvs.c.
Error in re-setting breakpoint 2: No source file named isis_tlvs.c.
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
#0 0xaaaaaadcab08 in free_tlv_router_cap /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_tlvs.c:5804:9
#1 0xaaaaaadca278 in isis_free_tlvs /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_tlvs.c:7697:2
#2 0xaaaaaad358cc in process_hello /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_pdu.c:831:2
#3 0xaaaaaad330f8 in isis_handle_pdu /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_pdu.c:1812:12
#4 0xaaaaaacc805c in LLVMFuzzerTestOneInput /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_main.c:382:5
#5 0xaaaaaabd61f4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/.libs/isisd+0x1361f4) (BuildId: f041bc2e5f75b89a58c31a844d9c0c17c146d074)
#6 0xaaaaaabc1700 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/.libs/isisd+0x121700) (BuildId: f041bc2e5f75b89a58c31a844d9c0c17c146d074)
#7 0xaaaaaabc6cd8 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/.libs/isisd+0x126cd8) (BuildId: f041bc2e5f75b89a58c31a844d9c0c17c146d074)
#8 0xaaaaaabee0d8 in main (/home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/.libs/isisd+0x14e0d8) (BuildId: f041bc2e5f75b89a58c31a844d9c0c17c146d074)
#9 0xfffff70e73f8 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#10 0xfffff70e74c8 in __libc_start_main csu/../csu/libc-start.c:392:3
#11 0xaaaaaabbd36c in _start (/home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/.libs/isisd+0x11d36c) (BuildId: f041bc2e5f75b89a58c31a844d9c0c17c146d074)
0x51d0000b23a8 is located 296 bytes inside of 2352-byte region [0x51d0000b2280,0x51d0000b2bb0)
freed by thread T0 here:
#0 0xaaaaaac8d428 in free (/home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/.libs/isisd+0x1ed428) (BuildId: f041bc2e5f75b89a58c31a844d9c0c17c146d074)
#1 0xfffff7848f7c in qfree /home/ubuntu/frr-public/frr_public_private-libfuzzer/lib/memory.c:134:2
#2 0xaaaaaae0abc0 in unpack_tlv_router_cap /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_tlvs.c:6187:4
#3 0xaaaaaae1b3e4 in unpack_tlv /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_tlvs.c:8025:10
#4 0xaaaaaadcd4ac in unpack_tlvs /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_tlvs.c:8047:8
#5 0xaaaaaadcd278 in isis_unpack_tlvs /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_tlvs.c:8078:7
#6 0xaaaaaad34b98 in process_hello /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_pdu.c:723:6
#7 0xaaaaaad330f8 in isis_handle_pdu /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_pdu.c:1812:12
#8 0xaaaaaacc805c in LLVMFuzzerTestOneInput /home/ubuntu/frr-public/frr_public_private-libfuzzer/isisd/isis_main.c
There was a problem hiding this comment.
Again please look at line 6155 and NOTICE that the function is doing a malloc of rcap. And then notice the return line immediately after the XFREE. Finally notice that rcap is not saved until after the while loop. As such this is a memory leak that is being introduced. NAK
There was a problem hiding this comment.
How about on line 6155 you put rcap = tlvs->router_cap = X… and remove this free and remove the setting of the line at the bottom of the while loop
louis-6wind
force-pushed
the
fix-rcap-double-free
branch
from
f03e954 to
9be69a4
Compare
A double-free crash happens when a subTLV of the "Router Capability" TLV is not readable and a previous "Router Capability" TLV was read. rcap was supposed to be freed later by isis_free_tlvs() -> free_tlv_router_cap(). In 78774bb ("isisd: add isis flex-algo lsp advertisement"), this was not the case because rcap was not saved to tlvs->router_cap when the function returned early because of a subTLV length issue. Always set tlvs->router_cap to free the memory. Note that this patch has the consequence that in case of subTLV error, the previously read "Router Capability" subTLVs are kept in memory. Fixes: 49efc80 ("isisd: Ensure rcap is freed in error case") Fixes: 78774bb ("isisd: add isis flex-algo lsp advertisement") Reported-by: Iggy Frankovic <[email protected]> Signed-off-by: Louis Scalbert <[email protected]>
Since the previous commit, if a router capability subTLV is not readable, the previously read subTLVs are kept. Update of the ISIS fuzz test. > $ wuschl rebuild tests/isisd/test_fuzz_isis_tlv > $ gzip -9 tests/isisd/test_fuzz_isis_tlv_tests.h > $ ./test_fuzz_isis_tlv 2>/dev/null | grep failed > Test 139 failed, output differs. > Test 150 failed, output differs. > 2 of 405 tests failed. > > $ ./test_fuzz_isis_tlv 139 2>/dev/null > Test 139 failed, output differs. > Expected output: > Unpack log: > Unpacking 564 bytes of TLVs... > Unpacking TLV... > Found TLV of type 193 and len 13. > Skipping unknown TLV 193 (13 bytes) > Unpacking TLV... > Found TLV of type 0 and len 0. > Skipping unknown TLV 0 (0 bytes) > Unpacking TLV... > Found TLV of type 0 and len 0. > Skipping unknown TLV 0 (0 bytes) > Unpacking TLV... > Found TLV of type 242 and len 12. > Unpacking Router Capability TLV... > WARNING: Router Capability subTLV length too large compared to expected size > Unpacked TLVs: > Received output: > Unpack log: > Unpacking 564 bytes of TLVs... > Unpacking TLV... > Found TLV of type 193 and len 13. > Skipping unknown TLV 193 (13 bytes) > Unpacking TLV... > Found TLV of type 0 and len 0. > Skipping unknown TLV 0 (0 bytes) > Unpacking TLV... > Found TLV of type 0 and len 0. > Skipping unknown TLV 0 (0 bytes) > Unpacking TLV... > Found TLV of type 242 and len 12. > Unpacking Router Capability TLV... > WARNING: Router Capability subTLV length too large compared to expected size > Unpacked TLVs: > Router Capability: 253.212.128.242 , D:1, S:1 > > $ ./test_fuzz_isis_tlv 150 2>/dev/null > Test 150 failed, output differs. > Expected output: > Unpack log: > Unpacking 403 bytes of TLVs... > Unpacking TLV... > Found TLV of type 129 and len 13. > Unpacking Protocols Supported TLV... > Protocols Supported: 73, 16, 255, 255, 255, 101, 10, 11, 11, 11, 11, 11, 11 > Unpacking TLV... > Found TLV of type 11 and len 11. > Skipping unknown TLV 11 (11 bytes) > Unpacking TLV... > Found TLV of type 242 and len 12. > Unpacking Router Capability TLV... > WARNING: Router Capability subTLV length too large compared to expected size > Unpacked TLVs: > Protocols Supported: 73, 16, 255, 255, 255, 101, 10, 11, 11, 11, 11, 11, 11 > Received output: > Unpack log: > Unpacking 403 bytes of TLVs... > Unpacking TLV... > Found TLV of type 129 and len 13. > Unpacking Protocols Supported TLV... > Protocols Supported: 73, 16, 255, 255, 255, 101, 10, 11, 11, 11, 11, 11, 11 > Unpacking TLV... > Found TLV of type 11 and len 11. > Skipping unknown TLV 11 (11 bytes) > Unpacking TLV... > Found TLV of type 242 and len 12. > Unpacking Router Capability TLV... > WARNING: Router Capability subTLV length too large compared to expected size > Unpacked TLVs: > Protocols Supported: 73, 16, 255, 255, 255, 101, 10, 11, 11, 11, 11, 11, 11 > Router Capability: 253.212.128.242 , D:1, S:1 Link: https://pypi.org/project/wuschl/ Signed-off-by: Louis Scalbert <[email protected]>
louis-6wind
force-pushed
the
fix-rcap-double-free
branch
from
9be69a4 to
cfd050a
Compare
|
LGTM |
please also approve to remove the merging blocker |
donaldsharp
approved these changes
Sep 17, 2024
This was referenced Sep 17, 2024
donaldsharp
added a commit
that referenced
this pull request
Sep 17, 2024
isisd: fix rcap tlv double-free crash (backport #16809)
donaldsharp
added a commit
that referenced
this pull request
Sep 17, 2024
isisd: fix rcap tlv double-free crash (backport #16809)
donaldsharp
added a commit
that referenced
this pull request
Sep 17, 2024
isisd: fix rcap tlv double-free crash (backport #16809)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix rcap tlv double-free crash. It is supposed to freed later by isis_free_tlvs() -> free_tlv_router_cap().
Fixes: 49efc80 ("isisd: Ensure rcap is freed in error case")