Opened 12 years ago

Last modified 7 years ago

#455 closed defect (Fixed)

tor segfaults on 64k-aligned cached-routers file

Reported by: croup Owned by:
Priority: Low Milestone:
Component: Core Tor/Tor Version: 0.2.0.2-alpha
Severity: Keywords:
Cc: croup, arma, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This bug is for svn and the 0.1.2.x tree, which are both affected.

When Tor gets to the end of an 4k-aligned file (page size?), it segfaults in eat_whitespace()

The problem is imminent at routerparse.c:899 when both end and eos are out of bounds. (eos is (always?) out of bounds when the mmap is page-aligned.)
Execution then enters router_parse_entry_from_string(), which calls tokenize_string(s, end...) with end out of bounds. That calls eat_whitespace, which is not safe for non-null-terminated strings (as it has no end pointer or length specifier).

Steps to reproduce:

  1. minimal torrc with RunAsDaemon 0, DataDirectory /some/path/ which points to...
  2. A valid 4096-byte (I've only tested with 2^x * 4096-byte files) cached-routers file (below, or take an existing cached-routers file and chop it, then duplicate/remove/modify exit policy entries until you get to an appropriate size)

I've collected sample cached-routers files of the obvious sizes that seems to trigger the bug on both my x86 and x86-64 machines.
http://70.242.110.86/svn/cr/

The crashes I get are in eat_whitespace(), but the original reporter on IRC was seeing segfaults in 0.1.2.14, routerparse.c:679 -- on PPC.
A representative backtrace from his PPC is at http://www.pastebin.ca/608408
One with a segfault in eat_whitespace (on x86-64) is at http://www.pastebin.ca/608454

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (10)

comment:1 Changed 12 years ago by weasel

Since pastebins do not live forever, I have reproduced the data here:

608408:
(tjg@bastard%) gdb --args /usr/local/bin/tor -f /Users/tjg/.vidalia/torrc
GNU gdb 6.3.50-20050815 (Apple version gdb-477) (Sun Apr 30 20:06:22 GMT 2006)
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB. Type "show warranty" for details.
This GDB was configured as "powerpc-apple-darwin"...Reading symbols for shared libraries ...... done

(gdb) r
Starting program: /usr/local/bin/tor -f /Users/tjg/.vidalia/torrc
Reading symbols for shared libraries .+ done
Jul 08 01:13:27.884 [notice] Tor v0.1.2.14. This is experimental software. Do not rely on it for strong anonymity.
Jul 08 01:13:27.904 [notice] Initialized libevent version 1.1a using method poll. Good.
Jul 08 01:13:27.904 [notice] Opening Socks listener on 127.0.0.1:9050

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x007f5000
0x0006d26c in router_parse_list_from_string (s=0xbffff678, eos=0x7f5000 <Address 0x7f5000 out of bounds>, dest=0x40b6a0, saved_location=SAVED_IN_CACHE) at routerparse.c:679

679 while (cp > *s && (!*cp
TOR_ISSPACE(*cp)))

(gdb) bt
#0 0x0006d26c in router_parse_list_from_string (s=0xbffff678, eos=0x7f5000 <Address 0x7f5000 out of bounds>, dest=0x40b6a0, saved_location=SAVED_IN_CACHE) at routerparse.c:679
#1 0x00068988 in router_load_routers_from_string (s=0x7f4674 "router nuFromBalub 85.214.29.10 9901 0 0\nplatform Tor 0.1.1.23 on Linux i686\npublished 2007-06-19 04:36:02\nopt fingerprint 1F7D E88F E0B9 1A57 B0A7 A122 1636 D14F 3FA3 3042\nuptime 11275509\nbandwidth 1"..., len=3080192, saved_location=SAVED_IN_CACHE, requested_fingerprints=0x0) at routerlist.c:2315
#2 0x00068bd8 in router_reload_router_list () at routerlist.c:375
#3 0x0004d540 in tor_main (argc=3, argv=0xbffff908) at main.c:1234
#4 0x00002184 in _start (argc=3, argv=0xbffff908, envp=0xbffff918) at /SourceCache/Csu/Csu-58.1.1/crt.c:272
#5 0x0000202c in start ()
(gdb) print start
$1 = {<text variable, no debug info>} 0x1ff0 <start>
(gdb) print end
No symbol "end" in current context.
(gdb) print cp
$2 = 0x7f5000 <Address 0x7f5000 out of bounds>
(gdb)

608454
[New Thread 47808772582112 (LWP 20674)]
Jul 08 09:38:15.935 [notice] Tor v0.1.2.14-dev. This is experimental software. Do not rely on it for strong anonymity.
Jul 08 09:38:15.936 [notice] Initialized libevent version 1.3a using method epoll. Good.
Jul 08 09:38:15.936 [notice] Opening Socks listener on 127.0.0.1:9050
Jul 08 09:38:15.936 [notice] Opening Control listener on 127.0.0.1:9051

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 47808772582112 (LWP 20674)]
eat_whitespace (s=0x2b7b59174000 <Address 0x2b7b59174000 out of bounds>)

