Message-ID: <20070121001947.932.0@paddy.troja.mff.cuni.cz>
Date: Sun, 21 Jan 2007 01:20:05 +0100 (CET)
From: Pavel Kankovsky <peak@...o.troja.mff.cuni.cz>
To: Felix von Leitner <felix-fulldisclosure@...e.de>
Cc: full-disclosure@...ts.grok.org.uk
Subject: Re: Major gcc 4.1.1 and up security issue

On Mon, 15 Jan 2007, Felix von Leitner wrote:

>   int foo(int a) {
>     assert((int)(a+100) > 0);
>     printf("%d %d\n",a+100,a);
>     return a;
>   }

The assert() here is quite different from the code you published on the 
web where the condition reads (a+100 > a).

> Now you might think that it's just assert, we use if and we are safe.
> No, assert() is just a macro that turns into if.  The whole assert code
> gets removed here, you won't see a trace of the whole overflow check in
> the disassembly.

Let's have a look at the code with ((int)(a+100) > 0) because it will 
provide a better insight into the problem.

Here is the i386 machine code produced by GCC 4.1.1 without any options:

080483d4 <foo>:
 80483d4:       55                      push   %ebp
 80483d5:       89 e5                   mov    %esp,%ebp
 80483d7:       83 ec 18                sub    $0x18,%esp
 80483da:       83 7d 08 9c             cmpl   $0xffffff9c,0x8(%ebp)  (1)
 80483de:       7f 24                   jg     8048404 <foo+0x30>     (2)
 80483e0:       c7 44 24 0c 08 85 04    movl   $0x8048508,0xc(%esp)   (3)
 80483e7:       08 
 80483e8:       c7 44 24 08 05 00 00    movl   $0x5,0x8(%esp)
 80483ef:       00 
 80483f0:       c7 44 24 04 0c 85 04    movl   $0x804850c,0x4(%esp)
 80483f7:       08 
 80483f8:       c7 04 24 10 85 04 08    movl   $0x8048510,(%esp)
 80483ff:       e8 f0 fe ff ff          call   80482f4 <__assert_fail@plt>
 8048404:       8b 55 08                mov    0x8(%ebp),%edx         (4)
 8048407:       83 c2 64                add    $0x64,%edx
 804840a:       8b 45 08                mov    0x8(%ebp),%eax
 804840d:       89 44 24 08             mov    %eax,0x8(%esp)
 8048411:       89 54 24 04             mov    %edx,0x4(%esp)
 8048415:       c7 04 24 21 85 04 08    movl   $0x8048521,(%esp)
 804841c:       e8 f3 fe ff ff          call   8048314 <printf@plt>
 8048421:       8b 45 08                mov    0x8(%ebp),%eax
 8048424:       c9                      leave  
 8048425:       c3                      ret    

Instructions marked with (1) and (2) implement the conditional branching.
Instructions starting at (3) call __assert_fail().
This is the assert().

The check is interesting. It does not look as the original condition,
ie. ((int)(a+100) > 0) at the first glance. Let's examine it:

- (1) compares the value of a with 0xffffff9c = -100
- (2) jumps to (4) if the value of a was greater than -100

In other words, the compiler subtracted 100 from both sides of 
the comparison and translated it into (a > -100).

This optimization (*) is ok as long as no overflow occurs during the
evaluation of the original condition. It modifies its semantics when 
integer overflows are involved but this is acceptable because the result 
of an overflowing arithmetic operation on signed integers is undefined.

A program expecting (a+100) <= 0 when a is signed int and a >= INT_MAX-100 
is and has always been broken.

> I opened a gcc bug for this.  They told me that the C standard says
> integer overflow for signed integers in undefined and therefore gcc is
> right in doing this.

Let's return to assert(a+100 > a). Now it is obvious what happened:
the compiler subtracted a from both sides of the comparison and got
100 > 0. This condition is always true therefore it optimized the
assert() away (**).

Again, the program is and has always broken because it relies on
undefined behaviour.


(*) It is somewhat odd the compiler does it without any -On option 
for n > 0. It is probably considered to be a kind of constant folding.

(**) We can see dead code elimination is another kind of optimization GCC 
performs without asking.


> I'm saying this will break tons of security checks in existing
> applications and will get people to get 0wned.  Please help make the gcc
> people fix this!

Helping people fix their broken code and teach them how to write 
correct code might be more productive imho. :P


--Pavel Kankovsky aka Peak  [ Boycott Microsoft--http://www.vcnet.com/bms ]
"Resistance is futile. Open your source code and prepare for assimilation."










_______________________________________________
Full-Disclosure - We believe in it.
Charter: http://lists.grok.org.uk/full-disclosure-charter.html
Hosted and sponsored by Secunia - http://secunia.com/

Powered by blists - more mailing lists