Fix stack buffer overflow in do_mroute() for (*,G) routes - #225
Merged
Conversation
do_mroute() copies the trailing outbound-interface arguments into a
fixed char *out[MAX_MC_VIFS] array with a plain
while (pos < msg->count)
out[num++] = msg->argv[pos++];
The only bound ever checked on msg->count is CMD_MAX_WORDS in
ipc_parse(), which is sized for the (S,G) form (ifname + source +
group + up to MAX_MC_VIFS outbound interfaces). For a (*,G) route,
where no source is given, only two words (ifname, group) are consumed
before the loop instead of three, so the same CMD_MAX_WORDS budget
lets one extra outbound interface through and the loop writes
MAX_MC_VIFS + 1 pointers into the array, one past its end.
This is reachable with an entirely ordinary command, e.g.
smcroutectl add eth0 239.1.1.1 vif0 vif1 ... vif32
(33 outbound interfaces on a group-only route), no malformed input
required. I confirmed the overflow by building the daemon with
AddressSanitizer and sending that exact request over its IPC socket;
ASan reports a stack-buffer-overflow in do_mroute() with the write
landing exactly one pointer past the 32-slot array. With the fix in
place the same request is cleanly rejected (EINVAL) and the daemon
keeps running, while both the (*,G) and (S,G) forms still work
correctly at their real maximum of MAX_MC_VIFS outbound interfaces.
The fix rejects the request up front, once pos is known, if the
number of outbound-interface words left would exceed MAX_MC_VIFS.
Signed-off-by: yi chen <94xhn1@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens IPC command handling for multicast route configuration by preventing a stack buffer overflow in do_mroute() when processing (*,G) routes with too many outbound interfaces.
Changes:
- Add an upper-bound check to reject IPC requests where the remaining argument count would overflow the fixed-size
out[MAX_MC_VIFS]array indo_mroute()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+117
to
+120
| if (msg->count - pos > MAX_MC_VIFS) { | ||
| errno = EINVAL; | ||
| return 1; | ||
| } |
Owner
|
Thanks for taking the time to report and fix this, much appreciated! |
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
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.
While looking at the IPC message handling in
src/msg.cI noticeddo_mroute()copies the trailing outbound-interface words into a fixed-size array without checking that they actually fit:The only limit ever placed on
msg->countisCMD_MAX_WORDSinipc.c'sipc_parse(), and that constant is sized for the(S,G)form of the command (ifname + source + group + up to MAX_MC_VIFS outbound interfaces, i.e. 3 fixed words). But for a(*,G)route, i.e. no explicit source (the "second argument is optional" form documented at the top ofmsg.h),do_mroute()only consumes 2 fixed words before the copy loop instead of 3. Sinceipc_parse()doesn't know or care which form is being used, it happily accepts a message with one more outbound interface than the array can actually hold, and the loop writesMAX_MC_VIFS + 1pointers into aMAX_MC_VIFS-sized array.This isn't some contrived edge case, it's reachable with a perfectly ordinary command:
(33 outbound interfaces on a group-only route). I verified this against the real daemon: built it with AddressSanitizer and sent that exact request over its IPC socket. ASan reports a stack-buffer-overflow in
do_mroute(), with the offending write landing exactly one pointer past the end ofout. After applying the fix the same request is cleanly rejected withEINVALand the daemon keeps running, and I re-checked that both the(*,G)and(S,G)forms still work at their real maximum ofMAX_MC_VIFSoutbound interfaces (no regression).The fix just rejects the request up front, once
posis known, if the remaining word count would overflow the array.