Ian Lance Taylor wrote:
>> You have just seen somebody who can be considered an expert in
>> matters of writing C sofware come up with a check that looks
>> correct, but is broken under current gcc semantics. That should
>> make you think.
> I'm not entirely unsympathetic to your arguments, but, assuming you
> are referring to the code I wrote three messages up, this comment is
> unfair. The code I wrote was correct and unbroken.
It might not have been broken in a world where you would have written a
requirements specification to exclude MAX_INT as a legal value for
vp->len, plus tests in the upper layer to enforce that, plus an audit
team to verify that anybody who creates that structs takes care not to
send you a MAX_INT.
In the absence of that, I would believe that reading code like:
struct s { int len; char* p; };
inline char
bar (struct s *sp, int n)
{
if (n < 0)
abort ();
if (n > sp->len)
abort ();
return sp->p[n];
}
would make you think that calling bar is safe with any n, as long as
sp->p points to some allocated memory of sp->len bytes. In fact, if
anybody ever did security audits of code, and wouldn't let the above
code pass as "prevents bad things from happening for invalid n, as long
as vp is sound", please raise your hand now.
> You suggest that it is broken because an attacker could take control
> of vp->len and set it to INT_MAX. But that just means that code in
> some other part of the program must check user input and prevent that
> from occurring.
> In fact, given the proposed attack of extract data from memory,
> INT_MAX is a red herring; any value larger than the actual memory
> buffer would suffice to read memory which should not be accessible.
Oh, I might not have sufficiently gone into detail about what I am
meaning by "controlling the value." I am of course assuming that the
input layer does proper input validation. Controlling vp->len means
nothing more than sending enough input to the system that I know will be
managed by a struct v. Take for instance some protocol that expects me
to send the length of some datum, and then the announced number of bytes
after that. ASN.1 BER encoding works like this, for instance.
I'll just send INT_MAX data to the application, and vp->len will be INT_MAX.
> I think a better way to describe your argument is that the compiler
> can remove a redundant test which would otherwise be part of a
> defense in depth. That is true. The thing is, most people want the
> compiler to remove redundant comparisons; most people don't want
> their code to have defense in depth, they want it to have just one
> layer of defense, because that is what will run fastest. We gcc
> developers get many more bug reports about missed optimizations than
> we do about confusing language conformant code generation.
No. My argument is that the test that is being removed is not redundant:
it simply *is* your single layer of defense. Imagine the network
application sketched around your code snippet: somebody might have
carefully verified that any write access to a struct v maintains the
invariance of that structure, that any read access goes through bar(),
and on top of that that bar() properly checks the bounds of the array.
He's very proud of his security architecture (for a reason, this is way
more than your average C programmer will do), but of course he knows
that his users expect performance, so he'll make the accessor function
inline, and adds no further checks, as he has convinced himself that
they are not needed.
And then he has a bad day, and needs to add some unloved reporting
module, and writes code like:
void
foo (struct s *sp, int n)
{
int len = sp->len;
int i;
int tot = 0;
for (i = 0; i <= len; ++i)
tot += bar (sp, i);
return tot;
}
which happens to trigger an endless loop on len==INT_MAX. And let's
face it, everybody has bugs in his code. So he gives the code a second
glance, to see whether there are any security risks lurking there. He
still doesn't see his bug, but figures, alright, this calls the safe
accessor function, nothing bad will happen. And starts working on the
next function.
But since he wrote code that's undefined under the C standard, gcc
figures it is ok to break the single line of defense that was in his
code. I think this is not acceptable. I need to be able to rely on the
fact that an expression like
if (n < 0) abort();
will actually abort if n is negative to stand any chance of ever writing
reasonably secure code. Instead of having to find the bugs in a few
critical places, I have to make sure that my whole program doesn't
trigger some unknown behaviour somewhere.
Regarding the number of bug reports you get: everybody understands why
performance is important, so everybody complains about it. The number
of people who sufficiently understand what the compiler is doing is much
smaller, so less people complain about it. That doesn't mean their
arguments are less valid, or even that the topics they address aren't
important for any user of gcc or its build results.
> One simple way to avoid problems in which the compiler removes
> redundant tests: compile without optimization. Another simple way:
> learn the language semantics and think about them.
You're kidding. Apart from the fact that the test is not redundant, and
actually prevents your computer from being used as a spam relay for all
kinds of nasty stuff: performance is not optional. But there is a
tradeoff between performance and security, the more you tweak the
performance buttons, the more likely that you turn small bugs into
security desasters. And it is well within your responsibility to choose
wisely with the default settings.
Oh, and teaching all of the programmers out there all the subtle nuances
of C and trying to get them to write proper code: good luck. That
simply won't happen.
> In any case, later today I hope to send out a patch for the
> -fstrict-overflow option.
That is very much appreciated.
Andreas
