netdev - Re: [PATCH] bpf: btf: Fix a missing check bug

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]

Date:   Mon, 8 Oct 2018 23:55:15 -0700
From:   Song Liu <liu.song.a23@...il.com>
To:     valdis.kletnieks@...edu
Cc:     wang6495@....edu, kjlu@....edu,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] bpf: btf: Fix a missing check bug

On Mon, Oct 8, 2018 at 6:07 PM <valdis.kletnieks@...edu> wrote:
>
> On Mon, 08 Oct 2018 17:44:46 -0700, Song Liu said:
>
> > I think I get the security concept here. However, hdr_len here is only used to
> > copy the whole header into kernel space, and it is not used in other
> > logic at all.
> > I cannot image any security flaw with either hdr_len > btf->hdr->hdr_len case or
> > hdr_len < btf->hdr->hdr_len. Could you please provide more insights on what
> > would break by malicious user space?
>
> Say the biggest allowed value for hdr_len is 128.  We check the value, the user has 98.
> They then stuff 16,383 into there.
>
> Now here's the problem - hdr_len is a local variable, and evaporates when the function
> returns.  From here on out, anybody who cares about the header length will use the
> value in btf->hdr_len....
>
> (And yes, somebody *does* care about the length, otherwise we wouldn't need a field
> saying what the length was....)
>
> Now think how many ways that can go pear-shaped.  You copied in 98 bytes, but outside
> the function, they think that header is almost 4 pages long.  Does that ever get used as
> a length for kmemcpy()?  Or a limit for a 'for (i=start; i< (start+hdr->hdr_len); i++)' that
> walks across a variable length header?
>
> Can you cook up a way to have a good chance to oops the kernel when it walks off the
> page you allocated the 98 bytes on?  Can you use it to export chunks of memory out to
> userspace?  Lots and lots of ways for this to kersplat a kernel...;

In current code, I don't thing any malicious hdr_len value could pass
btf_check_sec_info().
On the other hand, I agree this is a good-to-have check.

Acked-by: Song Liu <songliubraving@...com>