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

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

Pre-existing issues:
- [Medium] The patch fixes the batch line length check bypass in the main buffer but misses the identical vulnerability in continuation lines, allowing command smuggling.
- [Medium] The newly introduced `truncated` check relies on `strlen()`, which can be bypassed by injecting a null byte, re-enabling the command smuggling vulnerability.
--

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

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

This commit attempts to fix a command smuggling vulnerability where overlong
lines containing a '#' character bypass the buffer length check. It records
whether fgets() truncated the line before stripping the comment to ensure
overlong lines are properly rejected.

> 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: Medium]
This isn't a bug introduced by this patch, but rather a pre-existing issue.
Can the new truncated check be bypassed if the input contains a null byte?

Since fgets() reads up to sizeof(buf) - 1 bytes without stopping at null
bytes, but strlen() stops at the first null byte, wouldn't an overlong line
with an injected null byte result in a smaller strlen(buf)?

If that happens, wouldn't the truncated check evaluate to false, bypassing the
line limit and leaving the remainder of the overlong line in the file stream
to be parsed as a separate command?

> +
>  		cp = strchr(buf, '#');
>  		if (cp)
>  			*cp = '\0';
>  
> -		if (strlen(buf) == sizeof(buf) - 1) {
> +		if (truncated) {
>  			line_too_long = true;
>  			break;
>  		}

[Severity: Medium]
This is a pre-existing issue and was not introduced by the patch under review.
Does the same command smuggling vulnerability still exist for continuation
lines?

Looking further down in do_batch(), continuation lines are handled similarly:

tools/bpf/bpftool/main.c:do_batch() {
    ...
    while ((cp = strstr(buf, "\\\n")) != NULL) {
        if (!fgets(contline, sizeof(contline), fp) || ...)
            ...
        cp = strchr(contline, '#');
        if (cp)
            *cp = '\0';

        if (strlen(buf) + strlen(contline) + 1 > sizeof(buf)) {
            p_err("command %u is too long", lines);
    ...
}

If a continuation line exceeds the buffer size and contains a '#' character
within the first 4095 bytes, it will be prematurely truncated before the
length check.

Because this artificially reduces strlen(contline), doesn't this bypass the
bounds check and leave the unread remainder of the overlong continuation line
in the file stream to be executed as an unintended command?

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