at util.c:403

403 switch (*s) {
(gdb) print s
$1 = 0x2b7b59174000 <Address 0x2b7b59174000 out of bounds>
(gdb) bt
#0 eat_whitespace (s=0x2b7b59174000 <Address 0x2b7b59174000 out of bounds>)

at util.c:403

#1 0x000000000045fa66 in tokenize_string (

start=0x2b7b59174000 <Address 0x2b7b59174000 out of bounds>,
end=0x2b7b59174000 <Address 0x2b7b59174000 out of bounds>, out=0x8cada0,
where=RTR) at routerparse.c:1742

#2 0x0000000000460500 in router_parse_entry_from_string (

s=0x2b7b59173674 "router nuFromBalub 85.214.29.10 9901 0 0\nplatform Tor 0.1.1.23 on Linux i686\npublished 2007-06-19 04:36:02\nopt fingerprint 1F7D E88F E0B9 1A57 B0A7 A122 1636 D14F 3FA3 3042\nuptime 11275509\nbandwidth 1"...,
end=<value optimized out>, cache_copy=0) at routerparse.c:774

#3 0x0000000000461337 in router_parse_list_from_string (s=0x7fff52780a48,

eos=0x2b7b59174000 <Address 0x2b7b59174000 out of bounds>, dest=0x5ef1a0,
saved_location=SAVED_IN_CACHE) at routerparse.c:699

#4 0x000000000045c176 in router_load_routers_from_string (

s=0x2b7b59173674 "router nuFromBalub 85.214.29.10 9901 0 0\nplatform Tor 0.1.1.23 on Linux i686\npublished 2007-06-19 04:36:02\nopt fingerprint 1F7D E88F E0B9 1A57 B0A7 A122 1636 D14F 3FA3 3042\nuptime 11275509\nbandwidth 1"...,
len=47808775667712, saved_location=SAVED_IN_CACHE,
requested_fingerprints=0x0) at routerlist.c:2333

