Re: CVS commit: src/external/bsd/blocklist/lib

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> 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';