commit 7db83d6a303ff372a367afa68895fe1a19abb08f Author: Jacob Appelbaum Date: Fri Mar 19 19:34:36 2010 -0700 DNS issues diff --git a/dns.c b/dns.c index 1a5b39b..11957c4 100644 --- a/dns.c +++ b/dns.c @@ -1394,6 +1394,8 @@ stringToLabels(char *buf, int offset, int n, char *string) memcpy((_d), &(_dd), sizeof(unsigned)); } while(0); #endif + +/* This should be rewritten to use descriptive names for n, *d, m, etc... */ static int labelsToString(char *buf, int offset, int n, char *d, int m, int *j_return) { @@ -1413,6 +1415,7 @@ labelsToString(char *buf, int offset, int n, char *d, int m, int *j_return) if(i >= n) return -1; o = (ll & ~(3 << 6)) << 8 | *(unsigned char*)&buf[i]; i++; + /* XXX Is this possibly full of endless recursion? */ labelsToString(buf, o, n, &d[j], m - j, &k); j += k; break; @@ -1544,6 +1547,7 @@ dnsDecodeReply(char *buf, int offset, int n, int *id_return, error = EDNS_FORMAT; goto fail; } + /* This is missing check to see if i is still in buffer... */ DO_NTOHS(type, &buf[i]); i += 2; DO_NTOHS(class, &buf[i]); i += 2; @@ -1587,6 +1591,14 @@ do { \ for(j = 0; j < ancount; j++) { PARSE_ANSWER("an", fail); + /* XXX used length from network, instead of length from name-> ... + * also... + * For any request polipo makes for say yahoo.com, anything starting + * with y will match if the network returns a lie for the length... + * + * This is pretty sub-optimal... + * + */ if(strcasecmp_n(name->string, b, m) == 0) { if(class != 1) { do_log(D_DNS, "DNS: %s: unknown class %d.\n", commit 039e87e5bba790920101e6c39acab1246a1e49fc Author: Jacob Appelbaum Date: Fri Mar 19 19:06:58 2010 -0700 atomCat issues diff --git a/atom.c b/atom.c index ed18d92..68a5249 100644 --- a/atom.c +++ b/atom.c @@ -91,6 +91,10 @@ atomCat(AtomPtr atom, const char *string) char *s = buf; AtomPtr newAtom; int n = strlen(string); + /* XXX atom-length may be insanely large and overflow with n to be less + * than 128; n may also be a giant string - this is not safe. It's likely + * the case that we should ensure that atom-length is some MAX_SIZE that we + * never grow above... */ if(atom->length + n > 128) { s = malloc(atom->length + n + 1); if(s == NULL) commit 668d8e27a78adbf73194c5795562e1e565fc04c6 Author: Jacob Appelbaum Date: Fri Mar 19 18:58:14 2010 -0700 Initial TODO notes diff --git a/TODO b/TODO new file mode 100644 index 0000000..e07f5ed --- /dev/null +++ b/TODO @@ -0,0 +1,35 @@ +We should enable as much exploit mitigation support as possible + + +The following relates to SOCKS4/SOCKS5 interfacing with Tor. + +Why does Polipo have DNS support at all? + Removal of all DNS code or force to be 127.0.01? + Removal! + Add --disable-dns flag? + Perhaps overload all functions to log, rather than resolve? + This will help us find stray calls that _would_ leak information + +Caching attacks + We need to have a new identity function for Polipo + We already have this in Vidalia and Tor - Polipo needs it too + Browser caching is possible bad news for privacy; is it worse with + Polipo? + + +Skipping nul bytes happens everywhere - this allows for lots of bad stuff. + Explicit length should be set (think pascal strings) + This leads to possible information leakage (but is better than + information leakage with certainty) + +A "metric shitload of" integer mismatches. + Likes we've got a lot of int overflows without reasonable or proper + (certainly unsafe) + +Polipo needs to have a minimal control interface + likely a unix socket to avoid XSRF and other browser injection attacks. + Tor solves this with a password or cookie, etc + Ironically, that password makes Tor nearly impossible to setup + +To prevent local timing/cache checking issues + a separate user per application, per user commit 24d58b9b2da6efd9936737facb95f6eeca35b3af Author: Jacob Appelbaum Date: Fri Mar 19 12:47:28 2010 -0700 debug this diff --git a/Makefile b/Makefile index e6fd085..33fae90 100644 --- a/Makefile +++ b/Makefile @@ -23,8 +23,8 @@ CDEBUGFLAGS = -Os -g -Wall # # _FORTIFY_SOURCE tells the C library to perform additional safety checks. # -# CDEBUGFLAGS += -D_FORTIFY_SOURCE=2 -fstack-protector-all \ -# -Wstack-protector -fwrapv + CDEBUGFLAGS += -D_FORTIFY_SOURCE=2 -fstack-protector-all \ + -Wstack-protector -fwrapv -ansi -Wextra # To prevent some kinds of exploitation at run time, fix up the program's # got table to be read only (also reorders structures) after the first program commit 3311a08029d70f26e1c074da474d17e5ffd51a92 Author: Jacob Appelbaum Date: Sun Dec 27 16:20:51 2009 +0100 linker hardening diff --git a/Makefile b/Makefile index 78c31ab..e6fd085 100644 --- a/Makefile +++ b/Makefile @@ -26,6 +26,11 @@ CDEBUGFLAGS = -Os -g -Wall # CDEBUGFLAGS += -D_FORTIFY_SOURCE=2 -fstack-protector-all \ # -Wstack-protector -fwrapv +# To prevent some kinds of exploitation at run time, fix up the program's +# got table to be read only (also reorders structures) after the first program +# load. eg: ld -z relro -z now +LDFLAGS += -z relro -z now + # To compile on a pure POSIX system: # CC = c89 commit eca86763b2b6d643c51d289b602c76b6a8637a46 Author: Jacob Appelbaum Date: Sat Dec 12 16:16:41 2009 +0200 int fixups diff --git a/chunk.h b/chunk.h index 78d9743..c916a30 100644 --- a/chunk.h +++ b/chunk.h @@ -39,7 +39,7 @@ THE SOFTWARE. #define CHUNKS(bytes) ((unsigned long)(bytes) / CHUNK_SIZE) extern int chunkLowMark, chunkHighMark, chunkCriticalMark; -extern int used_chunks; +extern unsigned long used_chunks; void preinitChunks(void); void initChunks(void); commit 682a25eaa5255192aeb641d84041d1fb965900bc Author: Jacob Appelbaum Date: Sat Dec 12 16:08:15 2009 +0200 Cleanup ints for chunk.c This makes a few variables into unsigned ints diff --git a/chunk.c b/chunk.c index a11cc3f..0daef1d 100644 --- a/chunk.c +++ b/chunk.c @@ -93,11 +93,12 @@ initChunksCommon() } -int used_chunks = 0; +unsigned long used_chunks = 0; static void maybe_free_chunks(int arenas, int force) { + /* CHUNKS returns (unsigned long) */ if(force || used_chunks >= CHUNKS(chunkHighMark)) { discardObjects(force, force); } @@ -291,7 +292,7 @@ typedef struct _ChunkArena { } ChunkArenaRec, *ChunkArenaPtr; static ChunkArenaPtr chunkArenas, currentArena; -static int numArenas; +static unsigned int numArenas; #define CHUNK_IN_ARENA(chunk, arena) \ ((arena)->chunks && \ (char*)(chunk) >= (arena)->chunks && \ @@ -304,7 +305,7 @@ static int numArenas; void initChunks(void) { - int i; + unsigned int i; used_chunks = 0; initChunksCommon(); pagesize = getpagesize(); @@ -333,7 +334,7 @@ static ChunkArenaPtr findArena() { ChunkArenaPtr arena = NULL; - int i; + unsigned int i; for(i = 0; i < numArenas; i++) { arena = &(chunkArenas[i]); @@ -436,7 +437,8 @@ void free_chunk_arenas() { ChunkArenaPtr arena; - int i, rc; + unsigned int i; + int rc; for(i = 0; i < numArenas; i++) { arena = &(chunkArenas[i]); @@ -457,7 +459,7 @@ int totalChunkArenaSize() { ChunkArenaPtr arena; - int i, size = 0; + unsigned int i, size = 0; for(i = 0; i < numArenas; i++) { arena = &(chunkArenas[i]); commit 249e31733b39b667cef16f22b3708f0aef9986c4 Author: Jacob Appelbaum Date: Sat Dec 12 15:47:49 2009 +0200 Ensure we're dealing with ints during our compares We want to ensure that we're always using ints for comparisions with ints. diff --git a/io.c b/io.c index a64bc81..2cc7c08 100644 --- a/io.c +++ b/io.c @@ -458,7 +458,7 @@ do_connect(AtomPtr addr, int index, int port, assert(addr->length > 0 && addr->string[0] == DNS_A); assert(addr->length % sizeof(HostAddressRec) == 1); - if(index >= (addr->length - 1)/ sizeof(HostAddressRec)) + if(index >= (addr->length - 1)/ (int)sizeof(HostAddressRec)) index = 0; request.firstindex = index; @@ -521,7 +521,7 @@ do_scheduled_connect(int status, FdEventHandlerPtr event) assert(addr->length > 0 && addr->string[0] == DNS_A); assert(addr->length % sizeof(HostAddressRec) == 1); - assert(request->index < (addr->length - 1) / sizeof(HostAddressRec)); + assert(request->index < (addr->length - 1) / (int)sizeof(HostAddressRec)); if(status) { done = request->handler(status, event, request); commit 4613aa15cd94c687a7a58fb4200868a7ce9c7865 Author: Jacob Appelbaum Date: Sat Dec 12 15:46:01 2009 +0200 Make AtomPtr's length element an int It was an unsigned short but it was largely used with ints. diff --git a/atom.h b/atom.h index 0b9adeb..ee0b8ec 100644 --- a/atom.h +++ b/atom.h @@ -23,7 +23,7 @@ THE SOFTWARE. typedef struct _Atom { unsigned int refcount; struct _Atom *next; - unsigned short length; + int length; char string[1]; } AtomRec, *AtomPtr; commit 6eb6e906565edaec5785709a9bb788fc8b7d4857 Author: Jacob Appelbaum Date: Sat Dec 12 15:31:31 2009 +0200 Check the return value of fwrite() We weren't checking the return value of fwrite(). It was possible that our logging would fail and we would never know. diff --git a/config.c b/config.c index 60d9ec5..eb24419 100644 --- a/config.c +++ b/config.c @@ -102,6 +102,7 @@ declareConfigVariable(AtomPtr name, int type, void *value, static void printString(FILE *out, char *string, int html) { + int r = 0; if(html) { char buf[512]; int i; @@ -110,7 +111,10 @@ printString(FILE *out, char *string, int html) fprintf(out, "(overflow)"); return; } - fwrite(buf, 1, i, out); + r = fwrite(buf, 1, i, out); + if (r != i) { + printf("String printing failure: wrote %d, expected %d\n", r, i); + } } else { fprintf(out, "%s", string); } commit aaa5b04ca33c2dc16316ec734760fca25148c1a1 Author: Jacob Appelbaum Date: Sat Dec 12 15:27:31 2009 +0200 Check the return value of fwrite() We weren't checking the return value of fwrite(). It was possible that our logging would fail and we would never know. diff --git a/log.c b/log.c index ec5c545..0f37771 100644 --- a/log.c +++ b/log.c @@ -470,9 +470,13 @@ really_do_log_error_v(int type, int e, const char *f, va_list args) void really_do_log_n(int type, const char *s, int n) { + int r = 0; if((type & LOGGING_MAX & logLevel) != 0) { if(logF) { - fwrite(s, n, 1, logF); + r = fwrite(s, n, 1, logF); + if (r != n) { + printf("Logging failure: wrote %d, expected %d\n", r, n); + } } #ifdef HAVE_SYSLOG if(logSyslog) commit 12f9f0d353322b6e0194722e5284666f8158e11a Author: Jacob Appelbaum Date: Thu Dec 10 22:17:07 2009 +0200 Add GCC-specific hardening flags to our Makefile. This adds support for fortified source, moves the integer wrapping option into the main build system and turns on stack smashing protection. diff --git a/Makefile b/Makefile index b802c63..78c31ab 100644 --- a/Makefile +++ b/Makefile @@ -17,6 +17,15 @@ CDEBUGFLAGS = -Os -g -Wall # CDEBUGFLAGS = -Os -Wall # CDEBUGFLAGS = -g -Wall +# Add options to harden the build with specific gcc magic dust to help +# protect against buffer overflow exploits. SSP is available on gcc 4.1.2 +# and above. See: http://www.trl.ibm.com/projects/security/ssp/ +# +# _FORTIFY_SOURCE tells the C library to perform additional safety checks. +# +# CDEBUGFLAGS += -D_FORTIFY_SOURCE=2 -fstack-protector-all \ +# -Wstack-protector -fwrapv + # To compile on a pure POSIX system: # CC = c89 commit 817bd364275942381e37bf2111493de4769e319c Author: Jacob Appelbaum Date: Thu Dec 10 22:13:02 2009 +0200 Add an extra sanity check to avoid memmove segfault diff --git a/client.c b/client.c index 9d6e43c..86c6f20 100644 --- a/client.c +++ b/client.c @@ -998,7 +998,8 @@ httpClientDiscardBody(HTTPConnectionPtr connection) return 1; } - if(connection->reqlen > connection->reqbegin) { + if(connection->reqlen > connection->reqbegin && + (connection->reqlen - connection->reqbegin) > 0) { memmove(connection->reqbuf, connection->reqbuf + connection->reqbegin, connection->reqlen - connection->reqbegin); connection->reqlen -= connection->reqbegin;