tags 261755 +patch thanks On Sun, Aug 22, 2004 at 11:39:07AM +0200, Thomas Hood wrote: > The changes contemplated look very invasive. How quickly can this > bug be fixed?
Here we go: Hacky, non-portable, but pretty slick & non-invasive,
whatever that means. Now I'm going to check whether it is going to
catch all the cases where malicious characters could be possibly
injected.
This patch (hopefully) solves the problem of remote attacker (server or
otherwise) injects malicious control sequences in the HTTP headers. It
by no mean solves the spoofing bug, which is by nature tricky to address
well.
Cheers,
Jan.
--
"To me, clowns aren't funny. In fact, they're kind of scary. I've wondered
where this started and I think it goes back to the time I went to the circus,
and a clown killed my dad."
--- wget-1.9.1.WORK/debian/changelog 2004-08-22 19:34:16.000000000 +0200
+++ wget-1.9.1-jan/debian/changelog 2004-08-22 19:39:48.000000000 +0200
@@ -1,3 +1,12 @@
+wget (1.9.1-4.local-1) unstable; urgency=medium
+
+ * Local build
+ * Hopeless attempt to filter control chars in log output (see
+ Bug#267393)
+ * This probably SHOULD make it in Sarge revision 0
+
+ -- Jan MinĂĄĹ? <[EMAIL PROTECTED]> Sun, 22 Aug 2004 19:39:02 +0200
+
wget (1.9.1-4) unstable; urgency=low
* made passive the default. sorry forgot again.:(
--- wget-1.9.1.WORK/src/log.c 2004-08-22 19:34:16.000000000 +0200
+++ wget-1.9.1-jan/src/log.c 2004-08-22 19:31:33.000000000 +0200
@@ -63,6 +63,12 @@
#include "wget.h"
#include "utils.h"
+/* vasprintf() requires _GNU_SOURCE. Which is OK with Debian. */
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+#include <ctype.h>
+
#ifndef errno
extern int errno;
#endif
@@ -345,7 +351,49 @@
int expected_size;
int allocated;
};
+
+/* XXX Where does the declaration belong?? */
+void escape_buffer (char **src);
+/*
+ * escape_untrusted -- escape using '\NNN'. To be used wherever we want to
+ * print untrusted data.
+ *
+ * Syntax: escape_buffer (&buf-to-escape);
+ */
+void escape_buffer (char **src)
+{
+ char *dest;
+ int i, j;
+
+ /* We encode each byte using at most 4 bytes, + trailing '\0'. */
+ dest = xmalloc (4 * strlen (*src) + 1);
+
+ for (i = j = 0; (*src)[i] != '\0'; ++i) {
+ /*
+ * We allow any non-control character, because LINE TABULATION
+ * & friends can't do more harm than SPACE. And someone
+ * somewhere might be using these, so unless we actually can't
+ * protect against spoofing attacks, we don't pretend we can.
+ *
+ * Note that '\n' is included both in the isspace() *and*
+ * iscntrl() range.
+ */
+ if (isprint((*src)[i]) || isspace((*src)[i])) {
+ dest[j++] = (*src)[i];
+ } else {
+ dest[j++] = '\\';
+ dest[j++] = '0' + (((*src)[i] & 0xff) >> 6);
+ dest[j++] = '0' + (((*src)[i] & 0x3f) >> 3);
+ dest[j++] = '0' + ((*src)[i] & 7);
+ }
+ }
+ dest[j] = '\0';
+
+ xfree (*src);
+ *src = dest;
+}
+
/* Print a message to the log. A copy of message will be saved to
saved_log, for later reusal by log_dump_context().
@@ -364,15 +412,28 @@
int available_size = sizeof (smallmsg);
int numwritten;
FILE *fp = get_log_fp ();
+ char *buf;
+
+ /* int vasprintf(char **strp, const char *fmt, va_list ap); */
+ if (vasprintf (&buf , fmt, args) == -1) {
+ perror (_("Error"));
+ exit (1);
+ }
+
+ escape_buffer (&buf);
if (!save_context_p)
{
/* In the simple case just call vfprintf(), to avoid needless
allocation and games with vsnprintf(). */
- vfprintf (fp, fmt, args);
- goto flush;
- }
+ /* vfprintf() didn't check return value, neither will we */
+ (void) fprintf(fp, "%s", buf);
+ }
+ else /* goto flush; */ /* There's no need to use goto here */
+/* This else-clause purposefully shifted 4 columns to the left, so that the
+ * diff is easy to read --Jan */
+{
if (state->allocated != 0)
{
write_ptr = state->bigmsg;
@@ -384,8 +445,12 @@
missing from legacy systems. Therefore I consider it safe to
assume that its return value is meaningful. On the systems where
vsnprintf() is not available, we use the implementation from
- snprintf.c which does return the correct value. */
- numwritten = vsnprintf (write_ptr, available_size, fmt, args);
+ snprintf.c which does return the correct value.
+
+ With snprintf(), this probably doesn't hold anymore. But this is Debian,
+ so who cares. */
+
+ numwritten = snprintf (write_ptr, available_size, "%s", buf);
/* vsnprintf() will not step over the limit given by available_size.
If it fails, it will return either -1 (POSIX?) or the number of
@@ -420,7 +485,7 @@
if (state->bigmsg)
xfree (state->bigmsg);
- flush:
+} /* flush: */
if (flush_log_p)
logflush ();
else
pgph4snD1HOqU.pgp
Description: PGP signature
