Re: [PATCH bpf-next v4 3/3] bpftool: Fix bypass of the batch line length check by comments

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for batch line length check bypass, allowing command injection via embedded null bytes.
--

--- Patch [3]: [PATCH bpf-next v4 3/3] bpftool: Fix bypass of the batch line length check by comments ---
commit 1803699d659bb5e5e3beb9beeb6577cdf2791dda
Author: Yuan Chen <[email protected]>

bpftool: Fix bypass of the batch line length check by comments

This commit fixes an issue in do_batch() where trailing comments truncated
by '#' would bypass the line length check. It records whether fgets()
truncated the line before stripping the comment to reliably reject overlong
lines regardless of comments.

> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -367,11 +367,13 @@ static int do_batch(int argc, char **argv)
>  	if (json_output)
>  		jsonw_start_array(json_wtr);
>  	while (fgets(buf, sizeof(buf), fp)) {
> +		bool truncated = strlen(buf) == sizeof(buf) - 1;

[Severity: High]
This isn't a bug introduced by this patch, but does using strlen() here leave
the length check vulnerable to an embedded null byte bypass?

Since fgets() reads up to sizeof(buf) - 1 bytes, if a batch file line
contains an embedded null byte ('\0'), strlen(buf) will stop at the null
byte. This makes strlen(buf) strictly less than sizeof(buf) - 1, causing the
truncation check to evaluate to false even if the line exceeded the buffer
capacity.

If this bypasses the length check, could the unread remainder of the
buffer-exceeding line remain in the file stream and be parsed and executed
as a separate, arbitrary command on the next loop iteration? 

Might it be safer to check if the buffer is missing a newline character
instead of relying on strlen()?

[ ... ]

>  		/* Append continuation lines if any (coming after a line ending
>  		 * with '\' in the batch file).
>  		 */
>  		while ((cp = strstr(buf, "\\\n")) != NULL) {
> +			bool cont_truncated;
> +
>  			if (!fgets(contline, sizeof(contline), fp) ||
>  			    strlen(contline) == 0) {
[ ... ]
>  			}
>  
> +			cont_truncated = strlen(contline) == sizeof(contline) - 1;

[Severity: High]
This is also a pre-existing issue, but does the same embedded null byte
bypass apply to continuation lines here?

If contline contains a null byte, cont_truncated will evaluate to false,
potentially allowing an overlong continuation line to bypass the length
enforcement and execute trailing input as unintended commands.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3