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;