#5 0x000000000045d0e6 in router_reload_router_list () at routerlist.c:375
#6 0x0000000000446409 in tor_main (argc=3, argv=0x7fff52780d68) at main.c:1234
#7 0x00002b7b58c64374 in libc_start_main () from /lib/libc.so.6
#8 0x00000000004067a9 in _start ()
(gdb) list
398 eat_whitespace(const char *s)
399 {
400 tor_assert(s);
401
402 while (1) {
403 switch (*s) {
404 case '\0':
405 default:
406 return s;
407 case ' ':

comment:2 Changed 12 years ago by arma

The sample files in the url above are 403 for me, so here's another example, which
consistently crashes on moria (64 bit Linux):
http://freehaven.net/~arma/cached-routers

comment:3 Changed 12 years ago by croup

I fixed the permissions on the sample cached-routers files.

This fixes the crash for me. Not sure about the PPC one.

Index: src/or/routerparse.c
===================================================================
--- src/or/routerparse.c (revision 10760)
+++ src/or/routerparse.c (working copy)
@@ -2512,7 +2512,7 @@

}
++counts[tok->tp];
smartlist_add(out, tok);

  • *s = eat_whitespace(*s);

+ *s = eat_whitespace_eos(*s, end);

}


for (i = 0; table[i].t; ++i) {

comment:4 Changed 12 years ago by croup

Rather than the above patch,

Index: src/or/routerparse.c
===================================================================
--- src/or/routerparse.c (revision 10760)
+++ src/or/routerparse.c (working copy)
@@ -908,8 +908,15 @@

elt = extrainfo;

}

} else if (!have_extrainfo && !want_extrainfo) {

  • router = router_parse_entry_from_string(*s, end,

+ char *rdesc;
+ int tlen = end-*s;
+ if (tlen > 4095)
+ log_debug(LD_GENERAL, "Encountered %i-byte router description\n",
+ tlen);
+ rdesc = tor_strndup(*s, tlen);
+ router = router_parse_entry_from_string(rdesc, NULL,

saved_location != SAVED_IN_CACHE);

+ tor_free(rdesc);

if (router) {

signed_desc = &router->cache_info;
elt = router;

comment:5 Changed 12 years ago by croup

Copy to a buffer for both router_parse_entry_from_string and extrainfo_parse_entry_from_string.
Also, reduce the number of alloc/free calls. Maybe this is good, maybe not.

Index: src/or/routerparse.c
===================================================================
--- src/or/routerparse.c (revision 10777)
+++ src/or/routerparse.c (working copy)
@@ -851,6 +851,8 @@

void *elt;
const char *end, *start;
int have_extrainfo;

+ int rdlen, rdesc_max = 8192;
+ char *rdesc = NULL;

tor_assert(s);
tor_assert(*s);

@@ -862,6 +864,7 @@

tor_assert(eos >= *s);


+ rdesc = tor_malloc(rdesc_max);

while (1) {

*s = eat_whitespace_eos(*s, eos);
if ((eos - *s) < 32) /* make sure it's long enough. */

@@ -897,10 +900,19 @@

elt = NULL;


+ rdlen = end - *s;
+ if (rdlen >= rdesc_max) { /* grow rdesc */
+ log_info(LD_GENERAL, "Realloc %i->%i for routerdesc", rdesc_max, rdlen);
+ rdesc_max = (rdlen + 4096) & 0xfffff000; /* only grow mod 4k */
+ tor_realloc(rdesc, rdesc_max);
+ }
+ memcpy(rdesc, *s, rdlen);
+ *(rdesc+rdlen) = '\0';
+

if (have_extrainfo && want_extrainfo) {

routerlist_t *rl = router_get_routerlist();
/* XXXX020 fix this cast to digestmap_t* */

  • extrainfo = extrainfo_parse_entry_from_string(*s, end,

+ extrainfo = extrainfo_parse_entry_from_string(rdesc, rdesc+rdlen,

saved_location != SAVED_IN_CACHE,
(digestmap_t*)rl->identity_map);

if (extrainfo) {

@@ -908,8 +920,8 @@

elt = extrainfo;

}

} else if (!have_extrainfo && !want_extrainfo) {

  • router = router_parse_entry_from_string(*s, end,
  • saved_location != SAVED_IN_CACHE);

+ router = router_parse_entry_from_string(rdesc, rdesc+rdlen,
+ saved_location!=SAVED_IN_CACHE);

if (router) {

signed_desc = &router->cache_info;
elt = router;

@@ -926,7 +938,7 @@

*s = end;
smartlist_add(dest, elt);

}

-
+ tor_free(rdesc);

return 0;

}


comment:6 Changed 12 years ago by croup

A much more comprehensive patch, including a fix for a warning if a directory offers a 0-byte response body while specifying compression.

http://70.242.110.86/svn/parse-6.patch

comment:7 Changed 12 years ago by nickm

I've checked in a minimal workaround for 0.1.2.x as r10846: it's based on Mr. Croup's patch of
"Tuesday, 10 Jul 2007, 1:09am", but it's even simpler. I don't want to backport all the more
sophisticated parsing stuff to 0.1.2.x unless it's really necessary, since the changes
seem pretty subtle and tricky.

I'm going to start working my way through parse-6.patch now.

comment:8 Changed 12 years ago by nickm

I've applied parse-6.patch with minor cleanups as r10847.

comment:9 Changed 12 years ago by nickm

flyspray2trac: bug closed.

comment:10 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.