Bugs in gnupg
From Ggl's wiki
Contents |
Fefe (Felix von Leitner) found some interesting bugs in gnupg.
Full-disclosure - new class of printf issue: int overflow
This is about two issues.
abs within vasprintf
I just read some gnupg source code and stumbled upon their vasprintf implementation. Basically they make one pass over the format string to find out how much memory to malloc, and then they call sprintf on the malloced buffer.
Here is an excerpt:
if (*p == '*')
{
++p;
total_width += abs (va_arg (ap, int));
}
I noticed this code because it calls abs on an int. A little known fact about abs is that it can return negative values. If the int is 0x80000000, for example, abs() will also return 0x80000000. So, I thought, mhh, if someone can control this value but not the format string, he can cause total_width to be very small but then write lots of stuff to it.
int overflow in *printf
But that got me thinking. *printf return an int, and it's supposed to be the number of chars written. So a typical idiom is
size_t memory_needed=snprintf(NULL,0,format_string,...); char* ptr=malloc(memory_needed+1); sprintf(ptr,format_string,...);
What if sprintf returns a negative value? printf can return -1 if write() failed, but sprintf can not traditionally return -1. The single unix specification says this about sprintf:
If the value of n is zero on a call to snprintf(), nothing shall be
written, the number of bytes that would have been written had n been
sufficiently large excluding the terminating null shall be returned, and
s may be a null pointer.
So my guess is that nobody expects sprintf to return a negative value. Out of curiosity, I wrote this test program:
$ cat > t.c
#include <stdio.h>
int main() {
printf("%d\n",snprintf(0,0,"%*d %*d",0x40000000,1,0x40000000,1));
}
$ gcc -o t t.c
$ ./t
-2147483647
./t 17.02s user 0.03s system 99% cpu 17.161 total
$
the second line comes from my zsh, and as you can see running this program took 17 seconds. top shows that the process used 1 gig of memory while it ran. :-)
This scenario is pretty far fetched, obviously. In the above code snippet, returning a negative number doesn't hurt, because malloc will interpret it as unsigned really big number. But hey, there could be other scenarios.
The question is: do we want to do something about it? What should printf do if it detects an int overflow? Return -1? Is there a good solution to this? Solaris apparently returns -1.
Felix
PS: Does anyone understand why sprintf does not count the \0 at the end? I think that's pretty brain-dead.
Full-disclosure - gnupg diff available
Hi!
I did a gnupg audit recently. I was, frankly, appalled by the code quality. It is a desert of pointer manipulation, string copying, memcpy and strcpy are used all over the place, and sprintf, too.
You can find my diff at [1]
Please note that
- a) I might have missed something
- b) I don't claim all fixed parts were exploitable. My attention span
is quite short. If I couldn't figure out where data is coming from
in 30 seconds, I assumed it was user settable and put a fix in.
- c) I focused on write accesses. I probably missed a few out of bounds
read accesses. Crashing seems to be an acceptable error handling
mechanism in gnupg (there are already lots of assert() and BUG()
calls) so I also used assert() in most cases.
People are always saying how free and open source software is more secure because so many eyes look at it... well, I didn't see much evidence of that in gnupg. Please do use some of your time to actually look at source code and find bugs.
I tried to give Werner Koch (the author) advance warning, but he was neither helpful nor did he appear interested. So please don't make 0-days out of this.
Thank you,
Felix
Full-disclosure - Major gcc 4.1.1 and up security issue
So, in my gnupg diff, I used code like this:
assert(a+100 > a);
with a being an int. Here, have this example code:
#include <assert.h>
#include <stdio.h>
int foo(int a) {
assert((int)(a+100) > 0);
printf("%d %d\n",a+100,a);
return a;
}
int main() {
foo(100);
foo(0x7fffffff);
}
(Also available as http://ptrace.fefe.de/int.c)
Now, if you compile this on a system where int is 32-bit (i.e. almost anywhere these days), you might expect the assert to trigger in the second call to foo. Not so:
200 100 -2147483549 2147483647
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.
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.
I found the same issue with gcc regarding pointers. I found it with gcc 4.1, they told me the same story and fixed it for gcc 4.1.1. At least with pointers there is a workaround: you could cast the pointer to a ptrint_t, add the 100, and then check if it became smaller. But with signed it, the portable workaround would be a monstrosity like:
assert((((unsigned int)a)<<1)+100 > (((unsigned int)a)<<1));
or am I overlooking something?
I'm not saying that the gcc people are wrong with their legalese answer. 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!
Felix
PS: I think this is the same base problem and thus starts with gcc 4.1, but I can only say for sure it happens with gcc 4.1.1.
