Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

isisd: fix rcap tlv double-free crash #16809

Merged
merged 2 commits into from
Sep 17, 2024
Merged

isisd: fix rcap tlv double-free crash #16809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d617581
isisd: fix rcap tlv double-free crash
louis-6wind Sep 12, 2024
cfd050a
tests: update isisd fuzz test
louis-6wind Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions isisd/isis_tlvs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6147,16 +6147,17 @@ static int unpack_tlv_router_cap(enum isis_tlv_context context,
return 0;
}

if (tlvs->router_cap)
/* Multiple Router Capability found */
rcap = tlvs->router_cap;
else {
/* Allocate router cap structure and initialize SR Algorithms */
rcap = XCALLOC(MTYPE_ISIS_TLV, sizeof(struct isis_router_cap));
if (!tlvs->router_cap) {
/* First Router Capability TLV.
* Allocate router cap structure and initialize SR Algorithms */
tlvs->router_cap = XCALLOC(MTYPE_ISIS_TLV,
sizeof(struct isis_router_cap));
for (int i = 0; i < SR_ALGORITHM_COUNT; i++)
rcap->algo[i] = SR_ALGORITHM_UNSET;
tlvs->router_cap->algo[i] = SR_ALGORITHM_UNSET;
}

rcap = tlvs->router_cap;

/* Get Router ID and Flags */
rcap->router_id.s_addr = stream_get_ipv4(s);
rcap->flags = stream_getc(s);
Expand All @@ -6178,7 +6179,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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. If rcap is allocated at line 6155, this memory will never be freed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return 0;
}

Expand Down Expand Up @@ -6489,7 +6489,6 @@ static int unpack_tlv_router_cap(enum isis_tlv_context context,
}
subtlv_len = subtlv_len - length - 2;
}
tlvs->router_cap = rcap;
return 0;
}

Expand Down
Binary file modified tests/isisd/test_fuzz_isis_tlv_tests.h.gz
View file
Open in desktop
Binary file not shown.
Loading