> Module Name: src
> Committed By: christos
> Date: Wed Mar 26 13:52:47 UTC 2025
>
> Modified Files:
> src/external/bsd/blocklist/lib: bl.c
>
> Log Message:
> NUL-terminate the message string (thanks riastradh)
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.5 -r1.6 src/external/bsd/blocklist/lib/bl.c
>
> @@ -543,6 +543,7 @@ bl_recv(bl_t b)
> else {
> rem = MIN(sizeof(bi->bi_msg), rem + 1);
> strlcpy(bi->bi_msg, ub.bl.bl_data, rem);
> + bi->bi_msg[sizeof(bi->bi_msg) - 1] = '\0';
> }
This doesn't fix the issue, and doesn't do anything else useful
either.
The _input_ to strlcpy is absolutely required to be a NUL-terminated
string, even if it is longer than the length rem that you passed.
(This is part of why strlcpy is so dangerous as a putative replacement
for strncpy!)
The input here, ub.bl.bl_data, is read from over the network, so it is
controlled by the peer. Normally, this means you can make no
assumptions about whether it is NUL-terminated. Review of bl_send
suggests that it is _never_ NUL-terminated even if the peer is
trusted. The issue I reported in PR 59218 is that this code assumes
the input is NUL-terminated.
The fix I suggested is to either use strncpy or NUL-terminate the
_input_:
> 3. Use strncpy instead of strlcpy (and maybe NUL-terminate the
> output if needed), or NUL-terminate the input.
But you've NUL-terminated the _output_, which doesn't help to avoid a
buffer overrun if the input isn't NUL-terminated, and doesn't serve
any useful function of the input is NUL-terminated.
(Also please cite the PR in the commit message next time!)
On closer inspection, of this logic and of the corresponding bl_send
logic, I think what you really want to do here is neither strncpy nor
strlcpy but memcpy:
rem = MIN(sizeof(bi->bi_msg) - 1, rem);
memcpy(bi->bi_msg, ub.bl.bl_data, rem);
bi->bi_msg[rem] = '\0';
