Iljas Blag

… is usually a really dumb idea. You always tend to fuck something up. DON’T DO IT. Instead use strncpy, strlcpy, strncpy_s, or whatever boundschecking string copy function your libc provides. Obvioulsy this doesn’t mean your in the clear, but they are usually well documentent, and the documentation will usually tell you about any pitfalls it may have.

one of the most common mistakes with making your own string copy functions is that of forgetting that someone might call it with a length of 0, and you end up with an off-by-one overflow or underflow.

Let me demonstrate with ProFTPD code:

/* “safe” strncpy, saves room for at end of dest, and refuses to copy
* more than “n” bytes.
*/
char *sstrncpy(char *dest, const char *src, size_t n) {
register char *d = dest;

if (!dest)
return NULL;

if (src && *src) {
for (; *src && n > 1; n–)
*d++ = *src++;
}

*d = ”;

return dest;
}

if len is 0 in this case you still end up with *d = ”; and hence cause an off-by-one overflow.

So please, refrain from building your own string copy functions, coz you WILL fuck up !

So I just read the COPYRIGHT.c file in wu-ftpd. It states: ” Portions Copyright (c) 1983, 1995, 1996, 1997 Eric P. Allman.”

Why am I not surprised ?

I wonder what kernel memory looks like :)

Update:
NetBSD ptrace() information leak :)
see ftp://ftp.netbsd.org/pub/NetBSD/security/advisories/NetBSD-SA2006-025.txt.asc

http://www.cnn.com/2006/US/10/18/no.tag.ap/index.html

“Officials at an elementary school south of Boston have banned kids from playing tag, touch football and any other unsupervised chase game during recess for fear they’ll get hurt and hold the school liable.”

they go on to say:
“Recess is “a time when accidents can happen,” said Willett Elementary School Principal Gaylene Heppe, who approved the ban.”

You get a captain obvious award for that one (yea, that’s right, I stole the captain obvious awards thing from fefe !)

This is INSANE. Especially for this occasion I made an insane-o-meter in ms paint (YES, ms paint !)

http://www.apple.com/support/windowsvirus/

from the webpage:
“As you might imagine, we are upset at Windows for not being more hardy against such viruses, and even more upset with ourselves for not catching it.”

So they fucked up and try to blame it on microsoft.

That’s just grrrrrrrreat !

So I was reading some code and saw some really underhanded c code:

if(something);{
blah blah blah
}

yea, this stands out now, but it might not if you put it in between a few thousand lines of code !

Recently I was looking at how /dev/urandom was implemented in the linux kernel. So I looked at drivers/char/random.c. While I was reading that code I ran into the following:
…
static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
size_t nbytes)
{
ssize_t ret = 0, i;
__u8 tmp[EXTRACT_SIZE];
…
while (nbytes) {
…
extract_buf(r, tmp);
i = min_t(int, nbytes, EXTRACT_SIZE);
if (copy_to_user(buf, tmp, i)) { … }
…
}
…
}
…
static ssize_t
urandom_read(struct file * file, char __user * buf,
size_t nbytes, loff_t *ppos)
{
return extract_entropy_user(&nonblocking_pool, buf, nbytes);
}

urandom_read() is a direct read() hook that get’s called from vfs_read() (which in turn gets called from sys_read()). The bug here is pretty obvious. There is size_t truncation here:
i = min_t(int, nbytes, EXTRACT_SIZE);

At this point I was thinking “WTF ? this is wrong, how can this be in /dev/urandom code !!!!”. I remembered that there are some range checks in vfs_read() and hence I looked in vfs_read():

/*
* rw_verify_area doesn’t like huge counts. We limit
* them to something that fits in “int” so that others
* won’t have to do range checks all the time.
*/
#define MAX_RW_COUNT (INT_MAX & PAGE_CACHE_MASK)

