Message-ID: <1535960372.32005.1.camel@med.uni-goettingen.de>
Date:   Mon, 3 Sep 2018 07:39:31 +0000
From:   "Uecker, Martin" <Martin.Uecker@....uni-goettingen.de>
To:     "keescook@...omium.org" <keescook@...omium.org>
CC:     "torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: VLAs and security

Am Sonntag, den 02.09.2018, 10:40 -0700 schrieb Kees Cook:
> On Sun, Sep 2, 2018 at 1:08 AM, Uecker, Martin
> <Martin.Uecker@....uni-goettingen.de> wrote:
> > I do not agree that VLAs are generally bad for security.
> > I think the opposite is true. A VLA with the right size
> > allows the compiler to automatically perform or insert
> > meaningful bounds checks, while a fixed upper bound does not.
> 
> While I see what you mean, the trouble is that the compiler has no
> idea what the upper bounds of the _available_ stack is. This means
> that a large VLA might allow code to read/write beyond the stack
> allocation, which also bypasses the "standard" stack buffer overflow
> checks. Additionally, VLAs bypass the existing stack-size checks we've
> added to the kernel.

Limiting the size of the VLA should be sufficient to avoid this.

I don't know about your specific stack-size checks
in the kernel, but for general programming, the full solution
is for the compiler to probe the stack when growing.

But I was not talking about the bounds of the stack, but of the
array itself.

> > For example:
> > 
> > char buf[N];
> > buf[n] = 1;
> > 
> > Here, a compiler / analysis tool can for  n < N  using
> > static analysis or insert a run-time check.
> > 
> > Replacing this with
> > 
> > char buf[MAX_SIZE]
> > 
> > hides the information about the true upper bound
> > from automatic tools.
> 
> While this may be true for some tools, I don't agree VLAs are better
> in general. For example, the compiler actually knows the upper bound
> at build time now, and things like the printf format size checks and
> CONFIG_FORTIFY_SOURCE are now able to produce compile-time warnings
> (since "sizeof(buf)" isn't a runtime value). With a VLA, this is
> hidden from those tools, and detection depends on runtime analysis.

If the correct bound is actually a constant and the array
only ends up being a VLA for some random reason, I fully agree.

But if the true bound is smaller, then IMHO it is really bad advise
to tell programmers to use

char buf[MAX_SIZE]

instead of something like

assert(N <= MAX_SIZE); 
char buf[N]

because then errors of the form 

buf[n] = 1

with N < n < MAX_SIZE can not be detected anymore. Also the
code usually ends up being less readable, which is also a clear
disadvantage in my opinion.


> It should be noted that VLAs are also slow[1], so removing them not
> only improves robustness but also improves performance.

I have to admit that I am always a bit skeptical if somebody makes
generic claims such as "VLAs are slow" and then cites only a
single example. But I am not too surprised if compilers produce
crappy code for VLAs and that this can hurt performance in some
examples. But compared to dynamic allocation VLAs should be much
faster. They also reduce stack usage compared to always allocating
array with a fixed maximum size on the stack.

> > Of course, having predictable stack usage might be more
> > important in the kernel and might be a good argument
> > to still prefer the constant bound.
> 
> Between improved compile-time checking, faster runtime performance,
> and improved robustness against stack exhaustion, I strongly believe
> the kernel to be better off with VLAs entirely removed. And we are
> close: only 6 remain (out of the 115 I counted in v4.15).

Looking at some of the patches, I would say it is not 
clear to me that this is alway an improvement.

> > But loosing the tighter bounds is clearly a disadvantage
> > with respect to security that one should keep it mind.
> 
> Yes: without VLAs, stack array usage is reduced to "standard" stack
> buffer overflow concerns. Removing the VLA doesn't introduce a new
> risk: we already had to worry about fixed-size arrays. Removing VLAsalways
> means we don't have to worry about the VLA-specific risks anymore.

It introduces the new risk that certain logic error can
not be detected anymore by static analysis or run-time bounds
checking.

Best,
Martin

Powered by blists - more mailing lists