evan_tech -- (feh, messed this up once. and somehow i don't actually own this…

01:33 am, 25 Nov 03

(feh, messed this up once.
and somehow i don't actually own this community? so i made it friends-only and now i can't get the post back?)

Spot the security hole (from

conjecture):

struct header {
    int cmd, id, len;
};

[...]

struct header hdr;
char buf[1024], ubuf[1024];
if(read(sock, &hdr, sizeof(hdr)) != sizeof(hdr))
    exit(-1);
if(hdr.len < sizeof(hdr) || hdr.len > 1024)
    exit(-1);
read(sock, buf, hdr.len);
buf[1023] = 0;
snprintf(ubuf, sizeof(ubuf), "command was %s", buf);

xaosenkosmos

Misc problems, none of which are what you want:

Byte ordering, dammit. ICQ design redux, redux.
unchecked return value from the second read()
Weird interaction between sizeof(struct header) and the length of the second read. It appears len includes the header length, so the second read wants to be offset into buf with a decreased length, as well as the <1024 changed to reflect this (or, the if() needs to be changed so it doesn't appear this way. I feel like this is what was actually intended)

I still have yet to spot a real security vulnerability, though.

reply to this

supersat	I suppose a failed call to the second read could copy some random data in memory to ubuf, which could potentially be a security flaw.
reply to this

xaosenkosmos It won't copy bogus data in, the bogus data would have needed to have been there already. Using a temporary variable to store how many bytes were read, and then placing your NUL based on that would be advisable, but, eh. The protocol looks to be ascii with and odd little header.

The only way it would be a problem is if you had previously left "interesting" stuff in-memory before free'ing a buffer. (char *passwd = malloc(512); get_passwd_from_user(passwd); use_passwd(passwd); free(passwd);)

reply to this

zanfur

Weird interaction between sizeof(struct header) and the length of the second read. It appears len includes the header length, so the second read wants to be offset into buf with a decreased length, as well as the <1024 changed to reflect this (or, the if() needs to be changed so it doesn't appear this way. I feel like this is what was actually intended)

Yeah, that looks wonky, but there are no vulnerabilities there ... I don't understand why they bother to check for less than the header size if the header is not included in the second read. Unless, of course, they were trying to simply avoid small values.

reply to this

evan i think it was specifically to do the sizeof() : unsigned trick. it was lame, i agree.

reply to this

zanfur

read() takes an unsigned int, and sizeof returns an unsigned int, but hdr.len is signed. So, if you give a negative number to hdr.len, it will pass both the "hdr.len < sizeof(hdr)" test (because signed ints get transformed to unsigned when compared against an unsigned) and the "hdr.len > 1024" test (because a negative number is certainly less than a positive one).

From Rationale for American National Standard for Information Systems - Programming Language - C:

... the same ambiguity arises whenever an unsigned int confronts a signed int across an operator, and the signed int has a negative value. (Neither scheme does any better, or any worse, in resolving the ambiguity of this confrontation.) Suddenly, the negative signed int becomes a very large unsigned int, which may be surprising --- or it may be exactly what is desired by a knowledgable programmer.

Enyhoo, this leads to much buffer-overflowing nastiness when you try to read an ungodly large number of bytes (which is accomplished by putting any old negative value in hdr.len), especially when you consider that the buffer is on the stack for some godforsaken reason, and not malloc'ed as all good buffers should be. So, running on just about any big-endian platform (i.e. x86), you can overwrite the return address (also stored on the stack), and jump to an arbitrary location in code -- such as inside the long string you've just used to overwrite the return address. Net effect: the malicious user can run any code he/she damn well pleases, at the security level of the process doing the reading.

Of course, this can all be fixed by making the header struct use an unsigned int (size_t would be preferable) instead of a normal int for the len field.

reply to this

zanfur	What's this code from, may I dare to ask?
reply to this

evan	a friend of a friend, i assume. no real code. these sorts of bugs are all the rage these days among "hackers".
reply to this

zanfur

Yup. The only reason I saw it is because I've had to patch way too many of them. The UW gets lots of hack attempts. High bandwidth + loosely monitored == script kiddie heaven. The niftiest one I've seen so far is the commercial ssh bug from a couple years ago -- the "overrun" was writing a 32bit integer to a 16bit integer on the stack, that just so happened to be next to the return address for the function. To exploit it, you also had send your malicious code to another buffer (using another vulnerability that was otherwise benign), and then overrun the return value to point to it. That really shouldn't have compiled, but integer promotion lets it happen. The fix was, of course, to change the 16bit int to a 32bit one.

Automatic, uncasted integer promotion must die. It would make life a lot easier for us security folks.

reply to this

evan	there's a funny slide in my prog langs class where he talks about coercion (converting between types that aren't equivalent / supertypes), and then talks about what a bad idea it is. it says at the bottom "programmers could use float_of_int and similar but they whine about it". (see also: my caml code from a few posts back. i used string_of_int all over it, in, um, the part i didn't post.)
reply to this

conjecture

Automatic, uncasted integer promotion must die. It would make life a lot easier for us security folks.

True dat.

Here's the scarier thing I've been considering: malicious developer (and this is less likely to happen in a volunteer open-source project) wants to insert a special backdoor in software. Intentionally programming errors like this would be the way to go, I think, since it looks like a simple logic error. It's reminiscent of the kernel.org backdoor attempt several weeks ago:

if ((options == (__WCLONE|__WALL)) && (current->uid = 0))
retval = -EINVAL;

Though this is a bit more obvious.

Better yet: stealing from the esteemed Ken Thompson's Trusting Trust paper, why not backdoor a gcc binary on some distro's ftp server to automatically compile bugs or backdoors into all programs?

Agh it makes the head spin.

reply to this

evan

debian servers were recently hacked. they're supposedly fixing it, though i don't know how (it seems it'd be hard to be sure).

but it doesn't bother me too much because i don't think trusting the binaries is much worse than trusting the source distributed by (for example) gentoo. (see also: when gnu discovered their hack, and how you still can't download anything from them.)

i always say to people with pride: i am confident that my home machine is perfectly secure. why? i don't have a network connection. (sure, there are local hacks, but if you're going to break in my house you may as well just carry the box off...)

reply to this

zanfur

Now that I think of it, I actually gave a lecture to the C&C folks a couple years ago on exactly this type of vulnerability, and how it works. The instructor of the class had set up a machine that was supposedly secure, and asked me to come in and speak about Unix security, because he couldn't get Dittrich (who is way, way, waaaaaay more qualified than me) to give the talk. For my first trick, I sat down at it and hacked into it in under 60 seconds. It was a silly show, especially since it was only a lilo trick I used, but it certainly got them all interested.

reply to this

brad Whoa.... fascinating.

reply to this
- zanfur Are you "The" Brad, founder of lj? If so, you're pretty fascinating yourself.
  
  reply to this
  - evan i'm surprised you didn't know him already... you lived in the same dorm at about the same time, both into cs, etc.
    
    reply to this
xaosenkosmos That is slick.

reply to this

evan	i think it was specifically to do the sizeof() : unsigned trick. it was lame, i agree.
reply to this

zanfur	Are you "The" Brad, founder of lj? If so, you're pretty fascinating yourself.
reply to this

evan	i'm surprised you didn't know him already... you lived in the same dorm at about the same time, both into cs, etc.
reply to this

brad	Whoa.... fascinating.
reply to this