int rw_verify_area(int read_write, struct file *file, loff_t *ppos, size_t count)
{
…
loff_t pos;

if (unlikely((ssize_t) count < 0))
goto Einval;
pos = *ppos;
if (unlikely((pos < 0) || (loff_t) (pos + count) MAX_RW_COUNT ? MAX_RW_COUNT : count;

Einval:
return -EINVAL;
}

ssize_t vfs_read(struct file *file, char __user *buf, size_t count, loff_t *pos)
{
ssize_t ret;
…
ret = rw_verify_area(READ, file, pos, count);
if (ret >= 0) {
count = ret;
…
ret = file->f_op->read(file, buf, count, pos);
…
}
return ret;
}

(actually I was told about this code by linus torvalds himself, after I reported a ppc specific bug in /proc. I believe his (almost) exact words were (I lost the email) “We added this code in 2.6. to prevent integer abuse in read/write hooks, I believe it got backported to 2.4 aswell”. And it did get backported to 2.4, I checked. The reason I stumbled on this bug (it’s in src/arch/ppc/kernel/htab.c proc_dol2crvec() btw, bonus points for whoever can spot signedness issue) because of a capture the flag game at HITB bahrain last year. The game got pretty boring so we decided to own one of the organisers laptop (hi Dhillon !) WITH PERMISSION! He was using an iBook with some linux distro on it, and using the 2.6.10 kernel at the time. That kernel didn’t have the range checks yet and hence the proc_dol2crvec quickly gave us read access to most kernel memory. We eventually ended up rooting it because of that bug. Which brings me to my point, that all of these horrible read/write bugs in drivers were in fact exploitable (and were exploited) on linux prior to just adding the range checking code in rw_verify_area and hence should still have gotten fixed !.)

Hm, wandered off there for a bit, let’s get back to the code in rw_verify_area(). What it basicly does is check for:
- size not negative
- offset nog negative
- size + offset not negative
and then TRUNCATES size_t -> signed integer, see the code comment.

This is shocking imo! But done for security reasons, which is certainly a noble thing. But this hides horrible bugs. So in fact it does the opposite. It sends out a message that “It’s ok to write crappy kernel code, we’ve got code in place to mitigate”. Anyone who ever looked at some of the drivers in the linux kernel knows there is a huge pile of low quality code in there, and a lot of copypasting is done (let’s call it infecting other code with low quality code). So even if you have crappy code that’s covered by the range checks and truncation in rw_verify_area() and vfs_read() someone might copypast your code in some place where’s it’s not covered by these range checks and you’ve got a big fat security hole on your hands. Not to mention that there might be another codepath to read() hooks in drivers thru some ioctls (where it doesn’t go thru vfs_read()).

What this means in reality is that on systems where sizeof(size_t) > sizeof(int) (mostly 64 bit architectures) that you only get to read or write at most 2 gigabytes of data (in a single call to read() or write()), even though you might want to read or write more then that. Which is INSANE! There is no need for this. Obviously you could try to make a case for this and state that you should do this in a loop of 2gig reads or write’s at a time. But this is kind of silly. Because it adds a needless loop (there is already such a loop in place in the kernel most of the time, in the /dev/urandom case for example) and more importantly it adds syscall overhead !

I’m sure by now some people are going “But you could use mmap, USE MMAP !” NO, you can’t. Hm, well, not always. See, there are non-regular files out there that don’t let you mmap. /dev/urandom for example, or /proc/pid/mem (Zalewksi++). But wait, it get’s worse, write() is covered by the same restrictions as read() in rw_verify_area() and therefor you can only write 2 gigs at a time. Not something you really want (thanks fefe).

I bet there are apps out there that actually break on this! there have to be, with all the crap that people put out there there’s gotta be 1 or 2 applications that go up in flames because of this!

if you want to add some range checks, then check if the count given is int truncation. It breaks functionality !

http://newsvac.newsforge.com/newsvac/06/10/07/130207.shtml
http://www.pine.nl/press/pine-cert-20030101.txt
enough said !

edit:
another one
http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/kern/sys_generic.c.diff?r1=1.150&r2=1.151&f=h

“Prevent IOC_IN with zero size argument (this is only supported
if backward copatibility options are present) from attempting
to free memory that wasn’t allocated. This is an old bug, and
previously it would attempt to free a null pointer. I noticed
this bug when working on the previous revision, but forgot to
fix it.

Security: local DoS
Reported by: Peter Holm
MFC after: 3 days”

free(uninit_ptr);
that’s not just a DoS :)

Since I’ve already gone 2 post without blogging about security I figured it’s time to do another security related one. It’ll be another this-api-is-so-broken post.

asprintf is basicly very simular to sprintf and snprintf, but unlike those 2 calls it dynamically allocates memory and hence the programmer doesn’t have to worry about boundschecking. Programmers seem to misunderstand that and think that they’re in the clear if they just use asprintf. Nothing could be further from the truth. See, there’s a corner case. What if malloc (which it uses internally) fails ?

Before I go into that, let’s first see how asprintf works:
int asprintf(char **strp, char *fmt, …)
Basicly you give it a pointer to a pointer as first argument and then a formatstring with some arguments.

so what if asprintf fails ? It returns -1. However, the content of strp upon failure is “undefined” according to the linux manpage. the OpenBSD manpage says the following about it:
“The asprintf() and vasprintf() functions [...] If sufficient space cannot be allocated, these functions will return -1. The value of ret in this situation is implementation-dependent (on OpenBSD, ret will be set to the null pointer, but this behavior should not be relied upon).”

in reality, all the BSD’s set strp to NULL and the glibc implementation (used on most linux distributions) will not change it.

So in linux this makes it interesting for exploitation. Suppose one uses asprintf() and forgets to check the return value (which is very common btw, but more on that later). it leaves strp uninitialized. in most cases strp will be on the stack. Meaning that it’s realistic to assume a user has control over this uninitialized variable (more on this can be found at http://www.felinemenace.org/papers/UBehavior.zip and http://www.blackhat.com/presentations/bh-europe-06/bh-eu-06-Flake.pdf) . So depending on what the application does next with this pointer it might be exploitable. Common scenario’s are passing this pointer to free() at some point (so you don’t get a memory leak) or for example having it point to some path that’ll get used in chmod/chown/open/rename/…. Definatly not a good thing to have. Often (even when asprintf() is used correctly) people tend to forget to free() memory obtained with asprintf(). Sometimes people also incorrectly handle asprintf failure. Since everyone seems to be looking at google code search for vulnerable example’s I did the exact same thing and quickly spotted a nice example of incorrect failure handling:
static char *
gen_dbsuffix(char *db_name, char *sfx)
{
char *dbsuffix;

if (sfx == NULL)
sfx = “.ok”;

asprintf (&dbsuffix, “%s%s”, db_name, sfx);
if (dbsuffix == NULL) {
fprintf (stderr, “gen_dbsuffix: out of memory\n”);
exit(1);
}
return dbsuffix;
}
that’s in MIT kerberos btw. Spotting the error should be obvious (considering what I just said about asprintf). While doing these google code searches I found out that the following abuse asprintf in one way or another:
- gnu radius
- heimdal kerberos
- samba (I’m shocked I tell you !)
- glibc’s pt_chown
- mailutils
- gnu sasl
- MIT kerberos

rsync used to have a nasty bug related to asprintf aswell, but that one recently got fixed.

edit: 2 things I forgat to mention, the first being, that last time I checked dietlibc has simular behavior to glibc’s (with regard to asprintf failing). The 2nd that using asprintf in return into libc exploits can be very usefull, since you get to allocate memory, And put data on it, without needing to check any registers.

2nd edit: It seems I made a terrible mistake. fefe just emailed me and told me dietlibc’s asprintf has always set strp to NULL on failure. I do apolgise for this. Must have confused it with something else.

i never thought i’d see the day ilja van sprundel looked at microsoft code and decided to check into php security :P