bin/59218: blocklistd(8): possible fd leak and buffer overruns in bl_recv

[campbell+netbsd%mumble.net@localhost]

>Number:         59218
>Category:       bin
>Synopsis:       blocklistd(8): possible fd leak and buffer overruns in bl_recv
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Mar 26 14:00:00 +0000 2025
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
The NulBSD Blocklistation
>Environment:
>Description:
In bl.c 1.4, the following logic appears:

   438		union {
   439			char ctrl[CMSG_SPACE(sizeof(int)) + CMSG_SPACE(CRED_SIZE)];
   440			uint32_t fd;
   441		} ua;
...
   446		union {
   447			bl_message_t bl;
   448			char buf[512];
   449		} ub;
...
   458		iov.iov_base = ub.buf;
   459		iov.iov_len = sizeof(ub);
...
   467		msg.msg_control = ua.ctrl;
   468		msg.msg_controllen = sizeof(ua.ctrl) + 100;
   469	
   470	        rlen = recvmsg(b->b_fd, &msg, 0);
...
   485			case SCM_RIGHTS:
   486				if (cmsg->cmsg_len != CMSG_LEN(sizeof(int))) {
   487					bl_log(b, LOG_ERR,
   488					    "%s: unexpected cmsg_len %d != %zu",
   489					    __func__, cmsg->cmsg_len,
   490					    CMSG_LEN(2 * sizeof(int)));
   491					continue;
   492				}
   493				memcpy(&bi->bi_fd, CMSG_DATA(cmsg), sizeof(bi->bi_fd));
   494				got |= GOT_FD;
   495				break;
...
   545			strlcpy(bi->bi_msg, ub.bl.bl_data, rem);

There are several issues here:

1. For reasons unclear to me, on line 468, this passes a control data buffer to the kernel that's 100 bytes longer than has been allocated on the stack, so the kernel will gladly overrun the buffer if the peer sends too much control data:

   467		msg.msg_control = ua.ctrl;
   468		msg.msg_controllen = sizeof(ua.ctrl) + 100;

   (Whisky tango foxtrot?)

2. Nothing prevents a peer from sending more than one file descriptor through the cmsg(3) API, but the logic on lines 485-491 just quietly ignores any excess file descriptors, leaking them.

3. Nothing in the ellipsis appears to NUL-terminate the buffer ub.bl.bl_data which is passed into strlcpy on line 545, but strlcpy absolutely requires its input to be NUL-terminated (one of the risks it introduces when naively replacing strncpy):

   545			strlcpy(bi->bi_msg, ub.bl.bl_data, rem);
>How-To-Repeat:
code inspection
>Fix:
1. Don't add 100 to msg_controllen.

2. Iterate over all the file descriptors in the cmsg record and close them.

3. Use strncpy instead of strlcpy (and maybe NUL-terminate the output if needed), or NUL-terminate the input.