Re: [Tails-dev] Switch to Privoxy?

Delete this message

Reply to this message
Author: Jacob Appelbaum
Date:  
To: intrigeri
CC: tails-dev, Jacob Appelbaum
Subject: Re: [Tails-dev] Switch to Privoxy?
On 12/26/2011 04:45 AM, intrigeri wrote:
> Hi fellow Tails developers, hi Jacob!
>
>
> Context
> =======
>
> We've been under repeated pressure to replace Polipo with Privoxy in
> Tails. Most of the time, such attempts were made without much
> knowledge of the Tails context, and were backed with plain wrong
> reasons. Recently, Jacob advised us to remove Polipo; I assume it's
> for good reasons, that are still to be explained in details though.
>


Tor in general is ditching Polipo; we've worked on some patches, I think
almost none of them actually landed upstream. I did an audit with some
other people and we decided that it was pretty difficult to harden it.
Additionally, there was a huge bug that prevented large file download
but I think by now it is fixed.

> Let's see what good reasons we may have to switch to Privoxy; there
> are some. Let's look, with a fresh eye, what are the pros and cons of
> these two pieces of software, as far as Tails is concerned.
>


Sure.

> Preliminary note: in Tails 0.10, only APT, wget and applications that
> obey the HTTP_PROXY / http_proxy environment variables will use
> Polipo. Iceweasel _won't_.
>


Ok.

>
> Polipo is not supported by anyone for anonymity reasons
> =======================================================
>
> Correct.
>
> Who does support Privoxy for anonymity reasons?
>


We're using it for all Tor stuff now when we need an HTTP proxy.

>
> Polipo does not support downloading big files
> =============================================
>
> With our current configuration, "big" means bigger than 64MB:
> https://tails.boum.org/bugs/impossible_to_download_files_bigger_than_64M_with_Iceweasel/
>


Huh, I guess it's still not fixed (!) in newer versions.

> AFAIK, in Tails 0.10, only wget will be affected. This is an annoying
> limitation for command-line users (especially when you learn about it
> *after* having waited hours of stalled download), but not *that*
> critical now that iceweasel is not affected by it anymore.
>


That's enough, I think.

> Time to ship curl (that supports SOCKS5) instead of wget? It was
> suggested to ship curl, plus a simplistic wrapper script called
> "wget", that would pass curl the URL-looking arguments, and would
> abort loudly, pointing at the curl documentation, if anything more
> clever was attempted.
>


I think that would be a useful addition but I'm still a fan of wget.

>
> We already ship Polipo and know its possible weaknesses,
> while we don't know Privoxy weaknesses yet
> ========================================================
>
> Our Polipo configuration was inherited from Incognito. In the times
> when Polipo was shipped in the TBB, it's been under severe scrutiny
> for anonymity / privacy leaks, and more generally for security
> problems. A few issues were discovered; AFAIK all such issues are
> either fixed in the polipo package (1.0.4.1-1.1) shipped in Debian
> Squeeze, or don't affect Tails. This should be double-checked, though:
> https://tails.boum.org/todo/applications_audit/polipo/
>


I had a polipo branch that I gave to Chris long ago. It had a bunch of
fixes and general improvement ideas.

> Issues of the same kind may very well affect Privoxy too. If we were
> to switch to it, we would have to gather existing research results on
> this topic, check them, copy / design / adapt a configuration that
> works around possible issues, etc.; we could probably start from
> Liberté Linux's configuration, but this does not remove the need to
> audit it in any way. Such work is non-trivial, and it's not obvious we
> would gain anything at all, in the end, from a privacy or anonymity
> point-of-view; even, the risk to make things worse exists.
>


I think an audit of Privoxy has a better return; we already know Polipo
isn't great, it has bugs and is likely buggy in dangerous ways. We don't
know that about Privoxy.

>
> Privoxy supports content filtering
> ==================================
>
> Invalid for many reasons. Some of the most obvious ones are:
>
>   - malicious content may be inserted as HTTPS to workaround filtering
>     of the HTTP streams
>   - if body filtering is enabled, Privoxy downloads every resource in
>     full before starting to deliver bytes to the client => much higher
>     latency experienced by the user; yeah, we don't care as we're not
>     really talking of web browsing; but then, what advantage would
>     body filtering provide to the APT and wget kind of usage, exactly?

>


I'd not use it anyway.

>
> Polipo has better latency / user-experience
> ===========================================
>
> Invalid, as we're not thinking, TTBOMK, of inserting a HTTP proxy back
> between Iceweasel and Polipo.
>
>
> Privoxy allows one to designate different proxies for different URLs
> ====================================================================
>
> This would allow command-line applications in Tails to support
> eepsites. My take on this is that it would be welcome, but not
> compelling enough to make the balance weight much more on the side
> of change.
>
>
> Polipo is what's been used by most Tor users for years
> ======================================================
>
> TBB did ship Polipo, the Tor Debian / Ubuntu packages recommend Polipo
> as the preferred alternative on top of Privoxy. Anonymity set++, etc.,
> we all know the deal.
>


We're not shipping either of them anymore, I just checked with helix.
Privoxy is what we'd suggest if anything.

>
> Polipo is not very well maintained upstream
> ===========================================
>
> This is not to be easily ignored. IIRC the upstream author abandoned
> it for a while; it was more or less forked / maintained by the Tor
> project in the meantime; the upstream author came back, but the
> improvements made by Tor were not all merged yet.
>


Right.

> OTOH, apart of the file size download limit, it's not as if we had had
> to report many bugs against it.
>


There were a bunch.

>
> Polipo does caching
> ===================
>
> We don't care, since we're not talking about web browsing anymore here.
>


We should care - leaving a copy of data around might complicate things.

>
> Jacob wants us to remove Polipo
> ===============================
>
> Jacob, you talked of "lots of reliability/security issues" and told us
> that "Privoxy is a better http proxy for most things". Would you
> please elaborate, after reading this very email, _what_ exact issues
> you're thinking of, and _what_ makes Privoxy that much better?
>
>


% git branch
* bugs
master
type-fixup

I've attached the commits from bugs and type-fixup. They're old and I
think Chris at least has seen all of the commits in these branches.

All the best,
Jacob

> Wanna add more pros and cons?
>
> Cheers,


commit 7db83d6a303ff372a367afa68895fe1a19abb08f
Author: Jacob Appelbaum <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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;



commit 24d58b9b2da6efd9936737facb95f6eeca35b3af
Author: Jacob Appelbaum <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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 <jacob@???>
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;