[PATCH] bpf: btf: Fix a missing check bug

[PATCH] bpf: btf: Fix a missing check bug

[Wenwen Wang]

In btf_parse_hdr(), the length of the btf data header is firstly copied
from the user space to 'hdr_len' and checked to see whether it is larger
than 'btf_data_size'. If yes, an error code EINVAL is returned. Otherwise,
the whole header is copied again from the user space to 'btf->hdr'.
However, after the second copy, there is no check between
'btf->hdr->hdr_len' and 'hdr_len' to confirm that the two copies get the
same value. Given that the btf data is in the user space, a malicious user
can race to change the data between the two copies. By doing so, the user
can provide malicious data to the kernel and cause undefined behavior.

This patch adds a necessary check after the second copy, to make sure
'btf->hdr->hdr_len' has the same value as 'hdr_len'. Otherwise, an error
code EINVAL will be returned.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 kernel/bpf/btf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 2590700..7cce7db 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -2114,6 +2114,9 @@ static int btf_parse_hdr(struct btf_verifier_env *env, void __user *btf_data,
 
 	hdr = &btf->hdr;
 
+	if (hdr->hdr_len != hdr_len)
+		return -EINVAL;
+
 	btf_verifier_log_hdr(env, btf_data_size);
 
 	if (hdr->magic != BTF_MAGIC) {
-- 
2.7.4

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

[Song Liu]

On Sun, Oct 7, 2018 at 1:26 PM Wenwen Wang <wang6495@umn.edu> wrote:
>
> In btf_parse_hdr(), the length of the btf data header is firstly copied
> from the user space to 'hdr_len' and checked to see whether it is larger
> than 'btf_data_size'. If yes, an error code EINVAL is returned. Otherwise,
> the whole header is copied again from the user space to 'btf->hdr'.
> However, after the second copy, there is no check between
> 'btf->hdr->hdr_len' and 'hdr_len' to confirm that the two copies get the
> same value. Given that the btf data is in the user space, a malicious user
> can race to change the data between the two copies. By doing so, the user
> can provide malicious data to the kernel and cause undefined behavior.
>
> This patch adds a necessary check after the second copy, to make sure
> 'btf->hdr->hdr_len' has the same value as 'hdr_len'. Otherwise, an error
> code EINVAL will be returned.

These two numbers are copied from same memory location, right? So I
think this check is not necessary?

Thank,
Song

>
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  kernel/bpf/btf.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 2590700..7cce7db 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -2114,6 +2114,9 @@ static int btf_parse_hdr(struct btf_verifier_env *env, void __user *btf_data,
>
>         hdr = &btf->hdr;
>
> +       if (hdr->hdr_len != hdr_len)
> +               return -EINVAL;
> +
>         btf_verifier_log_hdr(env, btf_data_size);
>
>         if (hdr->magic != BTF_MAGIC) {
> --
> 2.7.4
>

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

[valdis.kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Mon, 08 Oct 2018 13:51:09 -0700, Song Liu said:
> On Sun, Oct 7, 2018 at 1:26 PM Wenwen Wang <wang6495@umn.edu> wrote:

> > same value. Given that the btf data is in the user space, a malicious user
> > can race to change the data between the two copies. By doing so, the user
> > can provide malicious data to the kernel and cause undefined behavior.

> These two numbers are copied from same memory location, right? So I
> think this check is not necessary?

Security researchers call this a TOCTOU bug - Time of Check - Time of Use.

What can happen:

1) We fetch the value  (say we get 90) from userspace and stash it in hdr_len.

2) We do some other stuff like check the hdr_len isn't too big, etc..

        meanwhile, on another CPU running another thread of the process...
                3) malicious code stuffs a 117 into that field

4) We fetch the entire header, incliding a now-changed hdr_len  (now 117) and
stick it in btf->hdr->hdr_len.

5) Any code that assumes that hdr_len and btf->hdr->hdr_len are the same value
explodes in interesting ways.



[-- Attachment #2: Type: application/pgp-signature, Size: 486 bytes --]

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

[Song Liu]

On Mon, Oct 8, 2018 at 2:17 PM <valdis.kletnieks@vt.edu> wrote:
>
> On Mon, 08 Oct 2018 13:51:09 -0700, Song Liu said:
> > On Sun, Oct 7, 2018 at 1:26 PM Wenwen Wang <wang6495@umn.edu> wrote:
>
> > > same value. Given that the btf data is in the user space, a malicious user
> > > can race to change the data between the two copies. By doing so, the user
> > > can provide malicious data to the kernel and cause undefined behavior.
>
> > These two numbers are copied from same memory location, right? So I
> > think this check is not necessary?
>
> Security researchers call this a TOCTOU bug - Time of Check - Time of Use.
>
> What can happen:
>
> 1) We fetch the value  (say we get 90) from userspace and stash it in hdr_len.
>
> 2) We do some other stuff like check the hdr_len isn't too big, etc..
>
>         meanwhile, on another CPU running another thread of the process...
>                 3) malicious code stuffs a 117 into that field
>
> 4) We fetch the entire header, incliding a now-changed hdr_len  (now 117) and
> stick it in btf->hdr->hdr_len.
>
> 5) Any code that assumes that hdr_len and btf->hdr->hdr_len are the same value
> explodes in interesting ways.

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?

Thanks,
Song

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

[valdis.kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 486 bytes --]

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

[Song Liu]

On Mon, Oct 8, 2018 at 6:07 PM <valdis.kletnieks@vt.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@fb.com>

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

[Alexei Starovoitov]

On Mon, Oct 08, 2018 at 11:55:15PM -0700, Song Liu wrote:
> On Mon, Oct 8, 2018 at 6:07 PM <valdis.kletnieks@vt.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@fb.com>

I agree with Song's analysis that the current shape of code in
btf_check_sec_info() has us covered, but it's a good coding style
to check for TOCTOU like this, hence applied to bpf-next.