linux-kernel - Re: [PATCH v4 02/10] user_events: Add minimal support for trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20211107233115.1f77e93c4bdf3ff649be99c1@kernel.org>
Date:   Sun, 7 Nov 2021 23:31:15 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Beau Belgrave <beaub@...ux.microsoft.com>
Cc:     rostedt@...dmis.org, linux-trace-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 02/10] user_events: Add minimal support for
 trace_event into ftrace

Hi Beau,

At first, thanks for breaking down your patch into this series!

Now I found that a suspicious security design issue in this patch.

On Thu,  4 Nov 2021 10:04:25 -0700
Beau Belgrave <beaub@...ux.microsoft.com> wrote:

> +
> +static enum print_line_t user_event_print_trace(struct trace_iterator *iter,
> +						int flags,
> +						struct trace_event *event)
> +{
> +	/* Unsafe to try to decode user provided print_fmt, use hex */
> +	trace_print_hex_dump_seq(&iter->seq, "", DUMP_PREFIX_OFFSET, 16,
> +				 1, iter->ent, iter->ent_size, true);

You said this is "Unsafe to try to decode user provided" here, because
this doesn't check the event data sanity, especially non-fixed size data.

However, it is not enough that you don't decode it here. Because synthetic
events (histograms) and event filters need to decode this recorded data entry
using the event format information.

This means this can cause a buffer overrun issue on the ring buffer, because
__data_loc (and __rel_loc too) can be compromised by the user.

If you want to just trace the user events with digit parameters, there is
a way to close this issue - support only the fixed size types (IOW, drop
__data_loc/rel_loc support) and always checks the 'length' of the written
data size. This ensures that those filters/synthetic events can access
those parameters as 'values'. Maybe eprobes still has to reject the user
events but the other parts will work well.

If you want to log some "string", it is hard. Maybe it needs a special
check when writing the event (address, length, and null termination.),
but it will increase the recording overhead.

Thank you,

-- 
Masami Hiramatsu <mhiramat@...nel.org>